Skip to content
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

Full compatibility with Bun #516

Closed
doroved opened this issue Jul 13, 2023 · 9 comments · Fixed by #536
Closed

Full compatibility with Bun #516

doroved opened this issue Jul 13, 2023 · 9 comments · Fixed by #536

Comments

@doroved
Copy link

doroved commented Jul 13, 2023

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
Copy link
Owner

Ky just passes the option through to Fetch:

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

@doroved
Copy link
Author

doroved commented Jul 14, 2023

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
Copy link
Collaborator

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
Copy link
Author

doroved commented Jul 24, 2023

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
Copy link
Collaborator

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

@doroved
Copy link
Author

doroved commented Jul 29, 2023

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.

@doroved doroved closed this as completed Jul 29, 2023
@doroved doroved reopened this Jul 29, 2023
@sholladay
Copy link
Collaborator

sholladay commented Aug 8, 2023

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
Copy link
Author

doroved commented Aug 9, 2023

@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
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants