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

Pass unknown options to fetch #536

Merged

Conversation

kdelmonte
Copy link
Contributor

@kdelmonte kdelmonte commented Oct 17, 2023

This PR implements option number 2 of this comment by @sholladay.

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.

Fixes #531
Fixes #516

Also, tested this against Next.js v13.5.5 patched fetch and I can verify that it works.

source/core/Ky.ts Outdated Show resolved Hide resolved
@kdelmonte kdelmonte force-pushed the pass-unknown-options-to-fetch branch from 76d87eb to 7b2516b Compare October 24, 2023 07:36
Copy link
Collaborator

@sholladay sholladay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great implementation. It's about as simple as you could make it, given the silly dance we have to do here.

I was originally thinking we would maintain a list of known request options to use for filtering, which I really did not want to do. But I see that the request class exposes most (all?) of its constructor options as instance properties and you are using that to avoid hardcoding the list. Nice work.

Technically, we could allow the Ky options to be passed on to fetch() and it likely wouldn't cause any trouble, at least in browsers. Although, there could be environments where fetch() throws for unexpected options. So doing it this way is certainly safer. 👍

@sindresorhus sindresorhus merged commit e93bc6d into sindresorhus:main Oct 24, 2023
1 check passed
@doroved
Copy link

doroved commented Oct 24, 2023

image Thanks, everything works, but now how to deal with TS? In bun fetch the type for proxy is there, how to use it in ky?

@doroved
Copy link

doroved commented Oct 24, 2023

image

Type in bun

@kdelmonte
Copy link
Contributor Author

kdelmonte commented Oct 27, 2023

This PR caused #539 which is being resolved with #540.

@doroved
Copy link

doroved commented Oct 27, 2023

This PR caused #539 which is being resolved with #540.

So what to do with TS types? I provided information above

@kdelmonte
Copy link
Contributor Author

@doroved , please create a separate issue and provide all the details of what you think needs to be done.

@loxK
Copy link

loxK commented Oct 28, 2023

This PR caused #539 which is being resolved with #540.

A couple of hours of debugging an app to finally find out that regression....

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 this pull request may close these issues.

Ky does not pass custom options down to the native fetch Full compatibility with Bun
5 participants