-
-
Notifications
You must be signed in to change notification settings - Fork 363
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add searchParams
option
#57
Add searchParams
option
#57
Conversation
index.js
Outdated
@@ -86,13 +86,14 @@ const timeout = (promise, ms) => Promise.race([ | |||
]); | |||
|
|||
class Ky { | |||
constructor(input, {timeout = 10000, hooks = {beforeRequest: []}, throwHttpErrors = true, json, ...otherOptions}) { | |||
constructor(input, {timeout = 10000, hooks = {beforeRequest: []}, throwHttpErrors = true, searchParams = '', json, ...otherOptions}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
searchParams,
(no empty string)
index.js
Outdated
this._retryCount = 0; | ||
|
||
this._options = { | ||
method: 'get', | ||
credentials: 'same-origin', // TODO: This can be removed when the spec change is implemented in all browsers. Context: https://www.chromestatus.com/feature/4539473312350208 | ||
retry: 2, | ||
searchParams: new URLSearchParams(searchParams), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is useless
Thanks for the detailed review @szmarczak 👍🏻 |
And it is, but it isn't a global. We would need to access it through See #51. We can't do anything about that yet. |
index.js
Outdated
this._input = this._options.prefixUrl + this._input; | ||
const url = new URL(this._options.prefixUrl + this._input); | ||
if (searchParams) { | ||
url.search = searchParams instanceof URLSearchParams ? searchParams : new URLSearchParams(searchParams).toString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about:
if (typeof searchParams === 'string') {
url.search = searchParams;
} else {
url.search = new URLSearchParams(searchParams).toString();
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your suggestion looks good to me, except for the fact that if searchParams
is an instance of URLSearchParams
you end up initiating a clone of it for no reason.
We could do this:
if (typeof searchParams === 'string' || searchParams instanceof URLSearchParams) {
url.search = searchParams;
} else {
url.search = new URLSearchParams(searchParams).toString();
}
Which both doesn't parse strings nor URLSearchParams
objects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lemme fix your code:
if (typeof searchParams === 'string') {
url.search = searchParams;
} else if (searchParams instanceof URLSearchParams) {
url.search = searchParams.toString();
} else {
url.search = new URLSearchParams(searchParams).toString();
}
I hope it works as expected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't checked that so I didn't know, thanks for the explanation :) If you made a commit with that it would be great :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a flash ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up doing:
if (typeof searchParams === 'string' || searchParams instanceof URLSearchParams) {
url.search = searchParams;
} else if (searchParams) {
url.search = new URLSearchParams(searchParams).toString();
}
Since if the searchParams
option isn't defined, the test prefix-url › prefixUrl option
fails (which makes sense).
@szmarczak Can you mark #57 (comment) as resolved? We've already addressed the issue in the discussion on #57 (comment) and in 6bb65d0. |
How about handling this one? ky('https://example.com/?a=1&b=2', { searchParams: { a: 2 } }) |
@maple3142 Currently, all query parameters get ignored when the |
This looks good, but I need some feedback on what behavior to use when a plain |
And do anyone disagree that it should replace the search params already in the URL instead of merging them in? |
@sindresorhus I've implemented your suggestion. Currently only strings and numbers are allowed, although we could probably support any primitive type excluding objects and arrays. |
It should probably mirror what |
Don't forget to update the TypeScript types (see #49 as an example). The build failure should be easy to fix now that the Cypress tests are up and running. :) |
And also add a test to https://github.com/sindresorhus/ky/blob/master/index.test-d.ts |
Can you also fix the merge conflict? |
@sholladay I've never used Cypress before, and so I'm not sure how to fix the build failure. |
@itaisteinherz No, we'll only use Cypress when it's impossible to use Node.js testing. Accept my code suggestion above and then rebase from master branch and the test should pass ;) |
Co-Authored-By: itaisteinherz <itaisteinherz@gmail.com>
Co-Authored-By: itaisteinherz <itaisteinherz@gmail.com>
Co-Authored-By: itaisteinherz <itaisteinherz@gmail.com>
Co-Authored-By: itaisteinherz <itaisteinherz@gmail.com>
Done, but I still had to merge from master because I couldn't figure out how to rebase properly 😓 |
LGTM. |
This still needs TypeScript typings, like mentioned in #57 (comment). Otherwise, the code changes are looking good. |
@sindresorhus I'll finish the TypeScript typings in an hour or so. |
Done in 8c8c1ca 👌🏻 |
You missed #57 (comment), but I can fix that for you. |
Thank you for working on this :) |
Fixes #54