Fix inconsistency with 404 getStaticProps cache-control (#66674)
While investigating unexpected stale responses when leveraging `getStaticProps` on the 404 page noticed we have an inconsistency between local and deployed due to `Cache-Control` being set to `no-store, must-revalidate` even though a revalidate period is provided. The inconsistency also differs between the HTML response and the data response `_next/data` which causes even more unexpected behavior. To avoid this behavior, this replaces the handling to ensure we honor the originally provided revalidate period during `notFound: true` for the `Cache-Control` header. Validated against provided reproduction here https://github.com/fusdev0/next-notfound-revalidate Deployment: https://vercel.live/link/next-notfound-revalidate-govzskknf-vtest314-ijjk-testing.vercel.app/fallback-blocking/fasdf Prior PR for prior context that introduced this https://github.com/vercel/next.js/pull/19165 x-ref: [slack thread](https://vercel.slack.com/archives/C0676QZBWKS/p1717492459342109)
This commit is contained in:
parent
755c9e445b
commit
16caf41995
4 changed files with 33 additions and 10 deletions
|
@ -2892,9 +2892,8 @@ export default abstract class Server<
|
|||
typeof cacheEntry.revalidate !== 'undefined' &&
|
||||
(!this.renderOpts.dev || (hasServerProps && !isNextDataRequest))
|
||||
) {
|
||||
// If this is a preview mode request, we shouldn't cache it. We also don't
|
||||
// cache 404 pages.
|
||||
if (isPreviewMode || (is404Page && !isNextDataRequest)) {
|
||||
// If this is a preview mode request, we shouldn't cache it
|
||||
if (isPreviewMode) {
|
||||
revalidate = 0
|
||||
}
|
||||
|
||||
|
@ -2906,6 +2905,18 @@ export default abstract class Server<
|
|||
}
|
||||
}
|
||||
|
||||
// If we are rendering the 404 page we derive the cache-control
|
||||
// revalidate period from the value that trigged the not found
|
||||
// to be rendered. So if `getStaticProps` returns
|
||||
// { notFound: true, revalidate 60 } the revalidate period should
|
||||
// be 60 but if a static asset 404s directly it should have a revalidate
|
||||
// period of 0 so that it doesn't get cached unexpectedly by a CDN
|
||||
else if (is404Page) {
|
||||
const notFoundRevalidate = getRequestMeta(req, 'notFoundRevalidate')
|
||||
revalidate =
|
||||
typeof notFoundRevalidate === 'undefined' ? 0 : notFoundRevalidate
|
||||
}
|
||||
|
||||
// If the cache entry has a revalidate value that's a number, use it.
|
||||
else if (typeof cacheEntry.revalidate === 'number') {
|
||||
if (cacheEntry.revalidate < 1) {
|
||||
|
@ -2953,6 +2964,12 @@ export default abstract class Server<
|
|||
}
|
||||
|
||||
if (!cachedData) {
|
||||
// add revalidate metadata before rendering 404 page
|
||||
// so that we can use this as source of truth for the
|
||||
// cache-control header instead of what the 404 page returns
|
||||
// for the revalidate value
|
||||
addRequestMeta(req, 'notFoundRevalidate', cacheEntry.revalidate)
|
||||
|
||||
if (cacheEntry.revalidate) {
|
||||
res.setHeader(
|
||||
'Cache-Control',
|
||||
|
@ -2971,7 +2988,6 @@ export default abstract class Server<
|
|||
if (this.renderOpts.dev) {
|
||||
query.__nextNotFoundSrcPage = pathname
|
||||
}
|
||||
|
||||
await this.render404(req, res, { pathname, query }, false)
|
||||
return null
|
||||
} else if (cachedData.kind === 'REDIRECT') {
|
||||
|
|
|
@ -94,6 +94,11 @@ export interface RequestMeta {
|
|||
cacheEntry: any,
|
||||
requestMeta: any
|
||||
) => Promise<boolean | void> | boolean | void
|
||||
|
||||
/**
|
||||
* The previous revalidate before rendering 404 page for notFound: true
|
||||
*/
|
||||
notFoundRevalidate?: number | false
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
@ -14,6 +14,6 @@ export const getStaticProps = () => {
|
|||
notFound: true,
|
||||
random: Math.random(),
|
||||
},
|
||||
revalidate: 1,
|
||||
revalidate: 6000,
|
||||
}
|
||||
}
|
||||
|
|
|
@ -81,9 +81,9 @@ const runTests = () => {
|
|||
let res = await fetchViaHTTP(appPort, '/fallback-blocking/hello')
|
||||
let $ = cheerio.load(await res.text())
|
||||
|
||||
const privateCache =
|
||||
'private, no-cache, no-store, max-age=0, must-revalidate'
|
||||
expect(res.headers.get('cache-control')).toBe(privateCache)
|
||||
expect(res.headers.get('cache-control')).toBe(
|
||||
`s-maxage=1, stale-while-revalidate`
|
||||
)
|
||||
expect(res.status).toBe(404)
|
||||
expect(JSON.parse($('#props').text()).notFound).toBe(true)
|
||||
|
||||
|
@ -91,7 +91,9 @@ const runTests = () => {
|
|||
res = await fetchViaHTTP(appPort, '/fallback-blocking/hello')
|
||||
$ = cheerio.load(await res.text())
|
||||
|
||||
expect(res.headers.get('cache-control')).toBe(privateCache)
|
||||
expect(res.headers.get('cache-control')).toBe(
|
||||
`s-maxage=1, stale-while-revalidate`
|
||||
)
|
||||
expect(res.status).toBe(404)
|
||||
expect(JSON.parse($('#props').text()).notFound).toBe(true)
|
||||
|
||||
|
@ -146,7 +148,7 @@ const runTests = () => {
|
|||
let $ = cheerio.load(await res.text())
|
||||
|
||||
expect(res.headers.get('cache-control')).toBe(
|
||||
'private, no-cache, no-store, max-age=0, must-revalidate'
|
||||
`s-maxage=1, stale-while-revalidate`
|
||||
)
|
||||
expect(res.status).toBe(404)
|
||||
expect(JSON.parse($('#props').text()).notFound).toBe(true)
|
||||
|
|
Loading…
Reference in a new issue