fix(app-router): only full-page cache routes Next.js classifies as static or SSG - #3451
james-elicx wants to merge 22 commits into
Conversation
…atic or SSG A dynamic-segment App page with revalidate but no generateStaticParams at its last dynamic segment, or one on the edge runtime, is dynamic in Next.js: it renders per request, is never ISR-cached, and accepts POST. vinext cached it. Classify routes with the Next.js segment walk and skip the cache read, write and RSC capture for routes that are not static or SSG, returning the never-cache Cache-Control header.
@vinext/cloudflare
create-vinext-app
@vinext/types
vinext
@cloudflare/workers-response-store
commit: |
|
/bigbonk review for issues |
Performance benchmarksCompared 1 improved · 0 regressed · 5 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
| if (options.isStaticGenerationEdgeRuntime) return false; | ||
| if (options.dynamicConfig === "force-dynamic" || options.revalidateSeconds === 0) return false; | ||
| if (options.dynamicConfig === "force-static" || options.dynamicConfig === "error") return true; | ||
| return !options.isDynamicRoute || options.hasGenerateStaticParams; |
There was a problem hiding this comment.
[P2] Preserve paths fully generated by an ancestor
This fallback makes every nested dynamic route whose last dynamic segment lacks its own generateStaticParams ineligible. Next.js also creates prerenderedRoutes when an earlier generator returns values for all pathname params (hadAllParamsGenerated in build/static-paths/app.ts). For example, /[category]/[slug] is SSG when the [category] layout returns { category: "docs", slug: "intro" }, even with no generator below it. Vinext already preserves and validates those extra keys in generateChainedStaticParams, but this check now skips cache reads/writes (and the 405 policy) for that generated path. Please include the concrete generated-path result in eligibility rather than relying only on the export's position.
There was a problem hiding this comment.
Intentionally out of scope, and documented in 95171ca. Classifying this shape needs generateStaticParams output. vinext only calls the generator on a miss for dynamicParams = false routes, and calling it on every request just to classify the route would tax the common version of this shape, which is ƒ in Next.js. So all backends treat it as dynamic. vinext stores less here, never more. The workaround is valid Next.js too: add generateStaticParams to the last dynamic segment (even return []).
|
Posted 1 actionable inline finding. Targeted tests passed: 203 tests across 4 files. |
|
/bigbonk review for issues |
| if (!branch) continue; | ||
| const branchSegments = branch.routeSegments ?? []; | ||
| // The slot folder is a child of the main-tree folder at this depth. | ||
| const slotDepth = routeSegments.length - branchSegments.length + 1; |
There was a problem hiding this comment.
[P2] Use the slot owner position instead of deriving depth
This inference breaks when the main and slot branches have different URL-invisible route groups. For example, with app/(main)/[id]/page.tsx exporting generateStaticParams, and a root app/@panel/[id]/page.tsx without it, Next.js BFS visits the slot [id] before the main page and finishes with the page export, so the route is SSG. Here routeSegments.length - branchSegments.length counts (main) as though it were above the slot, placing the slot [id] at the same depth as the main page; the later slot segment clears the flag and makes the route dynamic. Thread the slot’s actual ownerTreePosition into this collector (the route graph already records it) rather than reconstructing ownership from branch lengths.
There was a problem hiding this comment.
Fixed in 8bd74be. The static-params walk now visits segments in Next.js's loader tree order, using each slot's real ownerTreePosition from the route graph instead of working ownership out from branch lengths. That covers the (main) route group case.
|
Posted 1 actionable inline finding. Targeted tests passed: 203 tests across 4 files. |
… tree order Visit slots with a matched page before children at each level and default slots after, place each slot under the folder that owns it, and read only a default slot's default module. Require isStaticEligible on every cache policy and finalizer, and keep the route-cache-identity fixture ISR-cached.
|
/bigbonk review for issues |
| isForceDynamic: options.isForceDynamic, | ||
| isForceStatic: options.isForceStatic, | ||
| isProduction: options.isProduction, | ||
| isStaticEligible: options.isStaticEligible, |
There was a problem hiding this comment.
[P1] Mark ineligible routes dynamic in client cache metadata
This flag only disables the server cache policy. The RSC path below still sets staleTimePending because no dynamic API was observed, and the HTML metadata getter still returns kind: "static". A pure dynamic-segment page without generateStaticParams will therefore seed/reuse client navigation data with the static/pending TTL even though this PR now classifies it as dynamic and rerenders it on every server request. Include !isStaticEligible in the known-dynamic stale-time and initial-navigation-kind decisions so client navigations do not reuse this output as static.
There was a problem hiding this comment.
Reverted in 7e27732: marking ineligible routes dynamic for the client broke explicit full-prefetch reuse (segment-cache-client-params E2E). Eligibility stays a server read/write gate only; client metadata keeps following force-dynamic or observed dynamic usage.
| "layoutTreePositions" | "layouts" | "page" | "parallelBranches" | "routeSegments" | ||
| >, | ||
| ): AppPageStaticParamsWalkSegment[] { | ||
| const routeSegments = options.routeSegments ?? []; |
There was a problem hiding this comment.
[P2] Model a default children branch instead of the synthetic URL path
For routes materialized only by a nested slot, routeSegments describes the slot URL while route.page can be the owner’s default.tsx (childrenSlot.state === "default"). This collector currently places that default page after every synthetic URL segment. In Next.js the loader tree places __DEFAULT__ directly below the slot owner, so BFS can visit it before a deeper slot dynamic segment. For example, a root children default exporting generateStaticParams plus @feed/[id]/page without it ends false in Next’s walk but true here, enabling full-page caching. Thread the children-slot state/owner into this collector and emit __DEFAULT__ at its actual tree position.
| isStaticGenerationEdgeRuntime: __isEdgeRuntime( | ||
| __resolveAppPageStaticGenerationRuntime([ | ||
| ...route.layouts.map((layout) => layout?.runtime), | ||
| route.page?.runtime, |
There was a problem hiding this comment.
[P2] Resolve runtime from the page that materialized a slot-only route
Ignoring slots is correct for an ordinary children page, but vinext also synthesizes URLs whose only matched page is a nested slot and whose route.page is merely the children fallback. For app/@feed/foo/page.tsx with runtime = "edge", this checks the fallback module instead of the slot page, so the static URL remains eligible and can be cached; Next builds that route from the slot page file and sees its edge runtime. Preserve the originating page for synthetic routes (or otherwise select the active slot leaf) when resolving static-generation runtime.
| const response = await request.get(url); | ||
| const headers = response.headers(); | ||
| expect(response.ok(), JSON.stringify({ backend, headers })).toBe(true); | ||
| expect(headers["cache-control"]).toBe( |
There was a problem hiding this comment.
[P1] Do not require one backend’s exact no-store spelling
This spec runs for response-store, workers-cache, and kv. The Workers Cache completed-response admission intentionally rewrites a denied response through responseWithCachePolicy(..., null), which emits no-store, must-revalidate, so the workers-cache deploy job will fail this exact equality even though the response is correctly uncacheable. Assert that the value contains no-store (as the force-dynamic check above does), or branch by backend.
|
Posted 4 actionable inline findings. Targeted tests passed: 206 tests across 4 files. Worktree unchanged. |
… Next.js - Treat routes that can't be static as known-dynamic for the client navigation stale time, not only force-dynamic ones. - Walk a slot-only route's children default directly under its owner. - Read the runtime of a slot-only route from the slot page that materializes it. - Accept any no-store spelling in the deployed spec, since Workers Cache rewrites denied responses.
|
/bigbonk review for issues |
|
| let treePath = [...slotPath]; | ||
| branchSegments.forEach((name, index) => { | ||
| treePath = [...treePath, 0]; | ||
| const layout = configLayoutsByPosition.get(index + 1); |
There was a problem hiding this comment.
[P2] Include route-group layouts when the slot has no URL segments
configLayoutsByPosition can contain positions that have no corresponding branchSegments entry. In particular, discoverParallelSlots represents app/[id]/@details/(variant)/page.tsx with routeSegments: [] and the (variant)/layout.tsx at position 1. This loop never emits that layout, so if it exports generateStaticParams, Next.js's BFS sets the SSG flag after visiting [id], while vinext returns false and disables full-page caching. Walk all config-layout tree positions, including URL-invisible route-group levels, rather than only looking them up while iterating routeSegments.
| ): unknown[] { | ||
| const layoutRuntimes = (options.layouts ?? []).map((layout) => layout?.runtime); | ||
| const childrenDefaultPosition = resolveChildrenDefaultTreePosition(options.childrenSlot); | ||
| if (childrenDefaultPosition === null) return [...layoutRuntimes, options.page?.runtime]; |
There was a problem hiding this comment.
[P1] Detect slot-only routes whose children come from a catch-all
A slot-only synthetic route can have childrenSlot.state === "active" because its children fall through to a sibling catch-all (app/[...catchAll]/page.tsx plus app/@slot/baz/page.tsx; this shape is already covered in app-route-graph.test.ts). In that case this early return reads the catch-all page's runtime and ignores the slot page that materialized /baz. If the slot page exports runtime = "edge", the static /baz route remains eligible and can be full-page cached, unlike Next.js. Preserve the materializing-slot identity independently of whether children uses a default or catch-all, and use that slot's runtime here.
|
Posted 2 actionable inline findings. Targeted tests passed: 211 tests across 4 files. Worktree unchanged. |
…slot's runtime - Walk a slot's route-group layouts even when its page has no URL segments. - Mark routes that only a nested slot page materializes, and read their static-generation runtime from that slot page whatever renders the children.
|
/bigbonk review for issues |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues Please review exact head |
| // Next.js renders these routes per request, so they are dynamic before any | ||
| // request API runs. Their payload is never stored, even when a cacheLife | ||
| // resolves during the render. | ||
| const isKnownDynamicRoute = options.isForceDynamic || !options.isStaticEligible; |
There was a problem hiding this comment.
[P1] Keep full-prefetch payloads reusable for config-dynamic routes
Treating every ISR-ineligible route as known-dynamic also changes the client-cache metadata, and the existing segment-cache-client-params E2E now fails consistently: after a full hover prefetch of /.../clothing/1, clicking issues another full RSC request instead of consuming that payload. This route has dynamic segments and no generateStaticParams, but consists only of client components; Next.js still allows the explicit full prefetch to be reused even though the server must not persist it in the full-page cache. Please keep isStaticEligible as the server read/write gate without using it to force the dynamic stale-time/initial-navigation classification; those should remain based on force-dynamic or observed dynamic usage.
|
Posted 1 actionable inline finding. Targeted unit tests passed: 213 tests across 4 files. Worktree unchanged. |
…static Full-page cache eligibility is a server read/write gate. Using it for the client stale time and initial-navigation kind stopped explicit full prefetches of client-only dynamic-segment pages from being reused.
|
/bigbonk review for issues Please review exact head |
|
Posted 3 actionable inline findings. Targeted tests passed: 241 tests across 4 files. Worktree unchanged. |
|
/bigbonk review for issues Please review exact head |
| // Loading the intercepting tree for admission is like loading the | ||
| // route's own modules: generated params are still checked only once the | ||
| // cache misses, so a cache hit never runs generateStaticParams. | ||
| await loadAppPageCurrentRouteIntercept(currentRouteIntercept, route, routeHasSlot); |
There was a problem hiding this comment.
[P1] Skip loading a slot intercept that is not attached
A real slot intercept can resolve to a concrete current-route variant that does not contain intercept.slotKey; __resolveRouteStaticEligible correctly classifies that case as the unchanged source tree. This call nevertheless runs __pageLoader/layout/not-found loaders before that attachment check. On the newly added cache-hit path, an unused intercept module with a top-level throw or fetch can now fail or delay the request before isrGet, even though the cached/source response does not render that branch. Gate loadAppPageCurrentRouteIntercept on the sibling-page sentinel or routeHasSlot(route, currentRouteIntercept.slotKey) (as the owner-default loader already does).
There was a problem hiding this comment.
Fixed in bac7c59 and d7df05b. loadAppPageCurrentRouteIntercept returns early unless the intercept is attached (the sibling-page sentinel, or routeHasSlot(route, slotKey)), so on the cache gate an unattached slot intercept evaluates none of its page, not-found, layout or owner-default modules before isrGet; eligibility still classifies it as the unchanged source tree. The render path (resolveAppPageIntercept), the pre-render probe (resolveAppPageProbeIntercept), and the rerender target used by stale regeneration and core RSC hits (resolveAppPageInterceptionRerenderTarget, via an optional routeHasSlot) apply the same check. Tests cover the HIT, the miss, the stale regeneration and the probe fan-out with throwing loaders.
|
Posted 1 actionable inline finding. Targeted tests passed: 148 tests across 2 files. Worktree unchanged. |
|
/bigbonk review for issues Please review exact head |
| const interceptOwner = resolveParallelBranchOwnerPosition(slot, sourceDepth); | ||
| const parallelBranches = replaceSiblingsWithDefaults(interceptOwner, options.slotIndex); | ||
| parallelBranches[options.slotIndex] = { | ||
| configLayouts: interceptLayouts, |
There was a problem hiding this comment.
[P1] Preserve common slot layouts above the intercept marker
interceptLayouts contains only layouts at or below the marker (the route scanner starts collectInterceptingPages at interceptDir), so replacing slot.configLayouts wholesale drops layouts on the shared slot ancestry. For example, app/@modal/gallery/layout.tsx with dynamic = "force-dynamic" plus app/@modal/gallery/(.)photo/page.tsx loses the gallery config here; both eligibility passes can then classify the intercepted tree static and allow its RSC response/cache variant without no-store, unlike Next.js. Keep the prefix of slot.configLayouts before the marker and append the intercept layouts (with corresponding tree positions).
There was a problem hiding this comment.
Fixed in 3d4c26d. A slot intercept's tree now keeps the slot's layouts in the folders between the slot root and the intercept marker, found by the route scanner and emitted as interceptAncestorLayouts with tree positions. This is the same walk Next.js's loader tree does (app_structure.rs directory_tree_to_loader_tree_internal). I didn't keep a prefix of slot.configLayouts: those come from the source's active slot page, which in this example doesn't exist (/gallery's @modal renders its default), and they can come from a different subtree, such as a catch-all. Tests cover the tree shape and dynamic mode (app-segment-config), the codegen, dispatch's classification (private, no-store), and the module loader.
|
Posted 1 actionable inline finding. Focused tests passed: 183 tests across 2 files. Worktree unchanged. |
|
/bigbonk review for issues Please review exact head |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| targetPattern: ${JSON.stringify(ir.targetPattern)}, | ||
| sourceMatchPattern: ${JSON.stringify(ir.sourceMatchPattern)}, | ||
| sourcePageSegments: ${JSON.stringify(ir.sourcePageSegments)}, | ||
| interceptAncestorLayouts: ${moduleArray(ir.ancestorLayoutPaths?.length ?? 0)}, |
There was a problem hiding this comment.
[P1] Include ancestor layouts in the rendered intercept tree
These modules now flow into static eligibility, but the actual slot override and head inputs still consume only interceptLayouts (buildSlotOverrides and resolveActiveParallelRouteHeadInputs). For app/@modal/gallery/layout.tsx plus app/@modal/gallery/(.)photo/page.tsx, Next.js wraps the intercepting page and metadata with the gallery layout; vinext loads and classifies that module but never renders it. This also lets a static-enabling config from an unrendered layout influence cache admission. Merge the ancestor modules and their segment positions into the render/head intercept branch as well (or expose one combined layout chain).
There was a problem hiding this comment.
Fixed in 23c73f7. The slot's layouts above the marker now start the intercept's own layout chain in the route scanner: for @modal/gallery/(.)photo that's layoutPaths [@modal/gallery/layout.tsx, @modal/gallery/(.)photo/layout.tsx] with segments [["gallery"], ["gallery", "(.)photo"]]. Slot overrides, the slot branch render, head/metadata inputs, boundary render, static eligibility, the request-stage forceDynamic scan and the action-owner manifest all read that one list, so the gallery layout wraps the intercepted page and its metadata as in Next.js (next-app-loader createSubtreePropsFromSegmentPath nests each folder's layout). The separate interceptAncestorLayouts plumbing from 3d4c26d is removed. Tests: app-page-element-builder (render and metadata include the gallery layout), app-route-graph and the codegen test.
|
Posted 1 actionable inline finding. Targeted tests passed: 218 tests across 4 files. Worktree unchanged. |
|
/bigbonk review for issues Please review exact head |
| ); | ||
| const cachePolicy = { | ||
| isDraftMode, | ||
| isStaticEligible, |
There was a problem hiding this comment.
[P2] Use the effective interception eligibility for this early response
A current-route interception may already have changed renderStaticEligible above, but a generated-param miss is stamped with the base route’s isStaticEligible. For example, a static /feed/[slug] with dynamicParams = false plus a force-dynamic current-route intercept skips the cache, then an unknown slug returns this 404 without no-store, even though the same effective tree’s normal render is never-cache. Pass renderStaticEligible here so both the rendered and plain miss responses use the tree that was classified for admission.
There was a problem hiding this comment.
Fixed in c2a6e98: the generated-params miss block's cache policy now uses renderStaticEligible, so both the rendered and the plain 404 follow the tree classified for admission. The test covers your example (a force-dynamic current-route intercept over a static /feed/[slug] with dynamicParams = false): 404 with the never-cache header, no cache read and no write. It fails without the fix.
|
Posted 1 actionable inline finding. Targeted tests passed: 318 tests across 4 files. Worktree unchanged. |
|
/bigbonk review for issues Please review exact head |
| // response's cacheability too, like the render it replaces. | ||
| const cachePolicy = { | ||
| isDraftMode, | ||
| isStaticEligible: renderStaticEligible, |
There was a problem hiding this comment.
[P2] Classify uncached current-route intercept misses
renderStaticEligible is only updated in the shouldReadCache block above. When cache reads are disabled (notably isProduction: false, but also the interception-cache bypass), generated-param validation still runs and returns here before resolveAppPageIntercept can classify the active tree. A static /feed/[slug] with dynamicParams = false and a force-dynamic current-route intercept therefore sends an unknown-slug 404 without no-store in dev, while its normal render gets the never-cache policy. The new test inherits isProduction: true, so it misses this branch. Resolve/classify the attached intercept before stamping this early response as well.
There was a problem hiding this comment.
Fixed in 9d4c852. The current-route intercept is now matched, loaded (only when the route has its slot) and classified into renderStaticEligible whether or not the cache is read, before the generated-params miss returns. The cache read is still gated on shouldReadCache, and generated params are still checked only after a miss. Tests cover your example in dev and with the interception-cache bypass, and both fail without the fix. A guard test checks that an intercept the route has no slot for still isn't loaded.
|
Posted 1 actionable inline finding. |
|
/bigbonk review for issues Please review exact head |
| interceptSegment, | ||
| ...path.relative(interceptRoot, path.dirname(page)).split(path.sep).filter(Boolean), | ||
| ]; | ||
| const ancestorLayouts = slotRootDir |
There was a problem hiding this comment.
[P1] Preserve ancestor layouts for sibling intercepts too
This only collects layouts above the marker when slotRootDir exists, so slot-less sibling intercepts still lose route-group/folder layouts between the source owner and the marker. For example, app/feed/page.tsx plus app/feed/(shell)/layout.tsx exporting dynamic = "force-dynamic" and app/feed/(shell)/(.)photo/page.tsx produces an intercept with no (shell) layout. The sibling tree is therefore rendered/classified without that layout and its RSC response can omit no-store, whereas Next.js includes every folder on the intercepting app path. Derive the sibling ancestor chain from the source owner/marker path as well.
There was a problem hiding this comment.
The interception work moved out of this PR into #3473, which now fixes this in 4714822: a sibling intercept below its source page's folder starts its branch, layout chain and loading positions at that folder, so app/feed/(shell)/layout.tsx (and a loading.tsx there) wrap app/feed/(shell)/(.)photo and feed its classification. Route-graph and codegen tests cover it.
|
Posted 1 actionable inline finding. Targeted tests passed; worktree unchanged. |
…er tree The walk put slots that matched a page before children and slots that render default after it, as the webpack loader does. Next.js's default build (Turbopack) puts children first and every slot after it in folder-name order, and collectAppPageSegments walks that tree breadth-first. A main [id] page with generateStaticParams next to a slot [id] segment without it ended static here, but dynamic in Next.js, so the route could be full-page cached.
… loader tree Slot runtimes counted only for a route that a slot page materializes, so app/page.tsx with an edge app/@panel/page.tsx left / static-eligible and full-page cacheable. Next.js's default build (Turbopack) derives the runtime from the whole loader tree: every parallel branch (children, matched slots and default slots) merges at each node, and the node's own layout, page or default fills only an unset value. The runtime now comes from the same tree the generateStaticParams walk builds, which drops the materializedBySlot route flag.
…issing default export response The 500 for a page module without a default export returned before any place that applies the route's cache policy, so an edge-runtime or dynamic-segment page sent it without Cache-Control.
9d4c852 to
b2b9419
Compare
|
/bigbonk review for issues Please review exact head |
|
LGTM! |
2 participants
First PR in the stack that brings ISR query-key handling in line with Next.js. It classifies App pages the way Next.js does before any caching.
Problem
vinext ISR-cached some App pages that Next.js treats as dynamic (
ƒ) and renders per request:revalidate(or a cacheLife) but has nogenerateStaticParams;generateStaticParamsis only on a parent segment, not at the last dynamic segment;These routes also returned 405 for non-action
POST/PUT. In Next.js they accept those methods.Fix
hasAppPageGenerateStaticParamsAtLastDynamicSegmentports Next.js's breadth-first segment walk frombuildAppStaticPaths(N11):generateStaticParamsclears the SSG flag, and one with it sets the flag;isAppPageStaticEligiblecombines that result with the route's config:force-dynamic,revalidate = 0and the edge runtime make a route dynamic;force-staticanddynamic = "error"make it static;Cache-Control: private, no-cache, no-store, max-age=0, must-revalidate;cacheable: falseat admission;cacheComponents(PPR) routes keep their current behaviour and are out of scope for this stack.The build report and the deploy-time route classification (edge runtime, listed paths) come later in the stack. Until then the Response Store adapter still looks these routes up, finds nothing, and the app never writes them.
Behaviour change
A dynamic-segment App route that relies on ISR now needs
generateStaticParams. Returning[]opts every path into on-demand ISR, as in Next.js. I added it to these routes so they keep caching:cf-app-basicCDN-stage fixtures;ppr-impact-demo'sstatic-to-dynamic/[slug]gets the path its checked-in manifest already lists.Tests
tests/app-segment-config.test.ts: the walk (page, layout at the last dynamic segment, parent-only, slots, BFS order), runtime precedence, and eligibility;tests/app-page-dispatch.test.tsandtests/app-page-render.test.ts: routes that aren't static or SSG are never read or written and get the never-cache header;tests/app-page-method.test.ts: the method guard.packages/cloudflare/tests/response-store-adapter.e2e.test.ts): a new/dynamic-segment/[slug]demo route renders per request, sends the never-cache header for HTML and RSC, and leaves no metadata entry. 21/21 pass locally.tests/e2e/cloudflare-workers/cache-prewarm.spec.ts): the same check against the response-store, workers-cache and KV deployments.