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

[⚡ Feature]: Add KV (or custom) support for revalidateTag (next/cache) #529

Open
juanferreras opened this issue Nov 8, 2023 · 4 comments · May be fixed by #606
Open

[⚡ Feature]: Add KV (or custom) support for revalidateTag (next/cache) #529

juanferreras opened this issue Nov 8, 2023 · 4 comments · May be fixed by #606
Assignees
Labels
enhancement New feature or request

Comments

@juanferreras
Copy link
Contributor

Hi all!

I've seen several comments from maintainers ([1], [2]) following up the great work of feat: fetch (suspense) cache handling, and next/cache support #419 by @james-elicx that given limitations on Cache API (e.g. per-colo cache and requires a custom domain to be configured), we'd mostly want to use alternative storage mechanisms in production (most likely KV, but perhaps other users might prefer D1/R2/DO based on specific use cases or even something else).

I understand that the ultimate intention is to let users define in their own next.config.js their own adaptor for caching – but there might be a pre-made snippet for KV, or an exported util they can reference.

I've noted that there isn't any issue tracking that – so wanted to drop this in for that purpose, and in case maintainers want to explain more what they have in mind (or if there are any blockers they're tracking). I'm happy to take a look at contributing the changes too.

Thanks!

@james-elicx
Copy link
Contributor

I actually originally wanted to let people opt in to other storage mechanisms by the presence of the binding being detected, e.g. process.env.KV_SUSPENCE_CACHE would use the KV cache, but I think others weren't a fan of doing that which is why it was scrapped from the initial PR.

The general idea that was settled upon would be to require people use a separate cache interface using the incrementalCacheHandlerPath option in next.config.js. The Next.js docs recommend the following usage: incrementalCacheHandlerPath: require.resolve('./cache-handler.js'). The problem with this is that you then get the import for the CJS version when trying to read the path from next.config.js, instead of the ESM one, because Next.js want you to use require for it, and that becomes a real pain since Workers want ESM.

Personally, I would actually quite like to add the binding-presence based approach for things like KV/R2/D1, and can quite easily revive that, but I'm not sure whether @dario-piotrowicz would be okay with that approach.

I would also note that Vercel's data cache is regional, so using the cache api is a rather similar experience.

@dario-piotrowicz dario-piotrowicz added the enhancement New feature or request label Nov 10, 2023
@ibobo
Copy link
Contributor

ibobo commented Nov 14, 2023

Hi everyone!

I just released a Next.js based website in production on Pages and had a lot of headaches because the data cache is regional. In short terms, news being added to the website didn't show up at all in the homepage because the origin server was triggering revalidateTags and revalidatePaths in just one region. Having a distributed cache would solve this.

You're right @james-elicx, on Vercel the data cache is regional as it is on Cloudflare Pages, but there's an important difference (from the docs, my emphasis):

On-demand revalidation: Any data can be triggered for revalidation on-demand, regardless of the revalidation interval. The revalidation propagates to all regions within 300ms.

So, to better replicate the Vercel model, there should be at least a way to revalidate the data cache globally. Using KV is the obvious way to do this on Cloudflare. In the future a hybrid approach (use Cache API primarily with a short expiration and reading through to KV for global consistency) could be beneficial for really high scale apps.

I understand the hesitations about using an env or a binding for the configuration, but I think that allowing the use of the KV cache is of great value and that justifies some inconveniences.

Also, about the require.resolve, as I understand it, it just returns the full path to a file when used like require.resolve('./filename.js'), so could be replaced by a simple __dirname + '/filename.js'?

@james-elicx
Copy link
Contributor

Also, about the require.resolve, as I understand it, it just returns the full path to a file when used like require.resolve('./filename.js'), so could be replaced by a simple __dirname + '/filename.js'?

The main part of the problem is that the path provided for incrementalCacheHandlerPath is imported by Next.js using require, so it needs to be CJS or Next.js dev mode/building won't work, but we need it to be ESM, and in my view it's really not ideal to go down the path of trying to do unsafe transforms from CJS to ESM at build time using custom esbuild plugins.

@dario-piotrowicz
Copy link
Member

dario-piotrowicz commented Nov 15, 2023

Hi @juanferreras, sorry for the late reply 🙂

I think we agreed with @james-elicx that we can try to go with an approach in which the cache handler can be configurable via the standard incrementalCacheHandlerPath (my preference) and also based on binding naming convention (James' preference) just so to allow people to also use those if they do not want to rely on configs for this.

There's the pain point of getting cjs and needing to deal with it, but I hope that we can try to sort that out (by trying for instance to convert cjs to esm using something like this for example: https://github.com/wessberg/cjstoesm).

I'll leave this to James as he's been driving the chance implementation since the beginning and has already started looking at things back when, but I am happy to help or give it a shot myself if James doesn't have the time to tackle this 🙂

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

Successfully merging a pull request may close this issue.

4 participants