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: apply caching headers to pages router 404 with getStaticProps #2764

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
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
4 changes: 4 additions & 0 deletions src/build/content/prerendered.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ const routeToFilePath = (path: string) => {

const buildPagesCacheValue = async (
path: string,
initialRevalidateSeconds: number | false | undefined,
shouldUseEnumKind: boolean,
shouldSkipJson = false,
): Promise<NetlifyCachedPageValue> => ({
Expand All @@ -65,6 +66,7 @@ const buildPagesCacheValue = async (
pageData: shouldSkipJson ? {} : JSON.parse(await readFile(`${path}.json`, 'utf-8')),
headers: undefined,
status: undefined,
revalidate: initialRevalidateSeconds,
})

const buildAppCacheValue = async (
Expand Down Expand Up @@ -178,6 +180,7 @@ export const copyPrerenderedContent = async (ctx: PluginContext): Promise<void>
}
value = await buildPagesCacheValue(
join(ctx.publishDir, 'server/pages', key),
meta.initialRevalidateSeconds,
shouldUseEnumKind,
)
break
Expand Down Expand Up @@ -210,6 +213,7 @@ export const copyPrerenderedContent = async (ctx: PluginContext): Promise<void>
const key = routeToFilePath(route)
const value = await buildPagesCacheValue(
join(ctx.publishDir, 'server/pages', key),
undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you're right that this is the safest approach, seeing as we're not seeing any specific problems with the fallbacks at present.

shouldUseEnumKind,
true, // there is no corresponding json file for fallback, so we are skipping it for this entry
)
Expand Down
10 changes: 10 additions & 0 deletions src/run/handlers/cache.cts
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,11 @@ export class NetlifyCacheHandler implements CacheHandlerForMultipleVersions {

const { revalidate, ...restOfPageValue } = blob.value

const requestContext = getRequestContext()
if (requestContext) {
requestContext.pageHandlerRevalidate = revalidate
}

await this.injectEntryToPrerenderManifest(key, revalidate)

return {
Expand All @@ -272,6 +277,11 @@ export class NetlifyCacheHandler implements CacheHandlerForMultipleVersions {

const { revalidate, rscData, ...restOfPageValue } = blob.value

const requestContext = getRequestContext()
if (requestContext) {
requestContext.pageHandlerRevalidate = revalidate
}

Comment on lines +280 to +284
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure if we should do this for app router - maybe for this PR if we treat it as hot fix - remove this

await this.injectEntryToPrerenderManifest(key, revalidate)

return {
Expand Down
1 change: 1 addition & 0 deletions src/run/handlers/request-context.cts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ export type RequestContext = {
didPagesRouterOnDemandRevalidate?: boolean
serverTiming?: string
routeHandlerRevalidate?: NetlifyCachedRouteValue['revalidate']
pageHandlerRevalidate?: NetlifyCachedRouteValue['revalidate']
/**
* Track promise running in the background and need to be waited for.
* Uses `context.waitUntil` if available, otherwise stores promises to
Expand Down
46 changes: 34 additions & 12 deletions src/run/headers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import type { Span } from '@opentelemetry/api'
import type { NextConfigComplete } from 'next/dist/server/config-shared.js'

import { encodeBlobKey } from '../shared/blobkey.js'
import type { NetlifyCachedRouteValue } from '../shared/cache-types.cjs'

import { getLogger, RequestContext } from './handlers/request-context.cjs'
import type { RuntimeTracer } from './handlers/tracer.cjs'
Expand Down Expand Up @@ -208,6 +209,19 @@ export const adjustDateHeader = async ({
headers.set('date', lastModifiedDate.toUTCString())
}

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 === false ? 31536000 : revalidate}, stale-while-revalidate=31536000, durable`

headers.set('netlify-cdn-cache-control', cdnCacheControl)
}

/**
* Ensure stale-while-revalidate and s-maxage don't leak to the client, but
* assume the user knows what they are doing if CDN cache controls are set
Expand All @@ -225,13 +239,7 @@ export const setCacheControlHeaders = (
!headers.has('netlify-cdn-cache-control')
) {
// handle CDN Cache Control on Route Handler responses
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=${requestContext.routeHandlerRevalidate === false ? 31536000 : requestContext.routeHandlerRevalidate}, stale-while-revalidate=31536000, durable`

headers.set('netlify-cdn-cache-control', cdnCacheControl)
setCacheControlFromRequestContext(headers, requestContext.routeHandlerRevalidate)
return
}

Expand All @@ -242,11 +250,25 @@ export const setCacheControlHeaders = (
.log('NetlifyHeadersHandler.trailingSlashRedirect')
}

if (status === 404 && request.url.endsWith('.php')) {
// temporary CDN Cache Control handling for bot probes on PHP files
// https://linear.app/netlify/issue/FRB-1344/prevent-excessive-ssr-invocations-due-to-404-routes
headers.set('cache-control', 'public, max-age=0, must-revalidate')
headers.set('netlify-cdn-cache-control', `max-age=31536000, durable`)
if (status === 404) {
if (request.url.endsWith('.php')) {
// temporary CDN Cache Control handling for bot probes on PHP files
// https://linear.app/netlify/issue/FRB-1344/prevent-excessive-ssr-invocations-due-to-404-routes
headers.set('cache-control', 'public, max-age=0, must-revalidate')
headers.set('netlify-cdn-cache-control', `max-age=31536000, durable`)
return
}

if (
typeof requestContext.pageHandlerRevalidate !== 'undefined' &&
['GET', 'HEAD'].includes(request.method) &&
!headers.has('cdn-cache-control') &&
!headers.has('netlify-cdn-cache-control')
Comment on lines +263 to +266
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably should add check for cache-control being ~no-cache and/or no-store to let us use known working case this is after this

) {
// handle CDN Cache Control on 404 Page responses
setCacheControlFromRequestContext(headers, requestContext.pageHandlerRevalidate)
return
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Just making sure I understand correctly - this will only add cache control headers to 404 pages with a revalidate value. What about 404 pages that use getStaticProps but don't specify revalidate?

}

const cacheControl = headers.get('cache-control')
Expand Down
12 changes: 12 additions & 0 deletions tests/integration/page-router.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,10 @@ test<FixtureTestContext>('Should serve correct locale-aware custom 404 pages', a
load(responseImplicitDefaultLocale.body)('[data-testid="locale"]').text(),
'Served 404 page content should use default locale if locale is not explicitly used in pathname (after basePath)',
).toBe('en')
expect(
responseImplicitDefaultLocale.headers['netlify-cdn-cache-control'],
'Response for not existing route if locale is not explicitly used in pathname (after basePath) should have 404 status',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ops this is not right msg - just copied some expect() line from before and forgot to adjust it (and same for other assertions here)

).toBe('s-maxage=31536000, stale-while-revalidate=31536000, durable')

const responseExplicitDefaultLocale = await invokeFunction(ctx, {
url: '/base/path/en/not-existing-page',
Expand All @@ -139,6 +143,10 @@ test<FixtureTestContext>('Should serve correct locale-aware custom 404 pages', a
load(responseExplicitDefaultLocale.body)('[data-testid="locale"]').text(),
'Served 404 page content should use default locale if default locale is explicitly used in pathname (after basePath)',
).toBe('en')
expect(
responseExplicitDefaultLocale.headers['netlify-cdn-cache-control'],
'Response for not existing route if locale is not explicitly used in pathname (after basePath) should have 404 status',
).toBe('s-maxage=31536000, stale-while-revalidate=31536000, durable')

const responseNonDefaultLocale = await invokeFunction(ctx, {
url: '/base/path/fr/not-existing-page',
Expand All @@ -152,4 +160,8 @@ test<FixtureTestContext>('Should serve correct locale-aware custom 404 pages', a
load(responseNonDefaultLocale.body)('[data-testid="locale"]').text(),
'Served 404 page content should use non-default locale if non-default locale is explicitly used in pathname (after basePath)',
).toBe('fr')
expect(
responseNonDefaultLocale.headers['netlify-cdn-cache-control'],
'Response for not existing route if locale is not explicitly used in pathname (after basePath) should have 404 status',
).toBe('s-maxage=31536000, stale-while-revalidate=31536000, durable')
})
Loading