searchParams without base url break

[hsnali]

Hi,
So before 0.16, optional searchParams would use the browser base to create a new URL, now this is not the case so there is no way to use searchParams as I see it without fully realized url string.

Am I using this wrong or is this an issue for anyone else?

ky/index.js

Line 240 in bb46ca8

const url = new URL(this.request.url);

[sindresorhus]

Can you include an example of the code (your usage) that used to work?

[hsnali]

Sure,

const data = await ky.get('/api/products', {searchParams: {limit: 12}}).json();

Should result in /api/products?limit=12 in the fetched request url.

But ends up throwing the following error: TypeError: "/api/products is not a valid URL."

[sholladay]

I cannot reproduce this in a browser, however there are conditions under which this can happen.

We still respect the document base URL, we just rely on new Request() to do it for us on this line. In most environments, this.request.url should be a fully resolved URL (meaning we can pass it to new URL() as the only argument, without problems).

Server-side runtimes and Request polyfills might decide not to resolve the URL, especially if there is no DOM or other base URL to use. For example, Deno's implementation of Request currently does not resolve the input URL, so if you give it a relative URL, then request.url will still be that same relative URL.

Now, given that (annoyingly) relative URLs are not supported by new URL(), this does mean that in certain environments you cannot use a combination of relative URLs and the searchParams option. I would agree that this is quirky behavior and we could at least throw a better error message.

However, it's worth noting that:

1. Ky is intended to be used in a browser. Support for other environments is best-effort.
2. Our old implementation in v0.15 and below was also subject to this limitation and was brittle in other ways that were solved by the changes in v0.16 (such as the dependency on a global document object).

I'm not sure exactly why this would be happening to you with v0.16 without happening in earlier versions, but most likely there is no base URL in your environment or something is wrong with the Request implementation.

The only reliable fix for this that I can think of is to hack around the new URL() issue by resolving against a fake host and then trying to filter that fake host back out of the request URL. Something like...

if (this._options.searchParams) {
    const fakeBaseUrl = 'https://fake-host.invalid';
    const url = new URL(this.request.url, fakeBaseUrl);
    url.search = new URLSearchParams(this._options.searchParams);
    // Convert the URL back to a relative URL if it was previously relative.
    // We only needed an absolute URL due to limitations in `new URL()`
    if (url.origin === fakeBaseUrl) {
        url = url.pathname + url.search + url.hash;
    }
    this.request = new globals.Request(new globals.Request(url, this.request), this._options);
}

It's not clear to me what host would be used in such an environment where this code path would help, but hopefully the fetch() implementation would be able to figure it out.

Also, this hack scares me a bit. I'd be worried that data would somehow accidentally get sent to the wrong host, which would be a security nightmare. We could have a good test suite around this behavior, but I can't say I'm excited about it.

[hsnali]

Thanks for looking into it Seth, looks like the issue is not with ky, but with the mock environment we use and due to it working in the previous version I made the wrong conclusion. You are right, its definitely due to an out of date implementation of Request in this case, not pure browser implementation.

Your suggestions are interesting, and could definitely be useful in my specific mock case, but lets close this issue off.