-
Notifications
You must be signed in to change notification settings - Fork 130
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 next/cache
#297
Feature next/cache
#297
Conversation
🦋 Changeset detectedLatest commit: 54cce0c The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
🧪 A prerelease is available for testing 🧪 You can install this latest build in your project with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/next-on-pages/runs/5200481948/npm-package-next-on-pages-297 Or you can immediately run this with npx https://prerelease-registry.devprod.cloudflare.dev/next-on-pages/runs/5200481948/npm-package-next-on-pages-297 |
@bluebeel Thank you so so very much for the PR 🙏 Just wanted to sorry for not having reviewed it yet, and sorry I think I won't be able to this week. There are a few things I want to test properly and consider carefully, I'll get to this as soon as I am able 🙇 |
No problem at all 😁 And looking on how exactly the caching work on segment level... |
Hi @bluebeel, first of all thank you so very much for the awesome PR! You're implementation is super interesting and your explanations are super informative and helpful! thanks a lot! 🙏 😃 ❤️ 🚀 That being said... I can see some issues with this approach, most we can discuss later, but there is one which is particularly problematic and that it'd like to discuss right away as it feels to me like a very big one, so it'd be valuable to see your point of view and see if there is a path forward we could take. In your implementation the cache urls aren't protected so anyone could update the cache (via For example I have slightly simplified your geolocation app to just display the force-cache time, I've deployed it and then with standard In order to do that I needed to find out the item's cacheKey, I did that via logs so it was much easier in my case, in real scenarios it would be much less straightforward but still possible I believe (especially for open source projects, there people could use this function to get the correct cacheKey for known requests, right?). The above seems a too big of a security issue. For example, if an open source project were to publish a next-on-pages application with your changes, I am pretty sure I could find ways of injecting into their cache what I wanted, potentially causing big issues. What do you think of the above? Am I correct in my reasoning? Do you think there is a different approach we could take so that the cache is protected and not writable by external requests? |
Hi @dario-piotrowicz , Your reasoning is correct. To prevent that on our side, I was thinking maybe we can request the user to add a |
@bluebeel yeah this is one thing I was thinking about, I wonder if we can find a way to do that without users having to do anything... Also one thing that comes to mind, do these |
Good question, I never tried the How would you do it programmatically? 🤔 Writing directly inside the cache from the |
Yes, I think we could patch the global fetch (or for now separately as a nextFetch proof of concept function) so that we check, if the request is for the cache we get the value or set the value directly from it programmatically and return a constructed response, otherwise we run the request as usual. Does that seam feasible? 🙂 |
In addition to the security issue mentioned above (which is why Vercel uses an auth token from the environment variables for requests), there's another aspect that I feel is somewhat problematic with the current proposal. I'm concerned that introducing a dependency on the patched fetch in its current form will end up causing compatibility issues between different versions of Next.js 🙁. If we use the patched fetch from the latest version as the base point for our patched fetch, there may be problems with using it in older versions of Next.js. Likewise, it might have problems with future versions of Next.js. It feels like it would introduce the burden of constantly trying to ensure it works with as many Next.js versions as possible. In addition, the patched fetch that we use would need to be identical in how it functions and behaves to the Next.js one to prevent other issues, meaning it would essentially need to be copy pasted and tweaked with the relevant aspects. Now, there is another avenue that might be worth exploring. We could try using the This approach would allow using the What do you think of trying out this approach?
They should only ever come from the server. The That actually brings up another interesting point. I wonder if misusing a utility function for our patched fetch, instead of patching By the way @bluebeel, did you try patching
Routes that generate Prerender functions (which
Could you possibly elaborate on what you meant by this? Are you referring to revalidating a cached page? If so, that is not possible on Cloudflare Pages due to our inability to deploy and invoke the Node.js Prerender functions that it depends on. |
Last time I checked, they don't use it to secure the endpoint. At first I thought too they would inject it in Vercel deployment but the variable return undefined.
I'll try this weekend that approach.
So Cloudflare Pages will never support that? 😢 Can we not rewrite the Prerender function to make it possible? |
Nope. Prerender functions depend on the Node.js format and cannot be built to target the Edge runtime, nor would it be at all practical or logical to deploy them to Cloudflare given how they work - each one would need its own separate cron worker. This makes them wildly different to what we can work with, and it would not be possible to convert them in such a way to be used properly. |
Hey guys, really excited to see this transpiring! Outside of the patched fetch function, curious what benefits you see of replicating their internal API versus using a custom cache handler and writing an interface for KV, DO or potentially even R2? |
Hey there @mikebuilds, I'm afraid it is not really possible for us to use the Lee from Vercel touched on this here, explaining that they don't yet have a spec to deal with it in the build output. Hopefully, in the future, we will be able to take advantage of that option, but sadly not at the moment. |
hey all! I'm following the discussion here. Many thanks to everyone for all the details. One thing that isn't particularly clear to me from the discussion above is:
If we can make (2) work in any way possible (as long as it can be maintainable/not necessarily a huge burden/etc), I don't see anything against deciding not to support (1) at all. Ultimately, everything on the ISR front has been building towards what is now possible using tags. It also provides a migration path for anyone interested (as opposed to no path forward). |
Hey there @juanferreras, you are correct with point 1 - ISR will never be possible due to not being able to deploy Prerender functions. Regarding point 2, Your assumption is correct - finding a reliable way to patch |
Hello, First of all sorry for the long gap for the pull request. My June and July were quite busy personally. I updated what you asked @james-elicx and @dario-piotrowicz. NOTES:
When I add
|
Hello there, I actually started working on our own implementation of supporting the fetch cache this week because you completely disappeared for the past 1.5 months without saying anything. It's generally nice to say something if one is going to be MIA for prolonged periods of time... Anyway, after further investigation this week and upon reflection on previous comments, there are other approaches that I feel are much more preferential for dealing with caching, which go in a different direction from how this PR currently does. The notable differences in our implementation are as follows:
Rather, the approach that I have been working on this week is instead intercepting fetch calls to the API from the default fetch cache and handling them through that instead. More as if we were a suspense cache API endpoint, but without exposing it to the web and addressing previous concerns I shared here. As to where that leaves things with this PR, well, I'm not sure. That's also not really up to me. It would have been nice to have heard something sooner so that efforts could be focused on this PR instead... Regardless of what happens, I intend to open my own PR this week anyway. |
Hi there, My apologies for my unannounced break, personal matters forced me to temporarily put coding on hold. Regarding the cache implementation, my PR was originally intended as a starting point for discussion, not the go-to solution. I fully support the new direction you're taking with caching. I'll wait for your pr and give any opinion I may have on it. I just want a nice next cache support to land at some point in the package. Sorry again for not communicating sooner, looking forward to contributing again..m The pr can be closed I think then |
Thanks a lot @bluebeel it's so great to hear that you're on board with exploring James' solution and yeah thanks so much for starting the conversation with your PR, it really helped get the ball rolling 🙂 I think we can keep it open until we actually find a solution we're all happy with 😄
|
Hi, My interest is still big. I really want to have a platform that could compete with Vercel about NextJS as much as possible. If I can help in anyway for it I'll do it 🫡 |
Closed in favor of #419 |
Description
This pull request introduces enhancements to our Cloudflare Worker, enabling it to store generated content within the Cache API for quick retrieval and handle cache invalidation of data cached by
nextFetch
Next/Cache - How does it work
Depending on the runtime environment,
next/cache
can leverage two distinct types of cache systems -FileSystemCache
andFetchCache
.The
FileSystemCache
is used when the runtime environment is either a local system or a Node.js runtime. This caching mechanism stores the cached data directly on the disk. When deployed on the Vercel platform, this essentially translates to the use of an Elastic File System (EFS) connected to the Lambda function. On local systems, it utilizes the local filesystem for storage.On the other hand, the
FetchCache
comes into play when the application is running on an Edge runtime, which involves the execution of Edge functions on Cloudflare Workers. Due to the nature of this environment, it does not provide access to the same disk that is used when the application is running on Vercel. To circumnavigate this, Vercel makes use of an API that is directly connected to an EFS within their infrastructure. This API is pinged to read the cache content and return it. Additionally, to add new elements to the cache from the Edge, the same API can be used to write to the EFS within the Vercel infrastructure. This method, using FetchCache and the API, offers a shared cache resource between the Lambda functions running in Vercel and the Edge functions running on Cloudflare Workers.The
FetchCache
is specifically designed for edge runtime environments, as Vercel operates edge functions using Cloudflare workers. There's no need for a complete overhaul—by simply emulating Vercel's Cache API,next/cache
will work seamlessly on Cloudflare! 🔥Next/Cache - Mimicking Vercel's Private API
Vercel introduces two headers to enable
FetchCache
:x-vercel-sc-host
andx-vercel-sc-basepath
.x-vercel-sc-host
: This header holds the API URL for the request. Vercel, for instance, usessuspense-cache.vercel.com
for edge runtime andiad1.suspense-cache.vercel-infra.com
for a Node.js runtime (region-dependent).x-vercel-sc-basepath
: This additional header can provide an optional path for the request. While Vercel currently doesn't use it, it's potentially beneficial for platforms supporting fetch cache.In our implementation, we don't utilize a global API for all Cloudflare users. Instead, each customer and project will have its unique internal cache API endpoint.
We set the following values in the headers:
x-vercel-sc-host
: This is the hostname of the deployed pages project. As Next.js expects a HTTPS host, it won't work locally without the HTTPS flag in wrangler or ngrok.x-vercel-sc-basepath
: The path**/__next/cache**
ensures all requests are sent to<host>/__next/cache/xxxx
.In the worker, we intercept all requests with a pathname starting with
/__next/cache
and return the appropriate response based on whether we're setting or getting an element from the cache.Example of cache payload
Features supported
1 - patched-fetch of nextjs: Next.js patches the global fetch to incorporate next/cache features. The global fetch of the worker runtime encounters an error when we use the cache property or next property inside. I created a nextFetch function that is a copy-paste of the patched-fetch but as a wrapper around the standard fetch. I don't know why they didn't do something like that at the first place 🙃.
I didn't add it inside the repo because I didn't know where to put it. Here is the file and usage in my example nextjs app
2 - revalidateTag/revalidatePath: The implementation is there. It takes the
staticGenerationAsyncStorage
from the patched-fetch. From there it access theincrementalCache
. In mynextFetch
implementation, I noticed that incrementalCache was always undefined when running via pages and correctly set when running next dev. My workaround was to link directly the globalThis reference to the incrementalCache.Notes and future improvement
I've marked this as a draft for further discussions on improvements and testing (I could use help in this area as well 😭).
Best