-
-
Notifications
You must be signed in to change notification settings - Fork 334
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
Don't require custom fetch options to not be in Request to pass them explicitly #542
base: main
Are you sure you want to change the base?
Don't require custom fetch options to not be in Request to pass them explicitly #542
Conversation
The code looks fine to me but I'm skeptical this will be reliable in practice, as it so heavily depends on TypeScript, which is often lagging or incomplete. if I understand correctly, Next.js has a monkeypatched IMO, if they are going to add a custom option, they need to support passing it the same way as other options, through |
I am not sure what you mean by
I don't think anything here is related to typescript. The question is basically "which unknown properties should be passed on to the wrapped The initial fix for the Bun issue - #536 - attempted to use the heuristic of assuming that only properties that are not in I think your point that Next.js and Bun could have chosen to support to support those extra params in |
I should note that I ended up switching over to using wretch in the meantime. This is no knock on ky - I think both libraries are good, and I don't have a strong preference in terms of API design. But this really is kind of a blocker - those Next.js revalidation props are pretty key to making the most of their new app router's caching system - and I needed to keep making progress on what I was working on. So you can feel free to close the issue/PR if you like - I just wanted to explain my thinking above, and sorry for being long-winded |
This PR removes code that is meant to be a backup in case the I would be okay with that if TypeScript was always up to date, but in my experience, people start using new JavaScript APIs (including In case it's not obvious, the reason we want to maintain this strict separation of "request" options and "unknown" options, ideally with no duplication between them, is because anything that gets passed in the second argument to Anyway, if you do end up having some time and get the tests passing, I'll see if I can come up with a solution that doesn't overly rely on TypeScript while also not breaking #541. |
Bump :) |
Would be awesome if we can land this. Is there anything that is blocking it and we can help with? |
Fixes #541.
Since #540 already added explicit checks against the known keys of
RequestInit
, I don't think the!(key in request)
check is still needed, and removing it fixes the issue mentioned.I can try to add a test if the general idea seems reasonable - I think to do so, I would need to simulate what Next is doing with patching
Request
to have thenext
property.