-
Notifications
You must be signed in to change notification settings - Fork 87
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 { | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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' | ||
|
@@ -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 | ||
|
@@ -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 | ||
} | ||
|
||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably should add check for |
||
) { | ||
// handle CDN Cache Control on 404 Page responses | ||
setCacheControlFromRequestContext(headers, requestContext.pageHandlerRevalidate) | ||
return | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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') | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ops this is not right msg - just copied some |
||
).toBe('s-maxage=31536000, stale-while-revalidate=31536000, durable') | ||
|
||
const responseExplicitDefaultLocale = await invokeFunction(ctx, { | ||
url: '/base/path/en/not-existing-page', | ||
|
@@ -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', | ||
|
@@ -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') | ||
}) |
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.
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.