feat: add getSearchResults endpoint#233
Conversation
|
@arnestaphorsius is attempting to deploy a commit to the achievements-app Team on Vercel. A member of the Team first needs to authorize it. |
|
Hi @arnestaphorsius! Sorry for the delay. I'll get a review in for this within 24 hours. |
wescopeland
left a comment
There was a problem hiding this comment.
Thanks, there's some good stuff here. I have some feedback that's mostly minor.
| nextCursor | ||
| }; | ||
|
|
||
| url.searchParams.set("variables", JSON.stringify(variables)); |
There was a problem hiding this comment.
DX: variables includes undefined values. We're destructuring pageSize, pageOffset, and nextCursor from options, but when they're not provided, we're stuffing undefined into variables and shipping it as JSON.
JSON.stringify() strips undefined values automagically, so this does technically work, but it means we're relying on a serialization side effect to clean up the variables data.
I think we should be more explicit about what we send. Maybe something like:
const variables: Record<string, unknown> = {
countryCode,
languageCode,
searchTerm,
...(pageSize !== undefined && { pageSize }),
...(pageOffset !== undefined && { pageOffset }),
...(nextCursor !== undefined && { nextCursor })
};There was a problem hiding this comment.
Sure, let's strip them then. I don't really see a "nicer" way to handle it than what you suggested, so I'll go with that.
| * NOTE: This endpoint should be called WITHOUT authorization to receive price information. | ||
| * When called with authorization, price data may be filtered or unavailable. |
There was a problem hiding this comment.
DX: This makes sense. What gives me pause is the function signature here is inconsistent with every other function in psn-api.
Every other endpoint takes authorization as the 1st param. It makes sense this one is intentionally different, but it may confuse a consumer unless they read this JSDoc. They may see:
getPurchasedGames(authorization, ...)
getRecentlyPlayedGames(authorization, ...)
getSearchResults(searchTerm, ...) // wait, what?
Since call() already handles authorization as optional, we could accept it as an optional 1st param for signature consistency. Consumers who want prices omit it, consumers who don't care pass it in. The JSDoc warning stays, but the API shape is now uniform.
There was a problem hiding this comment.
Yea, consistency would be nice here.
But making the first parameter optional would force the other parameters to be optional too, and that would make the usage very confusing in my opinion. On top of that the endpoint also returns an error if you do not send a searchTerm.
I could have it as a required parameter, and simply not pass it to call(), of course explaining that in the JSDoc. WDYT?
| countryCode: string; | ||
| languageCode: string; |
There was a problem hiding this comment.
DX: We should document the expected format of these values.
The Accept-Language spec uses subtags like en_US, nl-NL, etc.
What if someone passes countryCode: "US" and languageCode: "en-us"? Given there's no validation on these inputs, some sort of JSDoc here with expected format, probably via @example, would help.
There was a problem hiding this comment.
Good suggestion. I've added a full description of the variables in the options object with example values.
There was a problem hiding this comment.
Testing: Recommend adding a test case which exercises nextCursor and pageOffset.
# Conflicts: # .gitignore
83b0e8d to
6dbf831
Compare
This PR adds the graphql endpoint getSearchResults to the library.
The endpoint:
You can see the endpoint in action here: https://store.playstation.com/en-us/search/batman/1. You have to use pagination to see it (first page load is server side).
@wescopeland Sending along an Authorization header breaks the price information of this endpoint (it will be null in the response), which is why I made it optional downstream. Obviously that's the fastest way, but perhaps you prefer a separate
callmethod to make a distinction between calls with or without authorization.