fix: apply caching headers to pages router 404 with getStaticProps #2764
+61
−12
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.02%↑
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
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
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?
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
Documentation
Tests
You can test this change yourself like so:
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