fix(app-router): send Next.js's never-cache header for known-dynamic App pages - #3453
james-elicx wants to merge 11 commits into
Conversation
|
/bigbonk review for issues |
@vinext/cloudflare
create-vinext-app
@vinext/types
vinext
@cloudflare/workers-response-store
commit: |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
| ): AppPageResponsePolicy { | ||
| if (options.isDraftMode) { | ||
| return { cacheControl: NO_STORE_CACHE_CONTROL }; | ||
| return { cacheControl: NEVER_CACHE_CONTROL }; |
There was a problem hiding this comment.
[P2] Preserve the development cache policy here. All of the changed branches now return NEVER_CACHE_CONTROL regardless of options.isProduction, so draft-mode, force-dynamic, revalidate = 0, and early-dynamic dev responses receive the production header. Next.js sets the never-cache value for draft/dynamic production responses but then explicitly overwrites App-page responses with Cache-Control: no-store when routeModule.isDev; vinext has no later dev override on this path. Keep the new value production-only (at minimum retaining the existing no-store policy outside production) and add a dev assertion.
There was a problem hiding this comment.
Fixed in 4521b0d (rebased). The never-cache value is now production-only. Dev keeps its existing no-store policy for draft mode, force-dynamic, revalidate = 0 and early-dynamic responses, and dev assertions cover it.
| expect(probePage).not.toHaveBeenCalled(); | ||
| expect(response.headers.get("x-vinext-cache")).toBeNull(); | ||
| expect(response.headers.get("cache-control")).toBe("no-store, must-revalidate"); | ||
| expect(response.headers.get("cache-control")).toBe("private, no-cache, no-store, max-age=0, must-revalidate"); |
There was a problem hiding this comment.
[P1] Format the two changed long assertions before merging. vp check packages/vinext/src/server/app-page-response.ts tests/app-page-response.test.ts tests/app-page-render.test.ts tests/app-page-dispatch.test.ts currently fails formatting on this file and the analogous changed assertion in tests/app-page-render.test.ts, so the required Check job cannot pass.
There was a problem hiding this comment.
Fixed: the long assertions are formatted, and vp check passes on those files.
|
Posted 2 actionable inline findings. Targeted tests passed; |
|
/bigbonk review for issues |
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
LGTM! |
522adb2 to
66e49eb
Compare
66e49eb to
e1a2459
Compare
e1a2459 to
4521b0d
Compare
|
/bigbonk review for issues Please review exact head |
|
|
LGTM! |
4521b0d to
edaf06b
Compare
|
/bigbonk review for issues Please review exact head |
|
LGTM! |
edaf06b to
f3d4efc
Compare
|
/bigbonk review for issues Please review exact head |
|
LGTM! |
f3d4efc to
3c62f67
Compare
|
/bigbonk review for issues Please review exact head |
| copyLinkHeaderProvenance(response.headers, stamped.headers); | ||
| applyCdnResponseHeaders(stamped.headers, { cacheControl: NEVER_CACHE_CONTROL }); | ||
| applyCdnResponseHeaders(stamped.headers, { | ||
| cacheControl: resolveUncacheableCacheControl(options.isProduction), |
There was a problem hiding this comment.
[P1] Apply this policy to all known-dynamic early responses
applyIneligibleRouteCachePolicy returns immediately when isDraftMode is true or isStaticEligible is true. Consequently draft-mode early responses and cacheComponents force-dynamic/revalidate = 0 early responses (for example a prepareElement redirect, probe special error, or SSR recovery response) never reach this changed header; the outer finalizer even skips redirects. These can leave without Next.js’s never-cache value. Make this helper recognize the same known-dynamic conditions as the normal response policies and add early-response coverage for them.
There was a problem hiding this comment.
Fixed in 39c83b4. The early-response stamp now uses the same known-dynamic conditions as the normal response policies: a route that can't be static, draft mode, force-dynamic, or revalidate = 0. So draft-mode early responses, and cacheComponents force-dynamic / revalidate = 0 early responses, get Next.js's never-cache value (no-store in dev). The early-response test now covers a prepared redirect under each of the three, on a route that can otherwise be static.
|
Posted 1 actionable inline finding. Targeted tests and checks passed; worktree remains clean at |
1ec5417 to
7ebbccd
Compare
|
/bigbonk review for issues Please review exact head |
| const dynamicParamsResponse = await validateAppPageDynamicParams({ | ||
| enforceStaticParamsOnly: options.dynamicParamsConfig === false, | ||
| generateStaticParams: options.generateStaticParams, | ||
| enforceStaticParamsOnly: dynamicParamsConfig === false, |
There was a problem hiding this comment.
[P1] Treat intercept-only dynamic segments as a dynamic route
isDynamicRoute below still comes from the promoted source route. A static /feed can own @modal/(.)photo/[id] with dynamicParams = false; for an intercepted /photo/unknown request the current route is /feed, so route.isDynamic is false and validateAppPageDynamicParams returns before consulting the intercepted tree config/generator. The unknown target then renders 200. Derive this flag from the effective intercepting route/tree (or otherwise force validation when that tree introduces params), and cover a static source with a dynamic intercept target.
There was a problem hiding this comment.
The interception work moved out of this PR into #3473, which now fixes this in 214f417: an attached intercept's own dynamic segments make the params gate treat the intercepting tree as dynamic, so a static /feed rendering @modal/(.)photo/[id] under dynamicParams = false 404s an id its generators never returned. Covered in the dispatch tests.
| generateStaticParams: options.generateStaticParams, | ||
| enforceStaticParamsOnly: dynamicParamsConfig === false, | ||
| generateStaticParams: currentRouteIntercept | ||
| ? options.resolveRouteGenerateStaticParams?.(route, currentRouteIntercept) |
There was a problem hiding this comment.
[P1] Pass the complete intercepted-route parameter schema
Selecting the intercepting tree generators is incomplete without its required/optional parameter names. This call supplies neither, so areStaticParamsAllowed checks only keys present in the request and allows missing values. For @modal/(.)[[...photo]] with dynamicParams = false and a generator returning only { photo: ["known"] }, an interception where photo is omitted is accepted even though Next.js does not generate that path (it requires an explicit empty optional-catch-all value). Resolve the effective tree param names/types and pass requiredParamNames plus optionalCatchAllParamNames, with omitted optional-catch-all coverage.
There was a problem hiding this comment.
The interception work moved out of this PR into #3473, which now fixes this in 4a073a3. The gate now also compares the intercepting tree's optional catch-alls that the request omits, for both independent and chained generators. So @modal/(.)[[...photo]] 404s when the generators return only { photo: ["known"] }, and renders for { photo: [] }. I didn't pass requiredParamNames: in the validator that means metadata-route semantics, which reject every chained-only generator set and generated sets that lack a key a parent layout supplies.
|
Posted 2 actionable inline findings. All 227 targeted tests passed; worktree remains clean at |
…App pages Next.js sends private, no-cache, no-store, max-age=0, must-revalidate for every App page render it knows is dynamic (revalidate 0) and for draft mode. vinext sent no-store, must-revalidate. Use Next.js's header for draft mode, force-dynamic, revalidate = 0, and renders that are dynamic before headers are sent. Streamed misses whose store decision isn't known when headers go out keep the origin no-store header.
…wn-dynamic routes
…esponses An otherwise static page that calls connection() and then redirect() during the probe, or reads a dynamic API before SSR recovers, now gets the never-cache header on its early response too. As in the HTML response policy, force-static and dynamic = "error" without a revalidate period stay static.
…responses The HTML response policy keeps nonce-bearing and progressive action renders no-store, after draft mode, non-static routes and force-dynamic but before revalidate = 0 and dynamic usage. Early responses now follow the same order. Generated-param misses also pass every known-dynamic input, so they get the same header as the other early responses.
…rce-dynamic and revalidate = 0 routes In a cacheComponents build these routes stay static-eligible, so the intercepted RSC response left without the header their own render sends.
…or its RSC cache policy The direct intercepted RSC response renders the source route, but its force-dynamic and revalidate = 0 checks read the matched target's config. A cacheComponents build keeps every route static-eligible, so a force-dynamic source left without the never-cache header, and a force-dynamic target stamped it on a static source's payload. The response now uses the source config activated for the render.
…boundary route /boundary-ssr-only reads cookies(), so it is dynamic. Its assertion only held because the build prerendered it: the static pass rethrows the SSR shell error, renders the local error.tsx on the server, and that response went out without the never-cache header, so the page was stored as a static 200. Early responses now count the dynamic API read, so the route is skipped as dynamic, as Next.js does. The runtime pass serves the __next_error__ shell and the browser re-render succeeds, the same result an uncached request already got before. The spec now asserts that, which also fails if the dynamic route is prerendered again.
…rly-response policy It passed only the static-eligibility inputs, so production read as dev and sent dev's no-store header instead of Next.js's never-cache header.
7ebbccd to
9c02b87
Compare
|
/bigbonk review for issues Please review exact head |
| const isKnownDynamic = | ||
| options.revalidateSeconds === 0 || | ||
| (!ignoresDynamicUsage && (options.peekDynamicUsage?.() ?? peekDynamicUsage())); | ||
| if (!isKnownDynamic) return null; |
There was a problem hiding this comment.
[P1] Apply no-store before returning for nonce/action renders
For an otherwise static-eligible HTML render with no dynamic API read and a nonzero/null revalidate value, isKnownDynamic is false and this return runs before the nonce/progressive-action branch. An early response from prepareElement, a probe, or SSR recovery therefore gets no Cache-Control at all, whereas resolveAppPageHtmlResponsePolicy makes these render modes unconditionally no-store; that can expose request-specific nonce/action output to caching. Move the HTML nonce/progressive-action check before this return (after the draft/non-static/force-dynamic precedence) and add cases with no other dynamic signal.
There was a problem hiding this comment.
Fixed in efd7761: early responses now give nonce-bearing and progressive action HTML renders no-store right after the draft mode, non-static and force-dynamic checks, as the HTML policy does, with or without another dynamic signal. Added cases with no other dynamic signal.
|
Posted 1 actionable inline finding. All 209 targeted tests and checks passed; worktree remains clean at |
…-store An early response of a static-eligible nonce-bearing or progressive action HTML render without another dynamic signal left without a Cache-Control header. The HTML policy makes those renders no-store right after draft mode, non-static routes and force-dynamic, whatever else they do, and early responses now follow the same order.
|
/bigbonk review for issues Please review exact head |
|
LGTM! |
1 participant
Stacked on #3452. Third PR in the stack that brings ISR query-key handling in line with Next.js.
Problem
Next.js sends
Cache-Control: private, no-cache, no-store, max-age=0, must-revalidatefor every App page response it knows is dynamic (getCacheControlHeaderwithrevalidate: 0), and for draft mode (build/templates/app-page.ts). vinext sentno-store, must-revalidatefor these responses.Fix
The HTML and RSC response policies now send
NEVER_CACHE_CONTROLfor:dynamic = "force-dynamic"andrevalidate = 0. These routes are already non-eligible after fix(app-router): only full-page cache routes Next.js classifies as static or SSG #3451, so this only changes cacheComponents builds;dynamicUsedDuringRenderfor HTML,dynamicUsedDuringBuildfor RSC).A direct intercepted RSC response gets the same header from its source route's config. Deriving intercepted renders' config from the intercepting tree isn't part of this stack. That's a parity gap already on
main; the parked draft #3473 records one approach.Some responses keep
no-store, must-revalidate:Tests
tests/app-page-response.test.ts: the policy matrix now expects the Next.js header for draft mode,force-dynamic,revalidate = 0and dynamic renders. Script-nonce and progressive-action renders still expectno-store, must-revalidate.tests/app-page-dispatch.test.tsandtests/app-page-render.test.ts: the two dynamic-before-headers assertions are updated./use-cache(connection()at the top) gets the Next.js header. 23/23 pass locally.