-
-
Notifications
You must be signed in to change notification settings - Fork 366
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 support for relative URLs and searchParams even in non-browsers #271
Add support for relative URLs and searchParams even in non-browsers #271
Conversation
07cc774
to
c6714b5
Compare
a71750d
to
07cc774
Compare
9371d1e
to
98ac573
Compare
98ac573
to
fa7aedd
Compare
Have you considered opening an issue over at https://github.com/whatwg/url ? |
🙌🏻 |
@@ -272,8 +272,8 @@ class Ky { | |||
this.request = new globals.Request(this._input, this._options); | |||
|
|||
if (this._options.searchParams) { | |||
const url = new URL(this.request.url); |
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.
Sorry I'm late to the party. Does this change anything? this.request.url
is an absolute URL anyway... I'm just asking because I don't like RegExps.
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 don't like regex either. this.request.url
can be a relative URL in some environments, e.g. Node and Deno.
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.
Then why not this?
const requestUrl = this.request.url;
const searchIndex = requestUrl.indexOf('?');
const urlWithoutParameters = searchIndex === -1 ? requestUrl : requestUrl.slice(0, searchIndex);
const url = `${urlWithoutParameters}${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.
I believe your code would remove the hash fragment from a URL, unlike the new URL()
code we had before.
Granted, the hash fragment shouldn't be sent to the server anyway, so that shouldn't really matter. But I figure we should be spec compliant and unobtrusive by only modifying the search params and nothing more. It's possible that a custom fetch
function might care about the hash fragment, for example.
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.
Hmm... Fair point. We could also indexOf('#')
but that would add more complexity. The regex solution is much clearer.
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.
Yep, my thoughts exactly. I did add tests that assert we don't modify the hash fragment. So if someone comes up with a clean non-regex solution, go ahead and give it a shot. It'll be checked thoroughly by the tests.
Hi, @asaneh. Is there a reason you linked those issue numbers here? It's unclear without any context. |
Given his activity on other repos I suppose he's just a spammer. |
Closes #236
This PR aims to solve the last problem I am aware of related to URL resolution in Ky. Historically, we've used the WHATWG URL constructor (
new URL()
) in various situations to parse, resolve, and modify URLs. But it's been highly problematic because that API only supports absolute URLs and throws an error if given anything else. It's difficult for Ky to know what base URL to resolve against just to satisfy that constraint, even in browsers (e.g. web workers do not have access todocument.baseURI
), not to mention Deno, Node, React Native, and other environments. This makesnew URL()
quite a footgun.I have chipped away at these kinds of URL resolution problems over time (e.g. #59 and #180). These days, we mostly rely on the much better behaved
Request
class which does support relative URLs, however it doesn't cover all the same use cases. There was one remaining place we still usednew URL()
: when the user providessearchParams
to modify theinput
URL. This is the kind of thingnew URL()
is perfectly suited for... unfortunately, as mentioned, it's just too finicky. Hopefully, someday they'll add support for relative URLs. Until then, we'll do our own string replacement.RIP to the WHATWG URL API. ⚰️