Skip to content
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

fix: dynamic not-prerendered routes revalidate tracking #2771

Merged
merged 4 commits into from
Mar 7, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 49 additions & 27 deletions src/run/handlers/cache.cts
Original file line number Diff line number Diff line change
Expand Up @@ -183,37 +183,56 @@ export class NetlifyCacheHandler implements CacheHandlerForMultipleVersions {

private async injectEntryToPrerenderManifest(
key: string,
revalidate: NetlifyCachedPageValue['revalidate'],
{ revalidate, cacheControl }: Pick<NetlifyCachedPageValue, 'revalidate' | 'cacheControl'>,
) {
if (this.options.serverDistDir && (typeof revalidate === 'number' || revalidate === false)) {
if (
this.options.serverDistDir &&
(typeof revalidate === 'number' ||
revalidate === false ||
typeof cacheControl !== 'undefined')
) {
try {
const { loadManifest } = await import('next/dist/server/load-manifest.js')
const prerenderManifest = loadManifest(
join(this.options.serverDistDir, '..', 'prerender-manifest.json'),
) as PrerenderManifest

try {
const { normalizePagePath } = await import(
'next/dist/shared/lib/page-path/normalize-page-path.js'
if (typeof cacheControl !== 'undefined') {
// instead of `revalidate` property, we might get `cacheControls` ( https://github.com/vercel/next.js/pull/76207 )
// then we need to keep track of revalidate values via SharedCacheControls
const { SharedCacheControls } = await import(
// @ts-expect-error supporting multiple next version, this module is not resolvable with currently used dev dependency
// eslint-disable-next-line import/no-unresolved, n/no-missing-import
'next/dist/server/lib/incremental-cache/shared-cache-controls.js'
)

prerenderManifest.routes[key] = {
experimentalPPR: undefined,
dataRoute: posixJoin('/_next/data', `${normalizePagePath(key)}.json`),
srcRoute: null, // FIXME: provide actual source route, however, when dynamically appending it doesn't really matter
initialRevalidateSeconds: revalidate,
// Pages routes do not have a prefetch data route.
prefetchDataRoute: undefined,
const sharedCacheControls = new SharedCacheControls(prerenderManifest)
sharedCacheControls.set(key, cacheControl)
} else if (typeof revalidate === 'number' || revalidate === false) {
// if we don't get cacheControls, but we still get revalidate, it should mean we are before
// https://github.com/vercel/next.js/pull/76207
try {
const { normalizePagePath } = await import(
'next/dist/shared/lib/page-path/normalize-page-path.js'
)

prerenderManifest.routes[key] = {
experimentalPPR: undefined,
dataRoute: posixJoin('/_next/data', `${normalizePagePath(key)}.json`),
srcRoute: null, // FIXME: provide actual source route, however, when dynamically appending it doesn't really matter
initialRevalidateSeconds: revalidate,
// Pages routes do not have a prefetch data route.
prefetchDataRoute: undefined,
}
} catch {
// depending on Next.js version - prerender manifest might not be mutable
// https://github.com/vercel/next.js/pull/64313
// if it's not mutable we will try to use SharedRevalidateTimings ( https://github.com/vercel/next.js/pull/64370) instead
const { SharedRevalidateTimings } = await import(
'next/dist/server/lib/incremental-cache/shared-revalidate-timings.js'
)
const sharedRevalidateTimings = new SharedRevalidateTimings(prerenderManifest)
sharedRevalidateTimings.set(key, revalidate)
}
} catch {
// depending on Next.js version - prerender manifest might not be mutable
// https://github.com/vercel/next.js/pull/64313
// if it's not mutable we will try to use SharedRevalidateTimings ( https://github.com/vercel/next.js/pull/64370) instead
const { SharedRevalidateTimings } = await import(
'next/dist/server/lib/incremental-cache/shared-revalidate-timings.js'
)
const sharedRevalidateTimings = new SharedRevalidateTimings(prerenderManifest)
sharedRevalidateTimings.set(key, revalidate)
}
} catch {}
}
Expand Down Expand Up @@ -315,7 +334,7 @@ export class NetlifyCacheHandler implements CacheHandlerForMultipleVersions {

span.addEvent(blob.value?.kind, { lastModified: blob.lastModified, revalidate, ttl })

await this.injectEntryToPrerenderManifest(key, revalidate)
await this.injectEntryToPrerenderManifest(key, blob.value)

return {
lastModified: blob.lastModified,
Expand All @@ -327,7 +346,7 @@ export class NetlifyCacheHandler implements CacheHandlerForMultipleVersions {

span.addEvent(blob.value?.kind, { lastModified: blob.lastModified, revalidate, ttl })

await this.injectEntryToPrerenderManifest(key, revalidate)
await this.injectEntryToPrerenderManifest(key, blob.value)

return {
lastModified: blob.lastModified,
Expand Down Expand Up @@ -355,22 +374,25 @@ export class NetlifyCacheHandler implements CacheHandlerForMultipleVersions {
if (isCachedRouteValue(data)) {
return {
...data,
revalidate: context.revalidate,
revalidate: context.revalidate ?? context.cacheControl?.revalidate,
cacheControl: context.cacheControl,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm assuming we don't have to do this at build time too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still record initialRevalidateSeconds from prerender-manifest.json under revalidate

revalidate: initialRevalidateSeconds,

Which is enough for how we read this value later for own needs (like figuring out initial cdn max age in some cases - like recently merged 404 caching fixes or preventing double SWR ) and Next.js also uses prerender-manifest.json directly for anything that was prerendered ( https://github.com/vercel/next.js/blob/7e5e69dd2592e2b8d165d15b267ba3491f7d8174/packages/next/src/server/lib/incremental-cache/shared-cache-controls.ts#L36-L67 ), so we don't have to actually do the same at build time - otherwise we would also need to adjust our own code in places we rely on revalidate field.

That is possibly something to think about, because cacheControl field does contain expire together revalidate, so this would allow exact stale-while-revalidate= value for cases that use

function setCacheControlFromRequestContext(
headers: Headers,
revalidate: NetlifyCachedRouteValue['revalidate'],
) {
const cdnCacheControl =
// if we are serving already stale response, instruct edge to not attempt to cache that response
headers.get('x-nextjs-cache') === 'STALE'
? 'public, max-age=0, must-revalidate, durable'
: `s-maxage=${revalidate || 31536000}, stale-while-revalidate=31536000, durable`
headers.set('netlify-cdn-cache-control', cdnCacheControl)
}

So technically it might not be correct not to store it, but it probably be better for some follow up as amount it's already here is addressing substantial problem (with those next versions we never cache not-prerendered dynamic pages on cdn now)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I'll go ahead and merge this

body: data.body.toString('base64'),
}
}

if (isCachedPageValue(data)) {
return {
...data,
revalidate: context.revalidate,
revalidate: context.revalidate ?? context.cacheControl?.revalidate,
cacheControl: context.cacheControl,
}
}

if (data?.kind === 'APP_PAGE') {
return {
...data,
revalidate: context.revalidate,
revalidate: context.revalidate ?? context.cacheControl?.revalidate,
cacheControl: context.cacheControl,
rscData: data.rscData?.toString('base64'),
}
}
Expand Down
24 changes: 23 additions & 1 deletion src/shared/cache-types.cts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,11 @@ import type {

export type { CacheHandlerContext } from 'next/dist/server/lib/incremental-cache/index.js'

type CacheControl = {
revalidate: Parameters<CacheHandler['set']>[2]['revalidate']
expire: number | undefined
}

/**
* Shape of the cache value that is returned from CacheHandler.get or passed to CacheHandler.set
*/
Expand All @@ -28,6 +33,7 @@ export type NetlifyCachedRouteValue = Omit<CachedRouteValueForMultipleVersions,
// Next.js doesn't produce cache-control tag we use to generate cdn cache control
// so store needed values as part of cached response data
revalidate?: Parameters<CacheHandler['set']>[2]['revalidate']
cacheControl?: CacheControl
}

/**
Expand All @@ -50,6 +56,7 @@ export type NetlifyCachedAppPageValue = Omit<
// Next.js stores rscData as buffer, while we store it as base64 encoded string
rscData: string | undefined
revalidate?: Parameters<CacheHandler['set']>[2]['revalidate']
cacheControl?: CacheControl
}

/**
Expand All @@ -64,6 +71,7 @@ type IncrementalCachedPageValueForMultipleVersions = Omit<IncrementalCachedPageV
*/
export type NetlifyCachedPageValue = IncrementalCachedPageValueForMultipleVersions & {
revalidate?: Parameters<CacheHandler['set']>[2]['revalidate']
cacheControl?: CacheControl
}

export type CachedFetchValueForMultipleVersions = Omit<CachedFetchValue, 'kind'> & {
Expand Down Expand Up @@ -131,4 +139,18 @@ type MapCacheHandlerClassMethod<T> = T extends (...args: infer Args) => infer Re

type MapCacheHandlerClass<T> = { [K in keyof T]: MapCacheHandlerClassMethod<T[K]> }

export type CacheHandlerForMultipleVersions = MapCacheHandlerClass<CacheHandler>
type BaseCacheHandlerForMultipleVersions = MapCacheHandlerClass<CacheHandler>

type CacheHandlerSetContext = Parameters<CacheHandler['set']>[2]

type CacheHandlerSetContextForMultipleVersions = CacheHandlerSetContext & {
cacheControl?: CacheControl
}

export type CacheHandlerForMultipleVersions = BaseCacheHandlerForMultipleVersions & {
set: (
key: Parameters<BaseCacheHandlerForMultipleVersions['set']>[0],
value: Parameters<BaseCacheHandlerForMultipleVersions['set']>[1],
context: CacheHandlerSetContextForMultipleVersions,
) => ReturnType<BaseCacheHandlerForMultipleVersions['set']>
}
8 changes: 4 additions & 4 deletions tests/e2e/page-router.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -413,12 +413,12 @@ test.describe('Simple Page Router (no basePath, no i18n)', () => {
expect(date1.localeCompare(beforeFirstFetch)).toBeGreaterThan(0)

// allow page to get stale
await page.waitForTimeout(60_000)
await page.waitForTimeout(61_000)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah yes, I got tripped up by this too 😄


const response2 = await page.goto(new URL(pathname, pageRouter.url).href)
expect(response2?.status()).toBe(200)
expect(response2?.headers()['cache-status']).toMatch(
/"Netlify (Edge|Durable)"; hit; fwd=stale/m,
/("Netlify Edge"; hit; fwd=stale|"Netlify Durable"; hit; ttl=-[0-9]+)/m,
)
expect(response2?.headers()['netlify-cdn-cache-control']).toMatch(
/s-maxage=60, stale-while-revalidate=[0-9]+, durable/,
Expand All @@ -436,8 +436,8 @@ test.describe('Simple Page Router (no basePath, no i18n)', () => {
const response3 = await page.goto(new URL(pathname, pageRouter.url).href)
expect(response3?.status()).toBe(200)
expect(response3?.headers()['cache-status']).toMatch(
// hit, without being followed by ';fwd=stale'
/"Netlify (Edge|Durable)"; hit(?!; fwd=stale)/m,
// hit, without being followed by ';fwd=stale' for edge or negative TTL for durable, optionally with fwd=stale
/("Netlify Edge"; hit(?!; fwd=stale)|"Netlify Durable"; hit(?!; ttl=-[0-9]+))/m,
)
expect(response3?.headers()['netlify-cdn-cache-control']).toMatch(
/s-maxage=60, stale-while-revalidate=[0-9]+, durable/,
Expand Down
2 changes: 2 additions & 0 deletions tests/fixtures/wasm-src/src/app/og-node/route.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,5 @@ export async function GET() {
height: 630,
})
}

export const dynamic = 'force-dynamic'
2 changes: 2 additions & 0 deletions tests/fixtures/wasm-src/src/app/og/route.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,5 @@ export async function GET() {
}

export const runtime = 'edge'

export const dynamic = 'force-dynamic'
2 changes: 2 additions & 0 deletions tests/fixtures/wasm/app/og-node/route.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,5 @@ export async function GET() {
height: 630,
})
}

export const dynamic = 'force-dynamic'
2 changes: 2 additions & 0 deletions tests/fixtures/wasm/app/og/route.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,5 @@ export async function GET() {
}

export const runtime = 'edge'

export const dynamic = 'force-dynamic'
Loading