-
Notifications
You must be signed in to change notification settings - Fork 37
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
feat: d1 adapter for the tag cache #320
Conversation
🦋 Changeset detectedLatest commit: f5d8133 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
01d1c68
to
d0d0738
Compare
commit: |
d0d0738
to
1cce4af
Compare
packages/cloudflare/src/cli/build/utils/extract-cache-assets-manifest.ts
Outdated
Show resolved
Hide resolved
401843a
to
69f2d20
Compare
229b2e2
to
fa3b420
Compare
69f2d20
to
3566d04
Compare
3566d04
to
0c28284
Compare
packages/cloudflare/src/cli/build/utils/extract-cache-assets-manifest.ts
Outdated
Show resolved
Hide resolved
79bab94
to
54a89cb
Compare
424670b
to
4b152b9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, nice work!!
To be perfectly honest, i'm still not 100% convinced that D1 is the perfect fit for this (at least not alone). The tag cache can be used in SSR for the fetch cache and i'm not sure if D1 can handle heavy workload for these cases.
It's probably fine in most cases but i still wanted to point this out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a couple comments.
IIUC we should first get opennextjs/opennextjs-aws#717 merged and then this PR?
Even if using D1 is not ideal, we can keep iterating before 1.0 but it would be nice to unblock people as early as we can.
WDYT @conico974 @james-elicx ?
Not necessarily, but it would avoid people getting a bunch of errors if they use this with page router only. And it avoids checking the cache tag for page router.
I agree, and it will allow to collect some feedback |
What's missing for this PR to get merged? Do you have an approximate ETA in mind? |
@vicb it should be ready actually, i just requested a review |
I thought we had agreed that we would add both modes with users having the option of deciding which they want to use, so there shouldn't be any dependency on the new mode for this PR. Unless we want to add the other mode in this PR as well and release both modes at the same time? |
I was referring to "Not necessarily, but it would avoid people getting a bunch of errors if they use this with page router only. And it avoids checking the cache tag for page router." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for creating this PR and all of the updates!μ
LGTM, a couple minor comments.
🚀
Co-authored-by: Victor Berchet <[email protected]>
Docs PR - opennextjs/docs#66