test: add test cases for unexpected permanent cdn cache control cases #2935
+505
−102
Conversation
This file contains hidden or 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
added
the
test all versions
📊 Package size report 0.06%↑
Unchanged files
🤖 This report was automatically generated by pkg-size-action |
pieh
force-pushed
the
fix/narrow-down-permanent-cdn-caching-with-reading-html
branch
from
3c33a68 to
d7b02eb
Compare
pieh
force-pushed
the
fix/narrow-down-permanent-cdn-caching-with-reading-html
branch
from
d7b02eb to
744891a
Compare
pieh
force-pushed
the
fix/narrow-down-permanent-cdn-caching-with-reading-html
branch
from
744891a to
9fe6763
Compare
Draft
…s not during server initialization AND when read html is Next produced fully static html
pieh
commented
May 19, 2025
Comment on lines
-65
to
+68
| let failBuildMock: Mock< | ||
| Parameters<PluginContext['utils']['build']['failBuild']>, | ||
| ReturnType<PluginContext['utils']['build']['failBuild']> | ||
| > | ||
| let failBuildMock: Mock<PluginContext['utils']['build']['failBuild']> |
There was a problem hiding this comment.
Unrelated, but as some point Vitest.Mock type changed and instead of requiring separate params and return type, it now wants just mocked function type so while I was here I adjusted type
pieh
commented
May 19, 2025
Comment on lines
+217
to
+222
| pagesManifest: { | ||
| '/blog/[slug]': 'pages/blog/[slug].js', | ||
| '/test': 'pages/test.html', | ||
| '/test2': 'pages/test2.html', | ||
| '/test3': 'pages/test3.js', | ||
| }, |
There was a problem hiding this comment.
example of important piece of the fix which is using server/pages-manifest.json to get pages paths that end with .html which indicate fully static pages router page
pieh
commented
May 19, 2025
|
|
||
| try { | ||
| await mkdir(destDir, { recursive: true }) | ||
| await Promise.all( | ||
| paths | ||
| .filter((path) => !paths.includes(`${path.slice(0, -5)}.json`)) | ||
| .filter((path) => !path.endsWith('.json') && !paths.includes(`${path.slice(0, -5)}.json`)) |
There was a problem hiding this comment.
just small optim to return false early for json and avoid array search, doesn't change results, because if path ends with .json then paths will also include ${path.slice(0, -5)}.json as it will be same path
pieh
commented
May 19, 2025
Comment on lines
41
to
+46
| const isFallback = fallbacks.includes(path.slice(0, -5)) | ||
| const isFullyStaticPage = !isFallback && fullyStaticPages.includes(path) | ||
|
|
||
| await writeFile( | ||
| join(destDir, await encodeBlobKey(path)), | ||
| JSON.stringify({ html, isFallback } satisfies HtmlBlob), | ||
| JSON.stringify({ html, isFullyStaticPage } satisfies HtmlBlob), |
There was a problem hiding this comment.
we are no longer storing wether html blob is fallback, but we still use it to determine isFullyStaticPage (this is probably not needed anymore, but doesn't seem to hurt to be doubly sure)
pieh
commented
May 19, 2025
| // modifying the next internals. | ||
| // See https://github.com/vercel/next.js/blob/592401bb7fec83079716b2c9b090db580a63483f/packages/next/src/server/next-server.ts#L321-L327 | ||
| // which starts NOT awaited async work | ||
| await new Promise((resolve) => setTimeout(resolve, 5_000)) |
There was a problem hiding this comment.
without some kind of sleep in function, it was VERY hit'n'miss wether issue was reproducable - with this sleep, at this locally it was always reprodicable - but because the whole issue is timing/async - this is not 100% test scenario, but I couldn't figure out anything better without doing some kind of modifications to next.js code itself
pieh
commented
May 19, 2025
Comment on lines
+201
to
+207
| // load netlify.toml if it exists | ||
| if (existsSync(join(ctx.cwd, 'netlify.toml'))) { | ||
| const resolvedNetlifyConfig = await resolveNetlifyConfig({ cwd: ctx.cwd }) | ||
| if (resolvedNetlifyConfig.config) { | ||
| netlifyConfig = resolvedNetlifyConfig.config | ||
| } | ||
| } |
There was a problem hiding this comment.
added this to support one of added test route handlers that reads non-nextjs produced html file and for that to work, we need to pass netlifyConfig produced from netlify.toml which instructs bundling to include some external files
pieh
commented
May 19, 2025
| timeoutMs: 4_000, | ||
| timeoutMs: 10_000, |
There was a problem hiding this comment.
bumped function invocation timeout to allow for 5s sleep in added test scenarios
pieh
commented
May 19, 2025
Comment on lines
-99
to
+107
| if (!file.isFallback) { | ||
| if (file.isFullyStaticPage) { |
There was a problem hiding this comment.
isFullyStaticPage will be false for fallbacks, but also additionally it somewhat inverts the check to only set usedFsReadForNonFallback for KNOWN static pages instead of not setting for fallbacks (but still setting it for everything else even if it's not pages router page)
pieh
commented
May 19, 2025
| @@ -51,6 +51,7 @@ | |||
| "@fastly/http-compute-js": "1.1.5", | |||
| "@netlify/blobs": "^8.2.0", | |||
| "@netlify/build": "^32.2.0", | |||
| "@netlify/config": "^23.0.1", | |||
There was a problem hiding this comment.
Added dev dependency is used only in integration tests fixture setup - it's not being used otherwise
eduardoboucas
approved these changes
May 19, 2025
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
We are catching fs.readFile for .html to set permanent caching header for fully static pages router pages (those that DON'T have
getStaticPropsorgetServerSideProps). However this is quite leaky and it might catch cases when we should NOT do that:getStaticPropsorgetServerSideProps- it will result in reading an.htmlfile (which as our trigger)This adds needed tracking to prevent second case from happening while preserving first one:
Documentation
Tests
Adding some tests that seems to catch problematic cases of not cacheable route handler getting cdn cache control set. Note that there is no great way to reproduce the problem - there is timing and async work involved - in added test I did add some "sleep" in route handler to allow that background work to hopefully finish which without any fixes reproduce problem of setting caching headers for responses that shouldn't have them
Relevant links (GitHub issues, etc.) or a picture of cute animal
https://linear.app/netlify/issue/FRB-1707/next-runtime-is-sending-cdn-cache-control-headers-unexpectedly