-
Notifications
You must be signed in to change notification settings - Fork 91
feat: support revalidateTag with SWR behavior #3173
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: support revalidateTag with SWR behavior #3173
Conversation
📊 Package size report 0.06%↑
Unchanged files
🤖 This report was automatically generated by pkg-size-action |
5be6bef to
4da8739
Compare
80e143c to
e69ecd6
Compare
e69ecd6 to
e0a4e48
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, just some nitpicks and minor suggestions
src/run/handlers/tags-handler.cts
Outdated
| const timestampsOrNulls = await Promise.all(tags.map((tag) => getTagManifest(tag, cacheStore))) | ||
|
|
||
| const timestamps = timestampsOrNulls.filter((timestamp) => timestamp !== null) | ||
| if (timestamps.length === 0) { | ||
| const expirationTimestamps = timestampsOrNulls |
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.
left over from refactor:
| const timestampsOrNulls = await Promise.all(tags.map((tag) => getTagManifest(tag, cacheStore))) | |
| const timestamps = timestampsOrNulls.filter((timestamp) => timestamp !== null) | |
| if (timestamps.length === 0) { | |
| const expirationTimestamps = timestampsOrNulls | |
| const manifestsOrNulls = await Promise.all(tags.map((tag) => getTagManifest(tag, cacheStore))) | |
| const expirationTimestamps = manifestsOrNulls |
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.
github didn't allow me to commit suggestion so this was done as part of 37c3e8e#diff-7fe4e66d51e744fc6fd6321c633def547066010bb05aff0ef621b64d84ec5442R35
src/run/handlers/tags-handler.cts
Outdated
| getLogger().withFields({ tags }).debug('doRevalidateTagAndPurgeEdgeCache') | ||
| async function doRevalidateTagAndPurgeEdgeCache( | ||
| tags: string[], | ||
| durations?: { expire?: number }, |
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.
suggestion:
| durations?: { expire?: number }, | |
| durations?: { expireSeconds?: number }, |
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.
the durations type is from Next.js - https://github.com/vercel/next.js/blob/fffa2831b61fa74852736eeaad2f17fbdd553bce/packages/next/src/server/lib/incremental-cache/index.ts#L78
I do not need to use the exact type here, but I do need to use original type at least in our CacheHandler and would need to add conversion from Next.js type to our type which doesn't seem worth it?
But for clarity I will add some jsdocs or code comments along the call chain to clarify that expire is in seconds
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.
added type with some descriptions 8f0a2d4
src/run/handlers/tags-handler.cts
Outdated
| export function markTagsAsStaleAndPurgeEdgeCache(tagOrTags: string | string[]) { | ||
| export function markTagsAsStaleAndPurgeEdgeCache( | ||
| tagOrTags: string | string[], | ||
| durations?: { expire?: number }, |
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.
suggestion:
| durations?: { expire?: number }, | |
| durations?: { expireSeconds?: number }, |
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.
same as #3173 (comment)
| if (nextVersionSatisfies('>=16.0.0-alpha.0')) { | ||
| test.describe('app router on-demand revalidation (Next 16 APIs)', () => { |
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.
since these tests will be relatively slow with all the waiting and they should be independent, maybe we could parallelize them?
| if (nextVersionSatisfies('>=16.0.0-alpha.0')) { | |
| test.describe('app router on-demand revalidation (Next 16 APIs)', () => { | |
| test.describe('app router on-demand revalidation (Next 16 APIs)', () => { | |
| test.describe.configure({ mode: 'parallel' }) |
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.
github didn't allow me to commit suggestion so I did it in e13e932
Co-authored-by: Philippe Serhal <[email protected]>
|
a4b1046...feat/support-update-tag-and-revalidate-tag-second-arg for the changes since review (including applied suggestions) |
Description
To support https://nextjs.org/blog/next-16-beta#improved-caching-apis
Note that
updateTagis supported without any changes (I just added test cases for it). The runtime changes are to supportrevaligateTagwith SWR behaviorDocumentation
Tests
Will be added
Relevant links (GitHub issues, etc.) or a picture of cute animal