sindresorhus / ky

🌳 Tiny & elegant JavaScript HTTP client based on the Fetch API
MIT License
11.91k stars 342 forks source link

Full compatibility with Bun #516

Closed doroved closed 9 months ago

doroved commented 1 year ago

Hi, it looks like ky is working in Bun (bun.sh), you need support for Bun fetch types, for example Bun implements the ability to use a proxy in a request

const response = await fetch(targetUrl, {
    proxy: `http://${proxyUsername}:${proxyPassword}@${proxyHost}:${proxyPort}`,
  })

But it doesn't work through ky. Please adapt the library for Bun

const response = await ky(targetUrl, {
    proxy: `http://${proxyUsername}:${proxyPassword}@${proxyHost}:${proxyPort}`,
  })
sindresorhus commented 1 year ago

Ky just passes the option through to Fetch:

The input and options are the same as fetch, with some exceptions:

doroved commented 1 year ago

Ky just passes the option through to Fetch:

The input and options are the same as fetch, with some exceptions:

Yes, it is, only proxy is not a native option it is a development of the Bun team, maybe that's why it doesn't work in Ky. Anyway, please check it, it would be great if you release an update with a solution to this problem and support for Bun's fetch types in ky.

https://codesandbox.io/p/sandbox/ky-bun-3rg5wm

image
sholladay commented 12 months ago

Just a hunch... does it work if you don't use .json()? Ky does a bit of extra processing on the options if you use .json().

doroved commented 12 months ago

Just a hunch... does it work if you don't use .json()? Ky does a bit of extra processing on the options if you use .json().

Without calling .text()|.json(), I can't check the response to be sure.

image

@sindresorhus Are you planning to address this issue)?

sholladay commented 11 months ago

What I should have said is to use response.json() instead of the ky().json() shortcut. They are different.

doroved commented 11 months ago

What I should have said is to use response.json() instead of the ky().json() shortcut. They are different.

Show me an example, it's not clear.

sholladay commented 11 months ago

Your code example does this:

const json = await ky(targetUrl, { proxy }).json();
console.log(json);

^^ that is what we refer to as the "json shortcut". It's a shortcut because you are calling .json() on a promise, which is normally not possible. Promises, including the one returned by fetch(), do not have a .json() method. But ky() returns a special promise that has an added .json() method for convenience.

Compare that with fetch(), notice how you first have to await its promise for a response before calling .json() on the response and then you have to await that too, since json() returns another promise.

const response = await fetch(targetUrl, { proxy });
console.log(await response.json());

Ky's json shortcut is optional. You can still the await the response and call its .json() method, just as you do with fetch(). Here's what it looks like using Ky without the json shortcut:

const response = await ky(targetUrl, { proxy });
console.log(await response.json());

In theory, there should not be much difference between all of the above constructs. But Ky does do a bit of extra work when you use the json shortcut, including adding an Accept header to the request.

I would like to know if using Ky without the json shortcut changes the result of your test, so that we can rule that out as a cause.

doroved commented 11 months ago

@sholladay

I tried this option and unfortunately nothing changed, the proxies still don't work.

const response = await ky(targetUrl, { proxy });
console. log(await response.json());
sholladay commented 11 months ago

Okay, I think I know what's happening here.

While it's true that Ky tries to pass any unknown options through to fetch (such as this non-standard proxy option), we do so via the Request class. We don't actually pass options to fetch() directly, but rather we pass options to new Request() and then pass the request instance to fetch(). I think this is what's causing the problem.

I suspect that this won't work:

const request = new Request(targetUrl, { proxy });
const response = await fetch(request);
console. log(await response.json());

That is essentially what Ky is doing. The Bun team probably didn't anticipate it would need to be passed that way.

I see two possible solutions. Both should probably be implemented but either would suffice for your particular case.

  1. Bun adds proxy support to its Request class. This would be convenient for their users more broadly, for the same reasons that Ky uses Request internally.
  2. Ky starts passing unknown options directly to fetch instead of relying on the Request class to do it for us. This is the right thing for Ky to do but might prove to be a little tricky. We would need to either pass all options to fetch, which could inadvertently override some of our options processing logic that's done with the Request class, or have some kind of filtering system to only pass unknown options to fetch, which would presumably involve some kind of hardcoded list of known options (which would have to include all options that the Request class understands) and that doesn't sound fun.

Solution 1 is likely easier to implement, so I think we should start there. File a feature request with Bun asking for proxy support in the Request class and paste a link to the issue here.