fix: apply caching headers to pages router 404 with getStaticProps #2764
+274
−17
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pieh
force-pushed
the
fix/caching-headers-for-get-static-props-404
branch
from
bac472e to
330dca0
Compare
📊 Package size report 0.01%↑
Unchanged files
🤖 This report was automatically generated by pkg-size-action |
pieh
commented
Feb 13, 2025
| @@ -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.
ops this is not right msg - just copied some expect() line from before and forgot to adjust it (and same for other assertions here)
pieh
commented
Feb 14, 2025
src/run/headers.ts
Outdated
Comment on lines
263
to
266
| typeof requestContext.pageHandlerRevalidate !== 'undefined' && | ||
| ['GET', 'HEAD'].includes(request.method) && | ||
| !headers.has('cdn-cache-control') && | ||
| !headers.has('netlify-cdn-cache-control') |
There was a problem hiding this comment.
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
pieh
commented
Feb 14, 2025
src/run/handlers/cache.cts
Outdated
Comment on lines
280
to
284
| const requestContext = getRequestContext() | ||
| if (requestContext) { | ||
| requestContext.pageHandlerRevalidate = revalidate | ||
| } | ||
|
|
There was a problem hiding this comment.
not sure if we should do this for app router - maybe for this PR if we treat it as hot fix - remove this
orinokai
reviewed
Feb 14, 2025
| @@ -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, | |||
There was a problem hiding this comment.
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.
| // handle CDN Cache Control on 404 Page responses | ||
| setCacheControlFromRequestContext(headers, requestContext.pageHandlerRevalidate) | ||
| return | ||
| } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
The method (I extracted from existing handling for App Router Route Handlers) supports false | number for revalidate:
opennextjs-netlify/src/run/headers.ts
Line 220 in 330dca0
false meaning that revalidate is not specified and then it would set ~permanent / 1 year caching header
There was a problem hiding this comment.
The test adjustment I did (minus currently wrong "message") -
opennextjs-netlify/tests/integration/page-router.test.ts
Lines 117 to 132 in 330dca0
revalidate not being specified and test asserting 1 year caching headers - this is used test fixture for it
We currently don't have test case for 404 with getStaticProps + revalidate being actually defined - I did some quick manual test and it seemed to be working (but this was real quick test and with rushing it maybe it's not). We should add a test case for it
… to limit potentially unwanted impact of the change
…od would have set cacheable header
orinokai
reviewed
Feb 24, 2025
| // 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` |
There was a problem hiding this comment.
I think it should be okay to use ${revalidate || 31536000} because we need to account for false and undefined (see comment below), and even if revalidate was 0 then we should be using 1 year.
| ['GET', 'HEAD'].includes(request.method) && | ||
| !headers.has('cdn-cache-control') && | ||
| !headers.has('netlify-cdn-cache-control') | ||
| ) { |
There was a problem hiding this comment.
In addition to removing !cacheControl || cacheControl.includes('no-store') from the condition, I found I had to also remove typeof requestContext.pageHandlerRevalidate !== 'undefined' because 404s without getStaticProps do not go through the cache handler so pageHandlerRevalidate is undefined in those cases. Was it there as a precaution, or was there a specific case you were guarding against? @pieh
There was a problem hiding this comment.
Was it there as a precaution, or was there a specific case you were guarding against? @pieh
Precaution mainly (same like previous cache-control check that we figure out we should remove during our previous sync), but I think the change you made make sense overall - it does widen scenarios of 404 caching where it possibly could not be intentional, but at the same time it catch more cases of 404 not being cached today
| @@ -153,3 +153,124 @@ test<FixtureTestContext>('Should serve correct locale-aware custom 404 pages', a | |||
| 'Served 404 page content should use non-default locale if non-default locale is explicitly used in pathname (after basePath)', | |||
| ).toBe('fr') | |||
| }) | |||
|
|
|||
| describe.only('404 caching', () => { | |||
| describe('404 without getStaticProps', () => { | |||
There was a problem hiding this comment.
I found it was necessary to keep these tests because handling is different for 404 without getStaticProps, however I did remove cases for 'matching dynamic path without revalidate' because behaviour should always be the same as 'matching dynamic path with revalidate' and cache handler should be called in both cases
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
404 responses are currently uncached and this PR introduces cache control headers that are consistent with those generated by Vercel
The change is breaking, as some customers may be relying on the current behaviour, so it is behind an env var for now, but it paves the way to releasing this in the next major version bump.
Tests
Integration tests added
Relevant links (GitHub issues, etc.) or a picture of cute animal
Fixes https://linear.app/netlify/issue/FRB-1636/404-page-with-getstaticprops-is-not-cached-on-cdn