feat(prerender): support layout-level generateStaticParams#735
feat(prerender): support layout-level generateStaticParams#735raed04 wants to merge 1 commit intocloudflare:mainfrom
Conversation
commit: |
…e#567) Layout files at dynamic segments (e.g. app/[category]/layout.tsx) can export generateStaticParams to provide parent params for nested child routes. Previously only page-level exports were scanned, silently skipping layout-only dynamic segments. Changes: - Extend generateStaticParamsMap code-gen to emit layout-level entries by walking each route's layouts[] and layoutTreePositions[] - Remove routeIndex guard in resolveParentParams; check staticParamsMap directly so layout-only prefixes are resolved - Handle null results as pass-through (CF Workers Proxy compatibility) instead of collapsing the param accumulator - Use decodeRouteSegment for static segments to match the authoritative convertSegmentsToRouteParts behavior - Remove dead routeIndex parameter and Map construction - Update entry-templates snapshots (removed stale TODO comments) - Add 10 new resolveParentParams tests covering layout-only parents, mixed page/layout, null pass-through, catch-all parents, error propagation, and three-level nesting Closes cloudflare#567
raed04
force-pushed
the
feat/layout-generate-static-params-567
branch
from
0362287 to
a4e39da
Compare
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: layout-level generateStaticParams
Good feature — this closes a real gap where layout-only dynamic segments were silently skipped during prerendering. The approach of extending generateStaticParamsMap code-gen and simplifying resolveParentParams is sound. A few items to address before merging:
Issues
-
Type unsoundness for null return —
StaticParamsMap(line 249-256 ofprerender.ts) declares the function return type asPromise<Record<string, string | string[]>[]>, which does not includenull. The new null pass-through logic inresolveParentParamsworks at runtime but the tests have to cast withas unknown as ...to work around this. The type should be updated toPromise<Record<string, string | string[]>[] | null>so this contract is explicit. -
Duplicate tests — Two of the new tests are exact duplicates of pre-existing ones (identical mock inputs, identical assertions, only different description strings). See inline comments.
-
Missing fixture coverage — The PR description calls out that
[ ] Verify with a real fixture that has layout-level generateStaticParamsis unchecked. The unit tests forresolveParentParamsare good but they only test the resolver in isolation — the code-gen IIFE inapp-rsc-entry.ts(the part that walksr.layouts/r.layoutTreePositionsto build prefix patterns) has no test coverage. A fixture withapp/[category]/layout.tsxexportinggenerateStaticParams(and no page at that level) would close the loop end-to-end and test the code-gen path.
Minor observations
-
The code-gen IIFE duplicates
convertSegmentsToRoutePartslogic inline. This is noted in comments but worth tracking — ifconvertSegmentsToRoutePartschanges, this copy will silently diverge. -
The snapshot diffs are purely mechanical (removing TODO comments) and look correct.
| }; | ||
| const result = await resolveParentParams(child, staticParamsMap); | ||
| expect(result).toEqual([{ category: "electronics" }, { category: "clothing" }]); | ||
| }); |
There was a problem hiding this comment.
This test is an exact duplicate of "resolves single parent dynamic segment" at line 776. The inputs, function call, and assertions are character-for-character identical — only the description string differs.
Since resolveParentParams no longer takes a routeIndex and doesn't inspect pagePath at all, there's no distinct code path being exercised here. The "layout-level" vs "page-level" distinction only matters in the code-gen (which builds the staticParamsMap), not in the resolver itself.
Consider removing this test or making it genuinely different — e.g., by passing mockRoute("/shop/:category/:item", { pagePath: null }) and verifying the behavior is the same, to document the intent that pagePath no longer matters.
| { b: "2", d: "y" }, | ||
| { b: "2", d: "z" }, | ||
| ]); | ||
| }); |
There was a problem hiding this comment.
Same issue — this is an exact duplicate of "resolves two levels of parent dynamic segments" at line 785. Same mockRoute, same staticParamsMap, same expected output.
| const child = mockRoute("/a/:b/c/:d/:e"); | ||
| const staticParamsMap: StaticParamsMap = { | ||
| // /a/:b has no generateStaticParams — simulates CF Workers Proxy returning null | ||
| "/a/:b": async () => null as unknown as Record<string, string | string[]>[], |
There was a problem hiding this comment.
The as unknown as Record<string, string | string[]>[] cast is a type smell. It's needed because StaticParamsMap's function return type doesn't allow null. Rather than casting in every null-related test, update the StaticParamsMap type definition (line 253) to:
}) => Promise<Record<string, string | string[]>[] | null>)Then these casts become unnecessary and the null pass-through behavior is part of the documented contract.
| for (const parentParams of currentParams) { | ||
| const results = await generateStaticParams({ params: parentParams }); | ||
| if (Array.isArray(results)) { | ||
| if (results === null) { |
There was a problem hiding this comment.
The null check here is correct behavior-wise, but the type system doesn't know generateStaticParams can return null — the StaticParamsMap function signature only allows Promise<Record<string, string | string[]>[]>. This means results === null is technically unreachable according to the types.
Update the return type in the StaticParamsMap type definition (line 253) to include | null so this branch is type-sound.
| // Convert filesystem segments to URL pattern parts (same logic as convertSegmentsToRouteParts) | ||
| const urlParts: string[] = []; | ||
| let hasDynamic = false; | ||
| for (const seg of prefixSegments) { | ||
| // Skip invisible segments | ||
| if (seg === "." || (seg.startsWith("(") && seg.endsWith(")")) || seg.startsWith("@")) | ||
| continue; | ||
| // Catch-all | ||
| const catchAll = seg.match(/^\[\.\.\.([\w-]+)\]$/); | ||
| if (catchAll) { | ||
| urlParts.push(`:${catchAll[1]}+`); | ||
| hasDynamic = true; | ||
| continue; | ||
| } | ||
| // Optional catch-all | ||
| const optCatchAll = seg.match(/^\[\[\.\.\.([\w-]+)\]\]$/); | ||
| if (optCatchAll) { | ||
| urlParts.push(`:${optCatchAll[1]}*`); | ||
| hasDynamic = true; | ||
| continue; | ||
| } | ||
| // Dynamic segment | ||
| const dyn = seg.match(/^\[([\w-]+)\]$/); | ||
| if (dyn) { | ||
| urlParts.push(`:${dyn[1]}`); | ||
| hasDynamic = true; | ||
| continue; | ||
| } | ||
| // Static segment — decode percent-encoded chars to match convertSegmentsToRouteParts | ||
| urlParts.push(decodeRouteSegment(seg)); |
There was a problem hiding this comment.
This block replicates the segment-to-URL-pattern conversion logic from convertSegmentsToRouteParts() in routing/app-router.ts. The comment acknowledges the duplication, which is good. Worth tracking as tech debt — if convertSegmentsToRouteParts gains handling for new segment types (e.g., intercepting route conventions), this copy won't pick it up.
Longer-term, consider extracting the shared segment→URL-part conversion into a reusable helper in routing/utils.ts. Not a blocker for this PR.
|
|
||
| // 2. Layout-level entries (layouts at dynamic segments that may export generateStaticParams) | ||
| for (const r of routes) { | ||
| if (!r.isDynamic) continue; |
There was a problem hiding this comment.
Nit: since the for loop is a no-op when r.layouts.length === 0, you could fold it into the guard:
| if (!r.isDynamic) continue; | |
| if (!r.isDynamic || r.layouts.length === 0) continue; |
Makes the intent clearer — skip routes that have no layouts to scan.
|
Review submitted successfully. Here's a summary of what I posted: 3 issues identified:
2 minor observations:
|
2 participants
Summary
Closes #567
Layout files at dynamic segments (e.g.
app/[category]/layout.tsx) can exportgenerateStaticParamsto provide parent params for nested child routes. Previously only page-level exports were scanned, silently skipping layout-only dynamic segments.generateStaticParamsMapcode-gen to emit layout-level entries by walking each route'slayouts[]andlayoutTreePositions[], with deduplication and page-entry priorityrouteIndexguard inresolveParentParams— checkstaticParamsMapdirectly so layout-only prefixes are resolved[]decodeRouteSegmentfor static segments to matchconvertSegmentsToRoutePartsbehaviorrouteIndexparameter and Map construction fromprerenderAppresolveParentParamstests covering layout-only parents, mixed page/layout, null pass-through, catch-all parents, error propagation, and three-level nestingChanged files
packages/vinext/src/entries/app-rsc-entry.tsgenerateStaticParamsMap: page entries then layout entriespackages/vinext/src/build/prerender.tsresolveParentParamssimplified signature, null pass-through fixtests/__snapshots__/entry-templates.test.ts.snaptests/prerender.test.tsrouteIndexFromhelperTest plan
resolveParentParamsunit tests passentry-templatessnapshot tests passvp checkpasses on all changed files (lint + types + format)generateStaticParams