Don't require custom fetch options to not be in Request to pass them explicitly

[dkokotov]

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 the next property.

[sholladay]

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 Request class that exposes request.next, and Ky is correctly passing that on, but you still want us to pass options.next when we call fetch(request, options) even though the request already has it? Why would that even be necessary? Does their fetch() just ignore the request properties when the options argument is also present? If so, that sounds like a design flaw in their fetch() implementation.

IMO, if they are going to add a custom option, they need to support passing it the same way as other options, through fetch(url, options), fetch(request, options), or fetch(request). All of those should work, with the option in either argument (or both). And if they do, then Ky's existing implementation should also work.

[dkokotov]

I am not sure what you mean by

heavily depends on TypeScript, which is often lagging or incomplete

I don't think anything here is related to typescript. The question is basically "which unknown properties should be passed on to the wrapped fetch's second options parameter rather than the first request parameter, to deal with frameworks which accept additional parameters via options but not via request". The two examples of such frameworks we have is Bun (with the proxy parameter as described in #516) and Next.js with the next parameter (which was the motivation for my issue #541).

The initial fix for the Bun issue - #536 - attempted to use the heuristic of assuming that only properties that are not in Request are extra properties, to avoid hardcoding a list of "known" parameters - but this already caused the problem reported by #539 - and the fix in #540 was to revert to having a hardcoded list. And my point is simply that with that done, the extra check that the property is also not in Request is unneeded.

I think your point that Next.js and Bun could have chosen to support to support those extra params in request as well as options seems reasonable to me - Next.js seems to have many issues with inconsistent implementation of the various fetch / Request parameter permutations. But you already did choose to implement the earlier fixes rather than take the stand that it should be on Bun to address this in the case of #516 - this is just actually trying to make the fix also work for Next.js case.

[dkokotov]

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

[sholladay]

I don't think anything here is related to typescript.

This PR removes code that is meant to be a backup in case the requestOptionsRegistry fails to catch options that are handled by the Request class. Since we're leaning on TypeScript to determine the completeness of the requestOptionsRegistry, this PR by definition makes us more reliant on TypeScript, as we will no longer have the backup mechanism that dynamically checks the request object.

I would be okay with that if TypeScript was always up to date, but in my experience, people start using new JavaScript APIs (including fetch() options) long before they are added to the TypeScript definitions.

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 fetch() overrides the first argument. So we use the request object as a single source of truth, make any changes to headers and other options there and then don't pass those options in the second argument to fetch(), in order to avoid accidentally overriding our own work.

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.

[sindresorhus]

Bump :)

[radum]

Would be awesome if we can land this. Is there anything that is blocking it and we can help with?

[sholladay]

@radum we're going to need a new champion for this. Feel free to open a new PR. Ideally one that takes into account my comment above, but if not that's okay. As long as there's a proposed solution with passing tests, I will try to help optimize its reliability.

[radum]

@sholladay I completely understand your comment but I don't see a way forward unless we have some sort of flag that lets the user patch the request with more stuff.

There are several ways of doing that of course. But is that something we are ok to add to ky? If the answer is yes then before we start typing any code we can agree of an api form that would support this.

Looking forward for your thoughts.

I am currently on a large codebase and ky is at its core but we are using Next 14, and with Next 15 looming over it becomes even more important for us to be able to do this.

[sholladay]

I don't want to make any of this visible to the user, it's an implementation detail.

We could add a hardcoded exception for problematic request properties. Or we could switch to using a list of known request properties that we want to pass along.

Either way is fine with me and should solve the problem.

[radum]

I see ok that makes sense.

In that case is this just a matter of doing if (!(key in requestOptionsRegistry) && !(key in kyOptionKeys) && (!(key in request) || key in vendorExceptions)) and we create a list of those exceptions like we have it for kyOptionKeys for example?