-
Notifications
You must be signed in to change notification settings - Fork 70
feat: add an experimental KV based tag cache #906
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
Conversation
🦋 Changeset detectedLatest commit: b27784b 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 |
commit: |
packages/cloudflare/src/api/overrides/tag-cache/kv-next-tag-cache.ts
Outdated
Show resolved
Hide resolved
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.
Just some small little change needed and a few thing maybe to add as comment
packages/cloudflare/src/api/overrides/tag-cache/kv-next-tag-cache.ts
Outdated
Show resolved
Hide resolved
packages/cloudflare/src/api/overrides/tag-cache/kv-next-tag-cache.ts
Outdated
Show resolved
Hide resolved
await kv.put(this.getCacheKey(tag), timeMs); | ||
}) | ||
); | ||
await purgeCacheByTags(tags); |
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.
This is already done in aws, we should not do it again here
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.
So I guess it should be removed from D1 and DO too?
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.
Not sure how/where this is done in aws though?
Can you point to the code?
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.
Nevermind I was wrong, but we should do it there https://github.com/opennextjs/opennextjs-aws/blob/23ed1df6b887526dc8b573c0ea099b4009b410ec/packages/open-next/src/adapters/cache.ts#L331-L333
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.
Before checking the length, right?
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.
Oh I see what you mean, we should implement a new method in the CDN invalidation.
So I guess this code is good to merge now (I'll add a TODO here and for D1 & DO).
(A follow-up I had was to only call purgeCacheByTags
when there is a CDN invalidator so that we do not log errors when not).
I could create an issue in aws to add the method and reference it in this repo.
Do that sound good?
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.
Yeah perfect
…che.ts Co-authored-by: conico974 <[email protected]>
Co-authored-by: conico974 <[email protected]>
Thanks for the review and feedback Nico! |
No description provided.