-
Notifications
You must be signed in to change notification settings - Fork 92
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
base: main
Are you sure you want to change the base?
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
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 |
const timestamps = timestampsOrNulls.filter((timestamp) => timestamp !== null) | ||
if (timestamps.length === 0) { | ||
const expirationTimestamps = timestampsOrNulls | ||
.filter((timestamp) => timestamp !== null) |
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 here
.filter((timestamp) => timestamp !== null) | |
.filter((manifest) => manifest !== null) |
return Math.max(...expirationTimestamps) | ||
} | ||
|
||
export type TagStaleOrExpired = |
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.
nit: this sounds like it wouldn't include fresh at first glance. maybe something like:
export type TagStaleOrExpired = | |
export type TagStaleOrExpiredStatus = |
or...
export type TagStaleOrExpired = | |
export type TagFreshness = |
🤔
/** | ||
* Timestamp when tagged cache entry should no longer serve stale content. | ||
*/ | ||
expiredAt: 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.
really nitpicky, but my brain interprets this past tense as meaning this would never be set to a future timestamp in advance
expiredAt: number | |
expireAt: number |
or
expiredAt: number | |
expiresAt: number |
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 }, |
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 }, |
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' }) |
Description
To support https://nextjs.org/blog/next-16-beta#improved-caching-apis
Note that
updateTag
is supported without any changes (I just added test cases for it). The runtime changes are to supportrevaligateTag
with SWR behaviorDocumentation
Tests
Will be added
Relevant links (GitHub issues, etc.) or a picture of cute animal