Passing { next: { revalidate, tags } } options to ky does not work in NextJS edge (at least with dev server)

[dkokotov]

The fix for passing unknown options to fetch (#536) does not work correctly for the next options used by Next.js to control cache revalidation, when using edge runtime on dev server.

I think this is because the ky implementation relies on passing only keys that are not in Request in the second argument of fetch. however, it looks like Next.js patches Request to also have a next key (I think this may be the culprit, though not totally sure: https://github.com/vercel/next.js/blob/canary/packages/next/src/server/web/sandbox/context.ts#L370), so this results in next key not being included.

I verified it with a wrapper fetch function that logs the second input (similar to how the test in the PR above is written). I can see that keys other than next are correctly passed, and in the node runtime the next key is passed, but in the edge runtime it is not.

I have not yet tested if this problem is present in the production edge environment, or only dev server.

[dkokotov]

One other thing worth mentioning is that it's a bit unergonomic to pass those unknown options to ky, since if I pass them directly, the type checker complains.

I either have to do one of two things

ky(someUrl, { next: { revalidate: 10000 } } as any);
//or 
const options = { next: { revalidate: 10000 } };
ky(someUrl, { ...options });

which work but are not ideal.

[sindresorhus]

// @kdelmonte

[kdelmonte]

I'll take a look.

[kdelmonte]

Relates to:
vercel/next.js#41531
vercel/edge-runtime#688

[kdelmonte]

@dkokotov I've been following this thread and it seems like this this is a next.js issue that shouldn't require any modifications to ky. Please clarify when you can.

Also, regarding the typescript error that you are getting, please provide a reproduction repository.

[dkokotov]

@kdelmonte I am following the thread you linked as well - since I am one of the people affected by that issue. It looks like Next.js is working on a fix, but I am waiting to see how it plays out - the linked PR does not yet have a fully working fix.

However, I don't think that issue is related to this one - other than the coincidence that both affect edge runtime on Next.js dev server.

the problem described in that issue has to do with the fact that when you do something like

const req1 = new Request('https://example.com', {
    method: 'POST',
    headers: { 'X-Example-Header': 'Foo' }
  });
  const req2 = new Request(req1);

req2 should carry over properties like method, headers, etc from req1, but for edge runtime in nextjs dev server, it does not. This ends up breaking ky when you do something like ky.post('https://example.com', { json: { foo: 'bar' } }) because ky's implementation for the json shortcut relies on the Request copy constructor.

The problem described in this issue is with passing additional properties to fetch that neither ky nor the web-standard fetch knows about. something like ky.get('https://example.com', { next: { revalidate: 10000 } }). This is important to be able to use the cache/revalidation capabilities of Next.js's app router.

This was previously reported in #531 and was attempted to be fixed in #536. And the fix mostly works - except in edge runtime in nextjs (it definitely does not work in edge with dev server. and I cannot test it with edge on Vercel prod, because they are currently having issues deploying edge functions). But the reason does not have anything to do with the nextjs issue above. As I mentioned in my original post, I think it has to do with the way #536 is implemented. Specifically in https://github.com/sindresorhus/ky/pull/536/files#diff-1d28e040cea335f5a22aa9c3f678f5d00b353e3ce966dc0978a91fe56f240b04R301 it only passes to fetch nonRequestOptions - which it computes in https://github.com/sindresorhus/ky/pull/536/files#diff-4a531f309dc0b2c2d0c4d04fae930a1731ebdc2cd5cfd204a1e98ea5a71e3f2fR10 as any keys that are not the known special ky-only properties, and not properties present in Request. My theory (which I think is supported by https://github.com/vercel/next.js/blob/canary/packages/next/src/server/web/sandbox/context.ts#L370) is that Next.js patches Request to also have a next property. As a result next is not included in nonRequestOptions and therefore not passed to fetch.

You can prove this with something like this:

const customFetch: typeof fetch = async (request, init) => {
    console.log('init is ', init);
    return fetch(request, init);
};
ky.post('https://example.com', { next: { revalidate: 1000 }, proxy: 'https://someproxy.com' });

if I run this in edge runtime I get

init is { proxy: 'https://someproxy.com' }

whereas in node I get

init is { next: { revalidate: 1000 }, proxy: 'https://someproxy.com' }

So the problem is very specific to the next option. And I dont think it will be fixed by the Next.js issue referenced in your comment above.

One possible solution could be if we could only check the "declared" keys of the original Request type when computing nonRequestOptions. otherwise I guess they would have to be hardcoded.

Let me know if any of the above doesn't make sense.

[kdelmonte]

Thank you for the clarification. I will investigate.

[dkokotov]

@kdelmonte (or @sindresorhus ) not sure if you've had a chance to look yet, but please let me know what you think of the approach in #542

[kdelmonte]

hey @dkokotov, I just took a look at your PR and I think it does make sense that we would not need the !(key in request) check after the work that was done as part of #540. I see that you do have some failing tests though.

[dkokotov]

To be honest I had some weird behaviors with the tests. When they ran on my fork they initially hung but then passed (https://github.com/dkokotov/ky/actions). so not sure why they are failing in the context of the PR. And I had trouble making them run locally.

As I mentioned in my comment on the PR (#542 (comment)) I ended up switching to a different library just cause I needed to make progress on what I had been trying to use this for. so I probably won't have time to chase down the test issues. Feel free to either take the issue / PR over or to close it.