feat(cloudflare): share one Workers Cache entry across a static App page's queries - #3465
Conversation
|
/bigbonk review for issues Please review exact head |
@vinext/cloudflare
create-vinext-app
@vinext/types
vinext
@cloudflare/workers-response-store
commit: |
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
dba4410 to
bced73b
Compare
|
/bigbonk review for issues Please review exact head |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
| new URL(rscPath, dispatchRequest.url).toString(), | ||
| ); | ||
| canonicalRsc = { headers, navigation: props.renderMode === "navigation" }; | ||
| dispatchRequest = cloneRequestWithHeaders(dispatchRequest, headers); |
There was a problem hiding this comment.
[P1] Preserve the full navigation params above the shared cache
The new header recomposition later in this dispatch uses props.params, which contains only the main route match. The response-stage render builds X-Vinext-Params from navigationParams, including params contributed by active parallel slots, but withoutRequestScopedHeaders() now removes that value before cache admission. Both MISS and HIT navigation responses therefore lose slot-only keys in useParams(). Please carry or recompute the full navigation params in this stage state before setting that header.
There was a problem hiding this comment.
This one is outside this PR. The request stage has rebuilt X-Vinext-Params from props.params since 2978637 on main (2026-09-07), overwriting the response stage's navigation params on every split-stage App RSC response, MISS or HIT. Removing the header before admission doesn't change what the client receives. Carrying navigation params through the stage state would change that transport contract, so I've noted it as a follow-up.
| function isAppPageRouteStaticEligible(route: AppRoute): boolean { | ||
| const readSegmentConfig = (filePath: string | null | undefined) => { | ||
| if (!filePath) return null; | ||
| const code = fs.readFileSync(filePath, "utf8"); |
There was a problem hiding this comment.
[P1] Classify MDX from parseable module metadata
This reads raw MDX and sends it through the TS/TSX parser used by the export helpers. Once the file contains Markdown, parsing fails and a real generateStaticParams export is missed. For example, the existing app/[slug]/page.mdx fixture with generateStaticParams followed by # Hello discovers /hello at runtime, but this code marks it unlisted, so the probe omits it from the final cacheability manifest and it is neither warmed nor query-normalized. Please use transformed/runtime metadata or an MDX-aware extraction path.
There was a problem hiding this comment.
Fixed in d6f0565. The segment config of .mdx pages is now read from their MDX ESM blocks only, so fenced code no longer counts.
| const runtime = extractExportConstString(code, "runtime"); | ||
| return { | ||
| ...(dynamic === null ? {} : { dynamic }), | ||
| ...(hasNamedExport(code, "generateStaticParams") ? { generateStaticParams() {} } : {}), |
There was a problem hiding this comment.
[P2] Ignore type-only generateStaticParams exports
hasNamedExport() intentionally treats export type { generateStaticParams } as a match, while the runtime segment helpers require a function value. This can classify a route as SSG even though its loaded module has no generator, causing paths discovered through shared layout/sibling static-param sources to be treated as listed and turning an otherwise droppable render failure into a deploy failure. Use a value-export-aware check here.
There was a problem hiding this comment.
Fixed in 0b73fa8. Discovery uses the runtime-export check that Route Handler classification uses, so type-only exports are ignored.
| return "done"; | ||
| } | ||
| if ( | ||
| group.pattern.route.kind === "app-page" && |
There was a problem hiding this comment.
[P1] Resolve the route before dropping an unlisted App-page failure
This checks the original grouped route before the resolvedRouteChanged handling below. If an unlisted App-page source is rewritten to a Pages page or Route Handler that returns 500, the probe envelope carries that resolved kind/pattern, but this branch silently drops the failure and the deploy succeeds. Restrict the exception to a resolved App page and evaluate listing after moving/validating the resolved route.
There was a problem hiding this comment.
Fixed in 50d608a. The unlisted 5xx drop now requires an App page result for the group's own route, or for a route the request stage resolved. Listing is checked at that resolved route.
| } finally { | ||
| if (fs.existsSync(pendingManifestPath)) fs.unlinkSync(pendingManifestPath); | ||
| const projectionPath = path.join(serverDirectory, CACHEABILITY_REQUEST_PROJECTION_MODULE); | ||
| const hasProjectionModule = |
There was a problem hiding this comment.
[P2] Verify that the Worker graph imports the projection
The build plugin emits this asset for every App build even when no runtime module imports it, so file existence does not establish that the request stage can read the projection. A stale or mismatched artifact can pass this check, get overwritten successfully, and still serve full-query dispatches while the response stage admits query-free identities. Require static reachability from the Worker entry, analogous to assertManifestModuleReachable().
There was a problem hiding this comment.
Fixed in 9513136. The writer now requires the projection to exist and to be statically reachable from the Wrangler main module. This exposed a real bug: the request-stage chunks were emitted under _next/static/, so their projection import pointed at a file that doesn't exist. That is fixed in 77154b9.
|
Submitted 5 actionable inline findings on exact head |
bced73b to
9513136
Compare
|
/bigbonk review for issues Please review exact head |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues Please review exact head |
1 similar comment
|
/bigbonk review for issues Please review exact head |
| let fence: string | null = null; | ||
| for (const line of source.split(/\r?\n/)) { | ||
| if (block) { | ||
| if (line.trim() === "") { |
There was a problem hiding this comment.
[P1] Continue valid MDX ESM across internal blank lines
MDX only ends an ESM block at a blank line once the accumulated JavaScript parses; blank lines inside an incomplete export are valid (for example export function generateStaticParams() {\n\n return [{ slug: "hello" }]\n}). This scanner instead closes the block at the first blank line, so extractMdxEsm() returns only the function signature and hasRuntimeExportedName() misses the generator. Discovery still receives /hello from the runtime endpoint, but marks it unlisted and the probe consequently omits it from warming/query normalization. Please scan through blank lines until the ESM construct is syntactically complete, or use the MDX parser's ESM nodes.
There was a problem hiding this comment.
Fixed in cf9ffd1. An ESM block now ends at a blank line only once the accumulated code parses. This uses the oxc parseSync that report.ts already uses, as MDX itself decides.
|
Submitted 1 actionable inline finding on exact head |
9513136 to
cf9ffd1
Compare
|
/bigbonk review for issues Please review exact head |
|
|
/bigbonk review for issues Please review exact head |
| const runtimePathSet = new Set<string>(); | ||
| for (const group of pattern.groups) { | ||
| if (group.dropped) continue; | ||
| if (!group.deferred && hasNoState(group)) continue; |
There was a problem hiding this comment.
[P2] Preserve loading-shell siblings when dynamic usage removes the full page
hasNoState(group) is true whenever the HTML probe reports dynamicUsage, and this continue drops every target in the group before the paired-representation handling runs. For a listed route with loading.tsx and a dynamic API such as headers() only below that boundary, the full HTML/RSC page is correctly non-cacheable but the loading shell is still deterministic and should remain warmable. The existing keeps a loading-shell warm candidate when the full page is dynamic test misses this because its envelope omits the newly added dynamicUsage: true. Please retain the rsc-loading-shell sibling (with a representation-specific manifest entry) when excluding the full page.
There was a problem hiding this comment.
Fixed in 568be55 + 5713c50. A new manifest field, runtimeRepresentationPaths, runtime-checks a single representation at exact paths. A path whose full page is excluded for dynamicUsage now keeps its rsc-loading-shell warm, if the path is listed or the route has on-demand ISR. HTML and RSC-full still get no state. Tests are in the probe and manifest suites.
| fence = fenceMarker; | ||
| continue; | ||
| } | ||
| if (/^(?:import|export)\s/.test(line)) block = [line]; |
There was a problem hiding this comment.
[P2] Respect MDX flow boundaries before treating a line as ESM
This starts an ESM block for any unindented export outside a fence, but MDX explicitly does not recognize import/export constructs that interrupt a paragraph (the upstream micromark test uses a\nexport default c as paragraph text). A dynamic MDX page can therefore contain displayed text such as export function generateStaticParams() {} and be classified here as SSG/listed even though its compiled module has no such export. If an ancestor generator discovers the concrete path, a render failure is then treated as a listed build failure instead of an unlisted path that may be dropped. Please derive ESM nodes from an MDX parser or track the Markdown flow boundaries, not just line prefixes.
There was a problem hiding this comment.
Fixed in 3e55c9b: discovery now reads an MDX page's ESM with @mdx-js/mdx, resolved through the @mdx-js/rollup that vinext injects, instead of the line scanner. Paragraph text that looks like an export is ignored, and fenced code or blank lines inside a function are handled. If the parser is missing or a source doesn't parse, that route isn't listed.
|
Submitted 2 actionable inline findings on exact head |
cf9ffd1 to
3e55c9b
Compare
|
/bigbonk review for issues Please review exact head |
| pattern: pattern.route.pattern, | ||
| state: "runtime-check", | ||
| ...(hasStaticFallback || allObservedPathsStaticallyGenerated | ||
| ...(hasStaticFallback || allowsUnknown |
There was a problem hiding this comment.
[P1] Exclude observed dynamic paths from the static fallback
When hasStaticFallback is true, a group whose probe reports dynamicUsage is skipped above without entering runtimePathSet, but this allowUnknown then makes that same observed pathname resolve to static-candidate. For example, a generateStaticParams() => [] route reached through a traffic-picked /posts/one that calls headers() produces a manifest where /posts/one falls through to the unknown fallback; the request stage strips its query and completed admission raises the static-to-dynamic 500, despite the probe explicitly classifying that path as dynamic/no-state. Preserve an exact override/exclusion for skipped dynamic paths before enabling the unknown static fallback.
There was a problem hiding this comment.
Declining. The current behaviour is Next.js parity. In Next.js 16.2.7's app-page template, isSSG is true for any path that matches a dynamicRoutes entry, which includes a generateStaticParams() => [] fallback. When such a path renders with revalidate === 0 (e.g. it calls headers()), Next.js throws "Page changed from static to dynamic at runtime" and returns 500. A traffic-picked /posts/one is an unknown path there too, so it gets the same 500. The test gives unknown paths on-demand ISR when no listed path used a dynamic API asserts this on purpose. Runtime-checking only the probed paths would also make the status depend on which paths traffic picked: /posts/one would get a 200 while an identical /posts/two still gets a 500. Nothing is stored in either case.
|
Submitted 1 actionable inline finding on exact head |
3e55c9b to
433c64f
Compare
…pp page manifest A stale or pre-built artifact without the projection module silently skipped the projection, so it deployed a manifest with RSC listings whose request stage never drops the query. Fail the deploy instead, as a missing manifest module does.
…nder Workers Cache
…tatic-candidate probe paths
…ery, for HTML and RSC
…der tree Follows the loader-tree runtime merge below this PR, which needs the route's segments and no longer takes a materializing-slot flag.
Discovery parsed raw MDX with the TypeScript parser, which fails once the file has Markdown, so a real generateStaticParams export was missed and the route's discovered paths were marked unlisted. Keep only the MDX ESM blocks, as MDX delimits them, before reading exports.
…covery
The eligibility read counted `export type { generateStaticParams }` as a
generator, while the runtime helpers need a function value, so a route
could be classified SSG and its discovered paths listed. Check runtime
value exports only, as Route Handler classification does.
… request resolved The unlisted App page 5xx drop read the grouped route before route resolution, so an unlisted App page source rewritten to a Pages page or Route Handler that failed was dropped and the deploy succeeded. Drop only a failure the envelope resolves to an App page, through an allowed move, and judge its listing under that resolved route and pathname.
The request stage imports the deploy-filled projection through a `./__vinext_cacheability_request_projection.js` external. Its chunks fell through to the assets directory, so that specifier resolved to a module the build never emits.
… projection A manifest with App page routes is only safe to publish when the request stage reads the projection filled beside it. Check the projection's static reachability from the Wrangler main module, as for the manifest itself.
MDX only ends an ESM block at a blank line once the accumulated JavaScript parses. Ending it at the first blank line cut a multi-line generateStaticParams down to its signature, so discovery missed it.
… manifest paths A route record could only runtime-check one representation for every path of its pattern. runtimeRepresentationPaths lists the exact paths at which a single representation is runtime-checked, beside the route's other lists.
…des the full page A dynamic API below loading.tsx leaves the loading shell deterministic. Dropping every target of a dynamic-usage path also dropped that shell, so keep it warmable and runtime-checked at its path.
Line prefixes took paragraph text such as "export function
generateStaticParams() {}" as ESM, which MDX doesn't. Read the mdxjsEsm
nodes from the @mdx-js/mdx parser that @mdx-js/rollup compiles with, and
never list an MDX route's paths when that parser isn't installed.
Discovery resolved @mdx-js/mdx only through vinext's optional @mdx-js/rollup peer, so an app building MDX with its own plugin had every MDX route unlisted. Also resolve it through the app's @mdx-js/rollup and from the app root. An MDX route still unreadable is taken as static: the built runtime reads its real exports and never stores a route that isn't static, while an unlisted path's render failure was dropped instead of failing the deploy.
An MDX route read without the MDX parser stays listed, so its paths' render failures still fail the deploy. Its exports are unknown, though, and no probe renders a fallback pattern, so an empty generateStaticParams result no longer certifies the route as a static-candidate fallback.
…probes move paths A moved failure was judged listed by scanning the destination pattern's groups, but a route-moving probe removes its group from that pattern when it completes. A listed path moved elsewhere before an alias resolving to it failed silently dropped that failure. Record every origin-listed route and pathname once, before any probe runs, and judge listing against that set.
…ilure is dropped An unlisted App page failure was dropped before its paired representations were split back to the original route, so header-sensitive routing that sent only the HTML probe to a failing page also dropped an RSC request that stays on a valid source route. Defer the paired targets to the original route first, as a moved success does, and drop only the failed primary.
A static HTML render doesn't prove the loading-shell render static: a static page may never render its loading boundary, while that boundary can read a dynamic API when the loading-shell request renders it. Certifying the shell from the HTML probe turned such a render into a static-to-dynamic 500 at admission. Certify only html and rsc-full from the HTML probe; the shell stays runtime-checked, so its completed render decides admission. Drop the loading-boundary route list discovery passed only for that.
…ge route's own path
…eries in Workers Cache Next.js prerenders a static page whose useSearchParams() sits inside Suspense once, with the fallback, and serves it for every query. The deployed Workers Cache test now requests /search-params/suspense with fresh queries until one HITs with the previous response's render ID, and requires that no query sent reaches either body. A unit case renders that page shape as the deploy probe does, in cache-candidate mode with the SSR useSearchParams() gate, and requires a static-candidate, renderer-static envelope without dynamic usage, so the probe certifies it and the request stage drops its query.
69c9734 to
7801a78
Compare
|
/bigbonk review for issues Please review exact head |
|
LGTM! |
1 participant
Stacked on #3463. Plan PR 6 (Workers Cache).
Change
Workers Cache now shares one entry across the queries of a static App page, as Next.js serves one prerendered entry whatever the query.
Request stage. A Workers Cache app-page dispatch drops its query, keeping a validated
_rsc, when all of these hold:static-candidate, using the same lookup admission uses (cacheabilityManifestPageState);cache === "shared";The canonical RSC and loading-shell URLs are built from the stripped URL, and per-request headers are still composed from the original URL. Nonce requests and
/_next/datarequests keep their query.Response stage. Request-scoped RSC headers are removed from shared app-page RSC responses before admission. The request stage recomposes them on every response.
Deploy.
runtime-checkin this PR; fix(cloudflare): certify a static App page's loading shell from its HTML probe #3490 certifies it where the route has a loading boundary.Unchanged. Response Store and KV get no manifest or deploy step. Pages Router pages and Route Handlers keep their classification.
Tests
Unit tests cover:
/_next/dataand canonical URLs;Each test fails without its change. 1041 pass across the targeted files.
Deployed e2e:
tests/e2e/cloudflare-workers/cache-prewarm.spec.ts(Workers Cache backend) requests/cached/featured?q=<new uuid>. It expects a HIT with the previous render id and no canary in the body, for HTML and for the canonical RSC request.Known gaps
runtime,dynamicandrevalidatefrom source, so a re-exported or computed value is missed there. The probe still catches it at deploy.generateStaticParams. fix(app-router): take a route's parent params from its layouts, not a sibling page #3493 (top of the stack) fixes it.Varyhandles them.