feat(build): wire build-time layout classification into RSC entry#842
Conversation
commit: |
NathanDrake2406
force-pushed
the
feat/pr-768-5-build-classification-wire
branch
from
dc802fc to
8a427d8
Compare
… RSC entry Introduce a Rollup generateBundle hook that patches the __VINEXT_CLASS stub in the generated RSC entry with a real dispatch table built from Layer 1 segment-config analysis and Layer 2 module-graph classification. The runtime probe loop in app-page-execution.ts consults this table and skips the dynamic-isolation probe for layouts we proved static or dynamic at build time. Add route-classification-manifest.ts as the codegen glue between the classifier and the entry template, and flow buildTimeClassifications through renderAppPageLifecycle so the runtime probe can honor the build-time decision. Fail loudly if generateBundle sees __VINEXT_CLASS referenced without the recognized stub body, so generator and plugin cannot silently drift.
NathanDrake2406
force-pushed
the
feat/pr-768-5-build-classification-wire
branch
from
8a427d8 to
ac081b1
Compare
NathanDrake2406
changed the title
There was a problem hiding this comment.
Pull request overview
Introduces build-time layout classification wiring for the App Router RSC entry so the runtime layout probe loop can skip per-layout dynamic isolation when a layout is already proven static/dynamic during the build.
Changes:
- Add a
__VINEXT_CLASS(routeIdx)stub to the generated RSC entry and thread per-routebuildTimeClassificationsinto the runtime classification block. - Implement a build-time classification manifest + codegen helpers to produce a per-route dispatch table (Layer 1 segment-config + Layer 2 module-graph “static” proofs).
- Extend layout-classification plumbing to carry tagged results and structured reasons, with broad test coverage and updated snapshots.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/route-classification-manifest.test.ts | New unit tests for manifest collection + codegen replacement helpers. |
| tests/layout-classification.test.ts | Update tests for module-graph classifier to return structured results (incl. first shim match). |
| tests/build-time-classification-integration.test.ts | New integration test that builds a fixture and asserts the emitted __VINEXT_CLASS is patched/populated. |
| tests/build-report.test.ts | Update segment-config classifier tests for tagged LayoutBuildClassification + reasons. |
| tests/app-router.test.ts | Assert the generated RSC entry contains the build-time dispatch stub and per-route routeIdx wiring. |
| tests/snapshots/entry-templates.test.ts.snap | Snapshot updates for the new stub + route fields + classification threading. |
| packages/vinext/src/index.ts | Collect Layer 1 manifest during RSC entry load; patch __VINEXT_CLASS in generateBundle using module graph analysis. |
| packages/vinext/src/entries/app-rsc-entry.ts | Emit __VINEXT_CLASS stub, add routeIdx + __buildTimeClassifications, and pass buildTimeClassifications into runtime classification. |
| packages/vinext/src/build/route-classification-manifest.ts | New manifest bridge + codegen helpers to build the dispatch replacement (and reasons dispatcher). |
| packages/vinext/src/build/report.ts | Change classifyLayoutSegmentConfig to return tagged classification with structured reason (or { kind: "absent" }). |
| packages/vinext/src/build/layout-classification.ts | Return structured module-graph results and propagate tagged classifications + reasons. |
| packages/vinext/src/build/layout-classification-types.ts | New shared discriminated-union types for build-time classification and reasons. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const canonicalize = (p: string): string => { | ||
| try { | ||
| return fs.realpathSync.native(p); | ||
| } catch { | ||
| return p; | ||
| } | ||
| }; |
There was a problem hiding this comment.
This generateBundle hook introduces a local canonicalize() wrapper around fs.realpathSync.native, but the repo already has a tryRealpathSync() helper (src/build/ssr-manifest.ts) used for path canonicalization. Reusing the shared helper (tryRealpathSync(p) ?? p) would reduce duplication and keep realpath handling consistent across the codebase.
| const canonicalize = (p: string): string => { | |
| try { | |
| return fs.realpathSync.native(p); | |
| } catch { | |
| return p; | |
| } | |
| }; | |
| const canonicalize = (p: string): string => tryRealpathSync(p) ?? p; |
| for (const route of routes) { | ||
| const layer1 = new Map<number, Layer1Class>(); | ||
| const layer1Reasons = new Map<number, ClassificationReason>(); | ||
|
|
||
| for (let layoutIndex = 0; layoutIndex < route.layouts.length; layoutIndex++) { | ||
| const layoutPath = route.layouts[layoutIndex]!; | ||
| let source: string; | ||
| try { | ||
| source = fs.readFileSync(layoutPath, "utf8"); | ||
| } catch (cause) { | ||
| throw new Error( | ||
| `vinext: failed to read layout for route ${route.pattern} at ${layoutPath}`, | ||
| { cause }, | ||
| ); | ||
| } | ||
| const result = classifyLayoutSegmentConfig(source); | ||
| if (result.kind === "static" || result.kind === "dynamic") { | ||
| layer1.set(layoutIndex, result.kind); | ||
| layer1Reasons.set(layoutIndex, result.reason); | ||
| } | ||
| } |
There was a problem hiding this comment.
collectRouteClassificationManifest reads each layout file once per route; shared layouts (especially the root layout) will be re-read many times in large apps, adding avoidable synchronous I/O during the build. Consider memoizing by layoutPath (e.g., Map<path, source> or Map<path, classification>) so each unique layout file is read/classified only once across all routes.
| const layer2PerRoute = new Map<number, Map<number, "static">>(); | ||
| for (let routeIdx = 0; routeIdx < rscClassificationManifest.routes.length; routeIdx++) { | ||
| const route = rscClassificationManifest.routes[routeIdx]!; | ||
| const perRoute = new Map<number, "static">(); | ||
| for (let layoutIdx = 0; layoutIdx < route.layoutPaths.length; layoutIdx++) { | ||
| // Skip layouts already decided by Layer 1 — segment config is | ||
| // authoritative, so there is no need to walk the module graph. | ||
| if (route.layer1.has(layoutIdx)) continue; | ||
| const layoutModuleId = canonicalize(route.layoutPaths[layoutIdx]!); | ||
| // If the layout module itself is not in the graph, we have no | ||
| // evidence either way — do NOT claim it static, or we would skip | ||
| // the runtime probe for a layout we never actually analysed. | ||
| // `classifyLayoutByModuleGraph` returns "static" for an empty | ||
| // traversal, so the seed presence check has to happen here. | ||
| if (!moduleInfo.getModuleInfo(layoutModuleId)) continue; | ||
| const graphResult = classifyLayoutByModuleGraph( | ||
| layoutModuleId, | ||
| dynamicShimPaths, | ||
| moduleInfo, | ||
| ); | ||
| if (graphResult.result === "static") { | ||
| perRoute.set(layoutIdx, "static"); | ||
| } | ||
| } |
There was a problem hiding this comment.
In generateBundle, Layer 2 BFS classification is performed per (routeIdx, layoutIdx) even when many routes share the same layout module (commonly the root layout). This can lead to O(routes × shared-layouts) repeated module-graph traversals. Consider caching classifyLayoutByModuleGraph results by canonicalized layoutModuleId and reusing them when populating perRoute maps.
Three targeted cleanups from code review: - Replace inline canonicalize() with tryRealpathSync(p) ?? p, which is already imported and does the same thing (realpathSync.native + fallback) - Memoize readFileSync calls in collectRouteClassificationManifest so shared layouts (especially the root layout) are read from disk only once per build rather than once per route - Cache classifyLayoutByModuleGraph results by canonicalized module ID in generateBundle so shared layouts are BFS-traversed only once per build rather than once per (routeIdx, layoutIdx) pair
|
/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: Build-time layout classification wiring
This is well-structured infrastructure work. The layered classification design (segment-config → module-graph → runtime probe) is clean, the priority semantics are correct, and the generateBundle hook's defensive error handling is a real strength. The test coverage is thorough — both the unit tests and the full-pipeline integration test.
A few issues worth addressing:
Key concerns
-
buildReasonsReplacementis dead code in this PR. It's exported, tested, and documented, but never called fromindex.ts. ThegenerateBundlehook only callsbuildGenerateBundleReplacement. The PR description saysClassificationReasonis consumed by a debug sidecar follow-up (#843), which is fine, but the function itself should either be called behind theVINEXT_DEBUG_CLASSIFICATIONenv check in this PR or deferred entirely to #843. Shipping tested-but-uncalled code creates maintenance risk — someone will see the tests passing and assume it's wired up. -
Minor: unused destructured
_valuein merge function. Thefor (const [layoutIdx, _value] of layer2)destructuring is an unnecessary allocation. Sincelayer2is typed asReadonlyMap<number, "static">, the value is always"static"and never read — but the underscore-prefixed binding still participates in iteration.for (const layoutIdx of layer2.keys())would be clearer. -
The
__VINEXT_CLASSstub regex extraction in integration tests is fragile. The regex/(\([\s\S]*?\))\(routeIdx\)/uses a non-greedy match for the parenthesized function body. If the generated switch statement itself contains)(routeIdx)as a substring (unlikely today, but possible if the codegen changes), this regex would break. Consider anchoring the regex more specifically or adding a comment noting this coupling.
Architecture observations
-
The Copilot reviewer's point about memoizing
classifyLayoutByModuleGraphresults is already addressed in this PR viagraphCachein thegenerateBundlehook (line 1742 ofindex.ts). That comment appears to have missed the cache. ThesourceCacheincollectRouteClassificationManifestsimilarly deduplicates Layer 1 file reads. Both caches are correctly scoped. -
The
canonicalizehelper reusestryRealpathSyncfromssr-manifest.ts(already imported inindex.ts), which is the right call. The Copilot suggestion to refactor was misleading — it's already using the shared helper. -
The stub-body drift detection (throwing when
__VINEXT_CLASSis referenced but the stub body isn't found) is a good pattern. It catches a real failure mode: upstream codegen changes that reshape the function without updating thegenerateBundleregex. -
The runtime consumer in
app-page-execution.tscorrectly short-circuits on build-time results and still probes for special errors (redirects, not-found). This means build-time classification skips only the dynamic isolation overhead, not error detection. That's the right trade-off.
Nits
- The snapshot diff is ~1000 lines of mechanical repetition (same stub block × 7 fixtures). Not actionable here, but snapshot-heavy tests for generated code are a maintenance burden worth considering alternatives for eventually.
| const merged = new Map<number, MergedLayoutClassification>(); | ||
|
|
||
| if (layer2) { | ||
| for (const [layoutIdx, _value] of layer2) { |
There was a problem hiding this comment.
The _value is never read and always "static" by type. Using layer2.keys() is cleaner:
| for (const [layoutIdx, _value] of layer2) { | |
| for (const layoutIdx of layer2.keys()) { |
| " }", | ||
| " }", | ||
| ].join("\n"); | ||
| } |
There was a problem hiding this comment.
buildReasonsReplacement is exported and fully tested but never called from the plugin. The generateBundle hook in index.ts only calls buildGenerateBundleReplacement. The docstring says it's gated behind VINEXT_DEBUG_CLASSIFICATION, but this PR doesn't implement that gate.
This is dead code that creates a false sense of completeness. Either:
- Wire it up behind the env check in this PR (even if the debug sidecar UI comes in chore(debug): classification reasons sidecar behind VINEXT_DEBUG_CLASSIFICATION [6/6] #843), or
- Move it entirely to chore(debug): classification reasons sidecar behind VINEXT_DEBUG_CLASSIFICATION [6/6] #843 so the exported surface matches what's actually used.
Leaving it tested-but-uncalled means a future refactor of the merge logic could break the reasons codepath without anyone noticing in production — the tests would still pass but the feature would be silently absent.
There was a problem hiding this comment.
Intentional. buildReasonsReplacement ships here because #843 imports it from this module — the reasons-dispatch codegen is infrastructure shared between the two PRs. Keeping the function and its tests here means #843 does not need to re-add a dependency on untested code. The wiring (stub injection + generateBundle patch behind VINEXT_DEBUG_CLASSIFICATION) lives in #843 where the full debug surface is assembled.
| if (this.environment?.name !== "rsc") return; | ||
| if (!rscClassificationManifest) return; | ||
|
|
||
| const stubRe = /function __VINEXT_CLASS\(routeIdx\)\s*\{\s*return null;?\s*\}/; |
There was a problem hiding this comment.
The return null;? optional semicolon in the regex is a good defensive choice for Rolldown's output variance, but the stub body emitted from app-rsc-entry.ts (line 753) always includes the semicolon. Consider adding a brief comment noting this is intentional tolerance for bundler output variation, so future readers don't think it's accidental.
| ); | ||
| return ` { | ||
| routeIdx: ${routeIdx}, | ||
| __buildTimeClassifications: __VINEXT_CLASS(${routeIdx}), |
There was a problem hiding this comment.
The __buildTimeClassifications property is eagerly evaluated at module load time (not at request time) because __VINEXT_CLASS(routeIdx) is called during the routes = [...] array construction. This is correct and intentional — the dispatch table is static data. But the double-underscore prefix + the fact that it's a call expression makes it look like it might be deferred.
Minor: consider a brief inline comment like // evaluated once at module load so readers don't have to trace to the stub definition to understand the evaluation semantics.
| } | ||
|
|
||
| const re = | ||
| /function\s+__VINEXT_CLASS\s*\(routeIdx\)\s*\{\s*return\s+(\([\s\S]*?\))\(routeIdx\);\s*\}/; |
There was a problem hiding this comment.
This non-greedy [\s\S]*? will match the shortest string between ( and ). If the inner switch statement contains a closing ) followed by (routeIdx) (e.g., from a future codegen change that wraps an IIFE inside the body), this regex would capture too little.
Not a blocking issue since the codegen is controlled, but worth a comment noting the coupling:
| /function\s+__VINEXT_CLASS\s*\(routeIdx\)\s*\{\s*return\s+(\([\s\S]*?\))\(routeIdx\);\s*\}/; | |
| /function\s+__VINEXT_CLASS\s*\(routeIdx\)\s*\{\s*return\s+(\([\s\S]*?\))\(routeIdx\);\s*\}/; | |
| // Note: non-greedy match assumes the inner dispatch function body does not | |
| // contain ')(routeIdx)' as a substring. Coupled to the codegen in | |
| // route-classification-manifest.ts. |
| // the runtime probe for a layout we never actually analysed. | ||
| // `classifyLayoutByModuleGraph` returns "static" for an empty | ||
| // traversal, so the seed presence check has to happen here. | ||
| if (!moduleInfo.getModuleInfo(layoutModuleId)) continue; |
There was a problem hiding this comment.
Good catch on the seed presence check. classifyLayoutByModuleGraph returns { result: "static" } for an empty BFS (no module info), which would falsely mark unknown layouts as static. This guard is essential. The comment explains it well.
|
�[33m[STARTED]�[39m Backing up original state... �[0;31m�[0;31m✖�[0m vp check --fix:�[0m Linting failed before analysis started |
|
@james-elicx Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
- Use layer2.keys() instead of destructuring with unused _value - Add comment to stub regex explaining intentional semicolon tolerance - Add comment to __buildTimeClassifications clarifying module-load evaluation - Add coupling comment to integration test regex explaining non-greedy assumption
|
/bigbonk review |
There was a problem hiding this comment.
Review
This is solid infrastructure work. The layered classification pipeline (segment-config → module-graph → runtime probe) is well-factored, the type design is clean, and the defensive generateBundle error handling for stub drift is a real strength. The integration test that exercises the full Vite build pipeline is especially good — it catches the class of silent regression (stub never patched → Layer 3 fallback) that originally motivated the PR.
A few items worth addressing:
1. rscClassificationManifest is never reset between builds
The manifest is captured during the load hook and consumed in generateBundle, but it's never cleared afterward. In a long-lived dev server that rebuilds the RSC entry (e.g., on HMR when a new route is added), the manifest from a previous build could leak into a subsequent one if the load hook isn't re-triggered for the RSC entry. In practice this likely doesn't matter today because the stub is only patched in production builds, but it's a latent footgun if generateBundle ever runs in dev-like contexts.
2. Stub regex doesn't account for bundler renaming
The stubRe pattern matches __VINEXT_CLASS as a literal function name. If Rolldown ever applies scope-hoisting or function renaming in the RSC environment (even without full minification), the regex won't match and the hook will throw the drift error. This is fine as long as the RSC entry is always the entry chunk (entry chunk bindings aren't renamed), but it's worth a comment noting that assumption explicitly.
3. Layer 2 "static" type in layer2PerRoute is slightly misleading
The map type is Map<number, Map<number, "static">>, but the value is always the literal "static" — it's effectively a Set. Using Map<number, Set<number>> and checking membership would be semantically clearer and avoid the question of what happens if someone adds "dynamic" as a Layer 2 value.
Minor
-
The
serializeReasonExpressionfunction handles the"runtime-probe"and"no-classifier"variants ofClassificationReason, but these are never produced by the build-time pipeline — they come from the runtime. No bug, since the function is correct for all variants, but it means the codegen is carrying dead branches for the debug sidecar that don't have any build-time producer yet. -
The integration test
afterAllis intentionally a no-op (leaves tmpdirs for debugging). This is fine for CI but worth noting if anyone runs the suite locally in a loop — it'll accumulate temp directories.
Overall this is well-structured and ready to land after the comments below are considered. The caching strategy (sourceCache for Layer 1, graphCache for Layer 2) addresses the performance concerns from the earlier review.
|
|
||
| // The `?` after the semicolon is intentional: Rolldown may or may not | ||
| // emit the trailing semicolon depending on minification settings. | ||
| const stubRe = /function __VINEXT_CLASS\(routeIdx\)\s*\{\s*return null;?\s*\}/; |
There was a problem hiding this comment.
The ? after the semicolon is a good idea for Rolldown output variance, but the regex also assumes __VINEXT_CLASS is never renamed by the bundler. Since this is an entry chunk binding, Rolldown won't rename it under normal settings. Worth a brief comment noting that assumption so future readers know it's intentional:
| const stubRe = /function __VINEXT_CLASS\(routeIdx\)\s*\{\s*return null;?\s*\}/; | |
| // The `?` after the semicolon is intentional: Rolldown may or may not | |
| // emit the trailing semicolon depending on minification settings. | |
| // This regex relies on `__VINEXT_CLASS` retaining its name, which holds | |
| // because RSC entry chunk bindings are not subject to scope-hoisting renames. | |
| const stubRe = /function __VINEXT_CLASS\(routeIdx\)\s*\{\s*return null;?\s*\}/; |
| // Collect Layer 1 (segment config) classifications for all layouts. | ||
| // Layer 2 (module graph) runs later in generateBundle once Rollup's | ||
| // module info is available. | ||
| rscClassificationManifest = collectRouteClassificationManifest(routes); |
There was a problem hiding this comment.
The manifest is captured here and consumed in generateBundle, but never cleared. If a rebuild of the RSC entry occurs without re-triggering this load hook (e.g., a hot-reload path that reuses the previous module), generateBundle would consume a stale manifest.
In practice this is likely safe because generateBundle only runs in production builds and the stub check gates everything. But a comment or a defensive rscClassificationManifest = null at the top of generateBundle (after the early returns) would make the lifecycle contract explicit.
| } | ||
| } | ||
|
|
||
| for (const [layoutIdx, value] of route.layer1) { |
There was a problem hiding this comment.
The route.layer1 iteration destructures [layoutIdx, value] but value carries the kind ("static" | "dynamic") which is already available. This is fine. However, the invariant check on layer1Reasons (lines 124-127) protects against a data integrity issue where layer1 and layer1Reasons are out of sync — but this can only happen if collectRouteClassificationManifest has a bug since both maps are populated in lockstep on lines 75-76. The throw is good defense-in-depth, but consider whether a debug.assert would be more appropriate here to avoid the overhead in production builds.
There was a problem hiding this comment.
The throw is the right choice here. node:assert is the only assert facility available in this codebase and it throws at the same cost — there is no stripped debug-only assert utility. The intent is fail-fast on a programmer error (layer1 and layer1Reasons out of sync), which throw expresses correctly. Left as-is.
| }, | ||
| }; | ||
|
|
||
| const layer2PerRoute = new Map<number, Map<number, "static">>(); |
There was a problem hiding this comment.
Minor: Map<number, Map<number, "static">> is effectively a Map<number, Set<number>> since the value is always the literal "static". Using a Set would make the intent clearer and avoid the question of what other values could appear. That said, this matches the type signature of buildGenerateBundleReplacement's parameter, so changing it would ripple — not blocking.
There was a problem hiding this comment.
Agreed on the semantics, but changing this would require updating the parameter type of buildGenerateBundleReplacement (and its sibling buildReasonsReplacement in #843), plus the layer2PerRoute maps built in the generateBundle hook. Deferring as a follow-up since the ripple isn't worth it mid-stack.
| */ | ||
| export function buildGenerateBundleReplacement( | ||
| manifest: RouteClassificationManifest, | ||
| layer2PerRoute: ReadonlyMap<number, ReadonlyMap<number, "static">>, |
There was a problem hiding this comment.
The layer2PerRoute parameter is typed as ReadonlyMap<number, ReadonlyMap<number, "static">>. This is the right constraint — Layer 2 can only prove static, never dynamic. But the docstring on line 167 says "Layer 2 results must be filtered to only "static" before calling this function" — this implies the caller could pass unfiltered results. The type already enforces this at compile time, so the docstring is slightly misleading. Consider rewording to "Layer 2 results are typed to only carry "static" entries" to reflect that this is a type-level guarantee, not a caller responsibility.
| const builder = await createBuilder({ | ||
| root: tmpDir, | ||
| configFile: false, | ||
| plugins: [vinext({ appDir: tmpDir, rscOutDir, ssrOutDir, clientOutDir })], |
There was a problem hiding this comment.
The vinext() call passes { appDir: tmpDir, rscOutDir, ssrOutDir, clientOutDir } — are appDir, rscOutDir, ssrOutDir, clientOutDir all real vinext options? I see appDir referenced in the fixture but the main plugin options type may not include all of these. If these are undocumented/internal options, that's fine for a test, but worth a brief comment noting they're internal-only build knobs.
There was a problem hiding this comment.
These are all public VinextOptions fields (appDir, rscOutDir, ssrOutDir, clientOutDir are all declared in the exported VinextOptions type). No special comment needed.
|
The review has been submitted. Here's a summary of what I covered: Key findings:
Positive observations:
|
- Expand stubRe comment to note __VINEXT_CLASS name-retention assumption - Null out rscClassificationManifest after generateBundle consumes it - Fix buildGenerateBundleReplacement docstring to reflect type-level guarantee
|
/bigbonk review |
There was a problem hiding this comment.
Review
This is well-structured infrastructure work with a clean layered design. The type system (discriminated unions for classification + reasons), the defensive generateBundle error handling, and the caching strategy (sourceCache for Layer 1, graphCache for Layer 2) are all solid. The integration test that exercises the full Vite build pipeline is the right investment — it catches the exact class of silent regression this PR exists to prevent.
A few items below, roughly ordered by severity.
Substantive
-
Source map invalidation on
target.codemutation. ThegenerateBundlehook directly mutatestarget.codewithout touchingtarget.map. If Rolldown emitted a source map for the RSC entry chunk, the replacement text will be longer than the stub it replaced (a switch statement vsreturn null), and every line after the patch point will have incorrect source mappings. This may not matter today if RSC entry source maps are not consumed, but it's a latent correctness issue. Consider either invalidating the map (target.map = null) or noting explicitly why it's safe to skip. -
The
routeIdxproperty is unnecessary runtime data. Every route object in the generatedroutes = [...]array now carries arouteIdxfield, but it's never read at runtime — it exists solely to generate the__VINEXT_CLASS(N)call expression during codegen. Since the call is evaluated at module load time and the result is stored in__buildTimeClassifications,routeIdxis dead weight in the runtime route table. Consider removing it from the route object and only using the loop variablerouteIdxduring codegen. This keeps the generated route table lean. -
__VINEXT_CLASSis untyped in the generated code. The stub function has no parameter type annotation and no return type. While this is inside a generated entry (not hand-authored TypeScript), therouteIdxparameter being untyped means Rolldown/Vite sees it asany. Not a runtime issue since the function is always called with a numeric literal, but adding a brief@param {number} routeIdxJSDoc or a TS annotation would make the generated code more self-documenting for anyone debugging the built output.
| ); | ||
| const patchedBody = `function __VINEXT_CLASS(routeIdx) { return (${replacement})(routeIdx); }`; | ||
| const target = chunksWithStubBody[0]!.chunk; | ||
| target.code = target.code.replace(stubRe, patchedBody); |
There was a problem hiding this comment.
This mutates target.code without updating target.map. If the RSC entry chunk has an associated source map, the map will become stale after this replacement (the patched body is longer than the stub). Consider either:
- Invalidating the map:
target.map = null;(safe if source maps aren't consumed for RSC entries) - Documenting why map correctness doesn't matter here
Not blocking if RSC entry source maps are never served, but worth a defensive line.
| ep ? getImportVar(ep) : "null", | ||
| ); | ||
| return ` { | ||
| routeIdx: ${routeIdx}, |
There was a problem hiding this comment.
routeIdx is emitted as a runtime property on every route object but is never read after module load — the __VINEXT_CLASS(${routeIdx}) call is already evaluated eagerly and stored in __buildTimeClassifications. This property is dead weight in the runtime route table.
Consider removing it from the generated route object. The codegen already has routeIdx available as the routes.map callback index — it only needs to appear in the __VINEXT_CLASS(N) call expression, not as a stored property.
| routeIdx: ${routeIdx}, | |
| __buildTimeClassifications: __VINEXT_CLASS(${routeIdx}), // evaluated once at module load |
| function __VINEXT_CLASS(routeIdx) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
The generated stub is plain JavaScript with no type annotations. Since this is inside a .ts-templated entry that goes through the RSC build, consider adding a minimal return type to make the generated output more self-documenting:
| function __VINEXT_CLASS(routeIdx) { | |
| return null; | |
| } | |
| function __VINEXT_CLASS(routeIdx) { | |
| return /** @type {Map<number, 'static' | 'dynamic'> | null} */ (null); | |
| } |
Non-blocking — this is cosmetic for anyone debugging the built RSC chunk.
| function serializeReasonExpression(reason: ClassificationReason): string { | ||
| switch (reason.layer) { | ||
| case "segment-config": { | ||
| const value = reason.value === Infinity ? "Infinity" : JSON.stringify(reason.value); |
There was a problem hiding this comment.
Minor: when reason.value is a number (e.g., 0), JSON.stringify(0) produces "0" which is correct. But the === Infinity check is order-dependent — it must come before JSON.stringify because JSON.stringify(Infinity) returns "null", which would silently produce value: null in the generated code. The current ordering is correct, but this is subtle enough to warrant a brief comment.
| const value = reason.value === Infinity ? "Infinity" : JSON.stringify(reason.value); | |
| // Infinity must be checked first: JSON.stringify(Infinity) produces "null". | |
| const value = reason.value === Infinity ? "Infinity" : JSON.stringify(reason.value); |
| const dynamicShimPaths: ReadonlySet<string> = new Set( | ||
| [ | ||
| resolveShimModulePath(shimsDir, "headers"), | ||
| resolveShimModulePath(shimsDir, "server"), | ||
| resolveShimModulePath(shimsDir, "cache"), | ||
| ].map(canonicalize), | ||
| ); |
There was a problem hiding this comment.
The dynamic shim set is constructed on every generateBundle invocation. Since shimsDir is stable for the lifetime of the plugin, this could be hoisted to plugin initialization (alongside the existing shimsDir definition). Minor perf nit — not blocking since generateBundle runs once per build.
| * it via the plugin's generateBundle hook. | ||
| */ | ||
| function extractDispatch(chunkSource: string): Dispatch { | ||
| const stubRe = /function\s+__VINEXT_CLASS\s*\(routeIdx\)\s*\{\s*return null;\s*\}/; |
There was a problem hiding this comment.
The stubRe in the test differs subtly from the one in the plugin: the test regex requires the semicolon (return null;\s*\}), while the plugin regex makes it optional (return null;?\s*\}). If Rolldown ever strips the semicolon, the plugin would handle it correctly but this test's "was it patched?" check would false-negative (it would not detect the unpatched stub, so it wouldn't throw, and the subsequent regex extraction would fail with a confusing error).
Consider making the test regex match the plugin's tolerance:
| const stubRe = /function\s+__VINEXT_CLASS\s*\(routeIdx\)\s*\{\s*return null;\s*\}/; | |
| const stubRe = /function\s+__VINEXT_CLASS\s*\(routeIdx\)\s*\{\s*return null;?\s*\}/; |
|
The review has been submitted. Here's what I covered: Substantive findings (3):
Inline comments (6):
Positive observations: The layered classification design is clean, caching is correctly scoped, the stub-body drift detection is a good defensive pattern, and the runtime consumer correctly short-circuits on build-time results while still probing for special errors. |
- Remove routeIdx from generated route objects (dead runtime property, only needed as codegen loop variable for __VINEXT_CLASS(N) call) - Null target.map after patching target.code in generateBundle - Add Infinity-before-JSON.stringify comment in serializeReasonExpression - Fix integration test stubRe to tolerate optional semicolon like the plugin - Update extractRouteIndexByPattern to anchor on __VINEXT_CLASS(N) instead of the removed routeIdx property - Update snapshots
- Mark classifyAllRouteLayouts as @internal test-only (production code calls classifyLayoutByModuleGraph directly via the manifest pipeline) - Document unreachable runtime-probe/no-classifier arms in serializeReasonExpression with explanation of why narrowing is deferred - Add manifest lifecycle invariant comment to load hook explaining the 1:1 correspondence between manifest layout indices and codegen routes - Add test for layer1/layer1Reasons mismatch guard in mergeLayersForRoute
|
/bigbonk review |
There was a problem hiding this comment.
Review
This is well-designed infrastructure. The layered classification pipeline (segment-config → module-graph → runtime probe), the defensive generateBundle stub-drift detection, and the caching strategy (sourceCache for Layer 1, graphCache for Layer 2) are all solid. The integration test exercising the full Vite build pipeline is the right investment — it catches the exact class of silent regression (stub never patched → Layer 3 fallback) that this PR exists to prevent.
The previous reviews covered most surface-level concerns and the author has addressed them (manifest clearing, source map nulling, stub regex comments). I have one remaining issue and a few observations.
Issue: classifyAllRouteLayouts is now dead production code
The @internal annotation on classifyAllRouteLayouts (line 119 of layout-classification.ts) says it's "Not called by production code" and is "Used only by tests/layout-classification.test.ts." This is accurate — the generateBundle hook in index.ts calls classifyLayoutByModuleGraph directly and composes via the numeric-index manifest. But classifyAllRouteLayouts is still exported from the module's public surface.
This isn't blocking, but the function now serves only as test infrastructure for verifying that the Layer 1 → Layer 2 composition logic works. If the generateBundle hook's composition logic ever diverges from what classifyAllRouteLayouts does (they share classifyLayoutByModuleGraph but have different merge strategies — string-keyed layout IDs vs numeric-keyed manifest entries), the test would pass while production behavior is wrong. Worth considering whether the test should instead exercise collectRouteClassificationManifest + buildGenerateBundleReplacement (the actual production path) rather than the now-vestigial classifyAllRouteLayouts.
Observations
-
The previous reviewer's concern about a "dead
routeIdxproperty" was a misread. There is norouteIdxproperty emitted on route objects —routeIdxis theArray.mapcallback index used only to generate the__VINEXT_CLASS(N)call expression. The generated code is correct and lean. -
The
buildReasonsReplacementbeing tested-but-uncalled is a reasonable choice given it's consumed by #843. The author's response (keeping it here so #843 imports tested code) makes sense for the stacked PR workflow. -
The dynamic shim set (
headers,server,cache) coverscookies()(inheaders.ts),connection()(inserver.ts), and cache APIs. This looks complete for the current API surface.
| } | ||
| } | ||
|
|
||
| for (const [layoutIdx, value] of route.layer1) { |
There was a problem hiding this comment.
Minor: the value binding from the destructured [layoutIdx, value] is only used as { kind: value, reason } on line 129. Since it's always "static" | "dynamic" (the Layer1Class type), this is fine — but it shadows the broader concept of "value" in this function. Consider naming it kind directly:
| for (const [layoutIdx, value] of route.layer1) { | |
| for (const [layoutIdx, kind] of route.layer1) { |
and then line 129 would read merged.set(layoutIdx, { kind, reason }) which is self-documenting.
There was a problem hiding this comment.
Renamed in 6827a50 — mergeLayersForRoute now destructures [layoutIdx, kind] and uses { kind, reason } shorthand on the merge.
| * | ||
| * @internal Not called by production code. The `generateBundle` hook in |
There was a problem hiding this comment.
The @internal annotation is good, but there's a subtle risk: this function's Layer 1 → Layer 2 composition uses string-keyed layout IDs ("layout:/blog") while the actual production path in generateBundle uses numeric layout indices via the manifest. If the two composition strategies ever diverge (e.g., the merge priority logic changes in mergeLayersForRoute but not here), the layout-classification.test.ts tests would still pass while production is broken.
Not blocking — the composition logic is simple enough today that the drift risk is low. But worth noting for future reference: if the merge logic grows, the tests should shift to exercise the production codepath (collectRouteClassificationManifest → buildGenerateBundleReplacement).
There was a problem hiding this comment.
Noted. The drift risk is real — once the merge logic grows beyond the current Layer-1-wins rule, the unit tests need to migrate onto collectRouteClassificationManifest + buildGenerateBundleReplacement so the assertions exercise the production composition rather than the string-keyed test path. Tracking for the next PR in this stack.
| ); | ||
| const patchedBody = `function __VINEXT_CLASS(routeIdx) { return (${replacement})(routeIdx); }`; | ||
| const target = chunksWithStubBody[0]!.chunk; | ||
| target.code = target.code.replace(stubRe, patchedBody); |
There was a problem hiding this comment.
The stubRe is used for both .test() (line 1697) and .replace() (here). Since .test() advances the regex's lastIndex for regexes with the g flag, and this regex doesn't have the g flag, the two uses are safe. But worth noting that the .replace() call here will only replace the first match — which is correct since the earlier check enforces exactly one chunk with the stub body. Good.
There was a problem hiding this comment.
Confirmed. The regex has no g flag, so .test() does not advance lastIndex, and the upstream presence/uniqueness check guarantees .replace() only ever rewrites the single stub chunk. Leaving as-is.
| const canonicalize = (p: string): string => tryRealpathSync(p) ?? p; | ||
|
|
||
| const dynamicShimPaths: ReadonlySet<string> = new Set( |
There was a problem hiding this comment.
The canonicalize closure and dynamicShimPaths set are reconstructed on every generateBundle invocation. Since shimsDir is stable for the plugin's lifetime and tryRealpathSync is deterministic for a given filesystem state, both could be hoisted to plugin initialization.
Not blocking — generateBundle runs once per build, so the cost is negligible. But if you end up touching this area again, hoisting would make the hook body a bit leaner.
There was a problem hiding this comment.
Hoisted in 6827a50. canonicalize and dynamicShimPaths now live next to the shimsDir definition at plugin init, and the generateBundle hook just references them.
| built = await buildMinimalFixture(); | ||
| }, 120_000); | ||
|
|
||
| afterAll(() => { |
There was a problem hiding this comment.
The afterAll is a deliberate no-op (leaves tmpdirs for debugging). This is fine for CI, but worth noting that buildMinimalFixture creates two temp directories (the fixture dir and the output dir) that accumulate on local dev machines. The route-classification-manifest.test.ts file cleans up in afterEach — the difference in cleanup strategy between the two test files is intentional (integration tests leave artifacts, unit tests don't) but not documented.
Consider adding a brief comment like // Intentionally different from unit tests — integration artifacts are useful for post-mortem debugging.
|
Review submitted successfully. Here's a summary of what I covered: Key finding:
Corrections to prior reviews:
Inline comments (5):
|
…inding
`canonicalize` and `dynamicShimPaths` were constructed inside the RSC
generateBundle hook, so they were rebuilt on every invocation even though
shimsDir is fixed for the plugin's lifetime and tryRealpathSync is
deterministic against the package's own shim files. The cost is small
because generateBundle runs once per build, but the hook body grew long
enough that hoisting the closure and the shim-path Set up next to the
shimsDir definition makes the bundle hook read more like the request-time
behaviour it is wiring up.
Separately, mergeLayersForRoute destructured Layer 1 entries as
[layoutIdx, value] and then wrote { kind: value, reason }. Since the
binding is always the Layer1Class kind, renaming it to `kind` lets the
merge use object-shorthand and removes the value-vs-kind shadowing.
No behaviour change; same realpath calls, same merge result, same
generated dispatch table. Targeted unit and integration suites still pass.
|
/bigbonk review |
There was a problem hiding this comment.
Review: Build-time layout classification wiring
This is well-designed infrastructure. The layered classification pipeline is clean, the codegen bridge is correctly factored, and the generateBundle hook's defensive error handling (stub-body drift detection, scan-phase skip, single-chunk enforcement) is genuinely good engineering. The caching strategy (sourceCache for Layer 1 file reads, graphCache for Layer 2 module-graph traversals) correctly deduplicates the two most expensive operations. Test coverage is thorough — the integration test that builds through the full Vite pipeline and evaluates the emitted dispatch function is the right level of verification for this kind of codegen work.
Previous reviews have been comprehensive. Most substantive concerns have been addressed (manifest clearing, source map nulling, hoisting canonicalize/dynamicShimPaths to plugin init, stub regex comments). I have a few remaining observations — one correctness concern on the mergeLayersForRoute Layer 2 reason, and a few minor items.
Substantive
-
Layer 2 reason objects are reconstructed on every
mergeLayersForRoutecall rather than being carried from the actual BFS traversal. The hardcoded{ layer: "module-graph", result: "static" }reason inmergeLayersForRoute(line 115-118 ofroute-classification-manifest.ts) discards anyfirstShimMatchthat the classifier may have found. This doesn't matter for Layer 2"static"results today (they never have afirstShimMatch), but it means the merge function silently drops diagnostic data for any future Layer 2 variant that carries it. See inline comment. -
buildGenerateBundleReplacementandbuildReasonsReplacementare near-duplicates. They share the same manifest iteration, merge logic, sort, and switch-case codegen structure — only the map entry serialization differs. This is acceptable for now given the small surface, but if a third variant appears (or the merge logic changes), these should be unified into a single function parameterized by the entry serializer.
Minor
-
The
needs-probekind inLayoutBuildClassificationis defined but never constructed by any code path in this PR.classifyAllRouteLayoutsmaps"needs-probe"from the module-graph result directly, but the production path ingenerateBundlefilters these out before they reachbuildGenerateBundleReplacement. The type is correct (it documents a possible Layer 2 outcome), but no test exercises a{ kind: "needs-probe", reason }flowing throughmergeLayersForRoute. The existinglayout-classification.test.tscovers theclassifyAllRouteLayoutspath, which does produceneeds-probe— but that's the test-only path, not the production path. -
Overall this is solid infrastructure that cleanly separates the classification pipeline from the (still-evolving) skip mechanism. The runtime consumer in
app-page-execution.tscorrectly short-circuits on build-time results while still probing for special errors (redirects, not-found), which is the right trade-off. Ready to land with or without the inline suggestions below.
| merged.set(layoutIdx, { | ||
| kind: "static", | ||
| reason: { layer: "module-graph", result: "static" }, | ||
| }); |
There was a problem hiding this comment.
The Layer 2 reason here is reconstructed as a fresh object literal rather than being passed through from the actual classifyLayoutByModuleGraph call. This works today because Layer 2 "static" results never carry a firstShimMatch, but it means the merge function is the only place that constructs Layer 2 reason objects — if a caller ever passes richer Layer 2 data through layer2PerRoute, the diagnostic information would be silently dropped.
This is fine for this PR since layer2PerRoute is typed as Map<number, "static"> (the reason can only ever be { layer: "module-graph", result: "static" }). But if layer2PerRoute ever carries a reason alongside the classification, this is the spot that would need updating. Non-blocking.
There was a problem hiding this comment.
Fixed in c03ed6bd.
layer2PerRoute now carries typed module-graph reason objects instead of lossy "static" markers, and mergeLayersForRoute() consumes those reasons directly instead of reconstructing them. I also factored the shared dispatch generation into buildRouteDispatchReplacement() so the classification and reason emitters stop drifting independently.
Validation: vp test run tests/route-classification-manifest.test.ts tests/build-time-classification-integration.test.ts.
| " default: return null;", | ||
| " }", | ||
| " }", | ||
| ].join("\n"); |
There was a problem hiding this comment.
Nit: the generated arrow function wraps the switch in an IIFE pattern:
function __VINEXT_CLASS(routeIdx) { return ((routeIdx) => { switch... })(routeIdx); }The double-invocation (outer function + inner arrow) is harmless but adds one unnecessary closure allocation per module load. An alternative codegen shape would be to emit the switch body directly:
function __VINEXT_CLASS(routeIdx) { switch (routeIdx) { case 0: ...; default: return null; } }This avoids the intermediate arrow function. Non-blocking — the dispatch runs once at module load, so the perf difference is negligible. But it would simplify the generated output and make the integration test's regex extraction easier.
| const moduleInfo = { | ||
| getModuleInfo: (moduleId: string) => { | ||
| const info = this.getModuleInfo(moduleId); | ||
| if (!info) return null; |
There was a problem hiding this comment.
Good adapter pattern. One observation: info.importedIds ?? [] and info.dynamicallyImportedIds ?? [] use nullish coalescing, but Rolldown's getModuleInfo returns these as arrays (never null/undefined) for modules that exist in the graph. The ?? [] is defensive, which is fine, but it also means that if getModuleInfo returns a module with importedIds: undefined (indicating a malformed or partially-loaded module), the BFS would silently treat it as a leaf node rather than flagging it. This is the correct behavior for the classifier (conservative = safe), just noting it.
| // the runtime probe for a layout we never actually analysed. | ||
| // `classifyLayoutByModuleGraph` returns "static" for an empty | ||
| // traversal, so the seed presence check has to happen here. | ||
| if (!moduleInfo.getModuleInfo(layoutModuleId)) continue; |
There was a problem hiding this comment.
This is a critical guard. The comment explains the reasoning well — classifyLayoutByModuleGraph returns "static" for an empty traversal (a module with no imports is trivially static), so without this check, a layout that simply isn't in the module graph (e.g., tree-shaken out, or not yet resolved) would be falsely classified as static. Good.
| // Map<layoutIndex, "static" | "dynamic"> for each route. Until the | ||
| // plugin patches this stub, every route falls back to the Layer 3 | ||
| // runtime probe, which is the current (slow) behaviour. | ||
| function __VINEXT_CLASS(routeIdx) { |
There was a problem hiding this comment.
Minor: the stub function signature function __VINEXT_CLASS(routeIdx) has no type annotation on the parameter or return type. Since this is inside a generated template string that goes through the RSC build, type annotations won't survive bundling anyway. But if the generated entry is ever type-checked during development (e.g., via the Vite dev server's TypeScript transform), the implicit any parameter could produce a lint warning depending on the project's strictness settings.
Non-blocking — the function only exists until generateBundle replaces it.
|
|
||
| beforeAll(async () => { | ||
| built = await buildMinimalFixture(); | ||
| }, 120_000); |
There was a problem hiding this comment.
The 120s timeout is appropriate for a full createBuilder + buildApp() cycle. Worth noting that if this test runs alongside other integration tests that also call buildApp(), the Vite deps optimizer cache can cause flaky lock contention (the reason the test suite runs serially). Since this test creates its own isolated tmpdir, it should be safe from that, but the symlinked node_modules means the optimizer cache directory is shared with the workspace. If flakiness appears, an isolated cacheDir in the Vite config would fix it.
|
Review submitted. Here's a summary of what I covered: Key findings:
Positive observations:
|
|
/bigbonk review |
There was a problem hiding this comment.
Review: Build-time layout classification wiring
This is well-structured infrastructure work. The layered classification design is clean, the generateBundle hook has good defensive error handling (stub-drift detection, scan-phase skip, single-chunk enforcement), and the caching strategy correctly deduplicates both Layer 1 file reads and Layer 2 module-graph traversals. The integration test that builds through the full Vite pipeline and vm.runInThisContext-evaluates the emitted dispatch function is the right level of verification for codegen work.
I have one correctness concern, one structural issue, and a few minor observations.
Issues
-
ModuleGraphStaticReasonis duplicated across two modules — The same type shape{ layer: "module-graph"; result: "static"; firstShimMatch?: string }is defined independently in bothindex.ts:111androute-classification-manifest.ts:97. These are not shared via import — they're structurally identical but disconnected. If one evolves (e.g., a new diagnostic field is added to the manifest version), the other won't get the update, and the compiler won't catch it since the types only need structural compatibility at call sites. This should be a single exported type fromlayout-classification-types.tsorroute-classification-manifest.ts. -
The IIFE wrapper in the patched
__VINEXT_CLASSadds an unnecessary closure — ThegenerateBundlehook produces:function __VINEXT_CLASS(routeIdx) { return ((routeIdx) => { switch... })(routeIdx); }
The outer function already receives
routeIdx, so the inner arrow function's parameter shadows it unnecessarily. This could be simplified to emit the switch body directly inside__VINEXT_CLASS, avoiding one closure allocation per module load. The perf impact is negligible (runs once), but the generated output would be cleaner and easier to debug.
Minor observations
-
The
serializeReasonExpressionfunction handles all fourClassificationReasonvariants (runtime-probe,no-classifier) even though onlysegment-configandmodule-graphare reachable from the build-time pipeline. The comment on lines 154-160 explains this well — it's for type exhaustiveness and forward compatibility with #843. Acceptable. -
The
classifyAllRouteLayoutsfunction is now explicitly@internaland test-only, while production usescollectRouteClassificationManifest+buildGenerateBundleReplacementwith numeric indices. The two paths shareclassifyLayoutByModuleGraphbut have different merge strategies (string-keyed layout IDs vs. numeric-keyed manifest entries). This is fine today since the merge logic is simple (Layer-1-wins), but if it grows, the test-only path could silently diverge from production. The@internalannotation documents this well. -
The integration test's
afterAllintentionally leaves temp directories for post-mortem debugging, while the unit test (route-classification-manifest.test.ts) cleans up inafterEach. The difference in cleanup strategy is intentional and reasonable.
| layer: "module-graph"; | ||
| result: "static"; | ||
| firstShimMatch?: string; | ||
| }; |
There was a problem hiding this comment.
This type is structurally identical to ModuleGraphStaticReason in route-classification-manifest.ts:97. Having two independent definitions of the same type means they can drift independently — if someone adds a field to one, the other won't get it, and the compiler only checks structural compatibility at call sites (which may not exercise all fields).
Consider exporting ModuleGraphStaticReason from route-classification-manifest.ts (or from layout-classification-types.ts alongside the other shared types) and importing it here.
| }; | |
| import type { | |
| ModuleGraphStaticReason, | |
| } from "./build/route-classification-manifest.js"; |
There was a problem hiding this comment.
Fixed in 22a17313.
ModuleGraphStaticReason now lives in layout-classification-types.ts as the shared leaf type, and both route-classification-manifest.ts and index.ts import it instead of carrying duplicate local definitions.
Validation: vp test run tests/route-classification-manifest.test.ts tests/build-time-classification-integration.test.ts.
| rscClassificationManifest, | ||
| layer2PerRoute, | ||
| ); | ||
| const patchedBody = `function __VINEXT_CLASS(routeIdx) { return (${replacement})(routeIdx); }`; |
There was a problem hiding this comment.
The generated replacement wraps the switch in an IIFE:
function __VINEXT_CLASS(routeIdx) { return ((routeIdx) => { switch... })(routeIdx); }The inner arrow function's routeIdx parameter shadows the outer function's routeIdx, adding one unnecessary closure allocation. A simpler codegen shape would emit the switch body directly:
function __VINEXT_CLASS(routeIdx) { switch (routeIdx) { case 0: ...; default: return null; } }This would require buildRouteDispatchReplacement to emit a switch statement body rather than a self-contained arrow function expression. Non-blocking since __VINEXT_CLASS runs once at module load, but it would simplify both the generated output and the integration test's regex extraction.
There was a problem hiding this comment.
Left as a follow-up.
I agree with the shape concern, but rewriting buildRouteDispatchReplacement() to emit statement-form switch bodies would change both buildGenerateBundleReplacement() and buildReasonsReplacement() plus the integration-test extraction path. That is a broader codegen refactor than I want to fold into this review-fix pass, especially with #843 depending on the shared dispatcher shape.
For this PR I limited the patch to the correctness/maintainability issues above; the extra closure here is non-blocking since __VINEXT_CLASS runs once at module load.
| graphCache.set(layoutModuleId, graphResult); | ||
| } | ||
| if (graphResult.result === "static") { | ||
| perRoute.set(layoutIdx, { layer: "module-graph", result: graphResult.result }); |
There was a problem hiding this comment.
Minor: the ModuleGraphStaticReason object is reconstructed here with { layer: "module-graph", result: graphResult.result } rather than using the moduleGraphReason() helper from layout-classification.ts (which exists specifically to build these reason objects). Using the helper would centralize reason construction:
| perRoute.set(layoutIdx, { layer: "module-graph", result: graphResult.result }); | |
| perRoute.set(layoutIdx, moduleGraphReason(graphResult)); |
This requires importing moduleGraphReason from layout-classification.ts and narrowing graphResult to { result: "static" } (which the if guard on line 1784 already ensures). The overloaded signature on moduleGraphReason already narrows the return type for result: "static" inputs.
There was a problem hiding this comment.
Fixed in 22a17313.
generateBundle now uses moduleGraphReason(graphResult) again, gated by an explicit isStaticModuleGraphResult() type guard so the call site stays type-safe without reconstructing the object literal.
Validation: vp test run tests/route-classification-manifest.test.ts tests/build-time-classification-integration.test.ts.
| return [ | ||
| "(routeIdx) => {", | ||
| " switch (routeIdx) {", | ||
| ...cases, | ||
| " default: return null;", | ||
| " }", | ||
| " }", | ||
| ].join("\n"); |
There was a problem hiding this comment.
The codegen emits the dispatch as an arrow function expression wrapped in an IIFE call. For the buildGenerateBundleReplacement case (which only serializes "static" or "dynamic" strings), the generated output looks like:
(routeIdx) => {
switch (routeIdx) {
case 0: return new Map([[0, "dynamic"]]);
default: return null;
}
}This is clean. But after index.ts:1797 wraps it in function __VINEXT_CLASS(routeIdx) { return (<this>)(routeIdx); }, the double-invocation emerges. Consider whether buildRouteDispatchReplacement could instead emit a statement-form body (just the switch) that gets inlined directly into the function, eliminating the IIFE pattern entirely. This would be a cross-cutting change affecting both buildGenerateBundleReplacement and buildReasonsReplacement, so probably better as a follow-up.
| const layer2PerRoute = new Map<number, Map<number, ModuleGraphStaticReason>>(); | ||
| const graphCache = new Map<string, ReturnType<typeof classifyLayoutByModuleGraph>>(); |
There was a problem hiding this comment.
Good: graphCache is keyed by layoutModuleId (the canonicalized path), so shared layouts across routes are classified once. This correctly addresses the O(routes × shared-layouts) concern from earlier reviews.
|
Review submitted. Here's a summary of what I covered: Key findings:
Positive observations:
|
|
/bigbonk review |
There was a problem hiding this comment.
Review: Build-time layout classification wiring
This is well-designed infrastructure. The layered classification pipeline (segment-config → module-graph → runtime probe) is cleanly factored, the type system is well-structured, and the defensive generateBundle hook has exactly the right error semantics. The PR has been through several rounds of review already, and the author has addressed all substantive feedback (type deduplication fix, manifest clearing, source map nulling, canonicalize/dynamicShimPaths hoisting, moduleGraphReason() usage, layer2PerRoute reason carrying).
The code is correct and the test coverage is thorough — 44 unit tests for the manifest helpers, 5 integration tests through the full Vite build pipeline, plus updated tests for the classifier and entry templates.
Remaining observations (non-blocking)
-
IIFE wrapper in generated code — The generated
__VINEXT_CLASSwraps the dispatch arrow in an IIFE (function __VINEXT_CLASS(routeIdx) { return ((routeIdx) => { switch... })(routeIdx); }). The author has acknowledged this and deferred the codegen simplification to avoid a cross-cutting change in this PR. Acceptable — it runs once at module load. -
classifyAllRouteLayoutstest-only divergence risk — The@internalannotation documents that production usescollectRouteClassificationManifest+buildGenerateBundleReplacement(numeric indices) whileclassifyAllRouteLayoutsuses string-keyed layout IDs. The author acknowledged the drift risk and is tracking it. The merge logic is simple enough today (Layer-1-wins) that the risk is low. -
buildReasonsReplacementtested but not called in production — This is intentional infrastructure for #843. The author's rationale (keeping it here so #843 imports tested code rather than landing untested code) is sound for a stacked PR workflow.
No blocking issues remain. Approving.
| outcome: "static" | "dynamic"; | ||
| error?: string; | ||
| } | ||
| | { layer: "no-classifier" }; |
There was a problem hiding this comment.
The { layer: "no-classifier" } variant of ClassificationReason has no producer in the build-time pipeline and no consumer in this PR. It's only reachable via the serializeReasonExpression exhaustiveness arm. Is this used by the runtime probe path in app-page-execution.ts, or is it purely forward-looking for #843?
Not asking for a change — just noting that the type is broader than its current production usage, which is fine for a discriminated union designed for extensibility.
| // emit the trailing semicolon depending on minification settings. | ||
| // This regex relies on `__VINEXT_CLASS` retaining its name, which holds | ||
| // because RSC entry chunk bindings are not subject to scope-hoisting renames. | ||
| const stubRe = /function __VINEXT_CLASS\(routeIdx\)\s*\{\s*return null;?\s*\}/; |
There was a problem hiding this comment.
Good comments explaining both the semicolon tolerance and the entry-chunk rename assumption. The regex + defensive error handling (stub-body drift detection, scan-phase skip, multi-chunk rejection) is genuinely well-engineered.
| // the runtime probe for a layout we never actually analysed. | ||
| // `classifyLayoutByModuleGraph` returns "static" for an empty | ||
| // traversal, so the seed presence check has to happen here. | ||
| if (!moduleInfo.getModuleInfo(layoutModuleId)) continue; |
There was a problem hiding this comment.
This guard is essential and the comment explains it well. classifyLayoutByModuleGraph returns { result: "static" } for an empty BFS traversal (a module with zero imports is trivially static), so without this check, a layout that isn't in the module graph at all would be falsely classified as static. Good defensive design.
| * says `force-dynamic` must never be demoted to "static" because its module | ||
| * graph happened to be clean. | ||
| */ | ||
| function mergeLayersForRoute( |
There was a problem hiding this comment.
The merge function correctly applies Layer 2 first, then overwrites with Layer 1 — a clean way to implement the Layer-1-wins priority rule without branching. Since layer2 only ever contains ModuleGraphStaticReason (typed as such), the overwrite semantics are always correct: Layer 1 dynamic overrides Layer 2 static, and Layer 1 static is idempotent with Layer 2 static.
|
|
||
| // Non-greedy match: assumes the inner dispatch body does not contain | ||
| // ')(routeIdx)' as a substring. Coupled to the codegen shape in | ||
| // route-classification-manifest.ts buildGenerateBundleReplacement. |
There was a problem hiding this comment.
Minor: this non-greedy [\s\S]*? regex is coupled to the codegen shape (it assumes no )(routeIdx) substring in the switch body). The comment on lines 54-56 documents this coupling well. If the codegen ever changes to inline the switch body (eliminating the IIFE), this extraction logic would need updating too. The coupling is fine given both are in the same repo.
|
Review submitted successfully. Here's a summary of what I covered: Verdict: Approved This PR adds a build-time layout classification pipeline that bridges the segment-config and module-graph classifiers with the RSC entry codegen via a Key strengths:
Non-blocking observations noted in inline comments:
All substantive issues from prior reviews have been addressed in the latest commits. |
| // emit the trailing semicolon depending on minification settings. | ||
| // This regex relies on `__VINEXT_CLASS` retaining its name, which holds | ||
| // because RSC entry chunk bindings are not subject to scope-hoisting renames. | ||
| const stubRe = /function __VINEXT_CLASS\(routeIdx\)\s*\{\s*return null;?\s*\}/; |
There was a problem hiding this comment.
To be honest I'm not the biggest fan of this approach. It feels a bit brittle.
It would be worth us finding another way to deal with this going forward - probably worth making an issue to track @NathanDrake2406 .
Summary
Adds a build-time layout classification pipeline. A Rollup
generateBundlehook patches the__VINEXT_CLASSstub in the generated RSC entry with a real dispatch table, built from segment-config (Layer 1) and module-graph (Layer 2) analysis. The runtime probe loop inapp-page-execution.tsconsults this table and skips the Layer 3 dynamic-isolation probe for layouts already proved static or dynamic at build time.This lands as standalone reusable infrastructure. Build-time classification is the precondition for any future layout-skip optimization, independent of how the skip mechanism is implemented at runtime.
What changes
build/route-classification-manifest.tsbridges the classifier with the entry template codegen.build/layout-classification-types.tscarries the taggedLayoutBuildClassificationandClassificationReasontypes.ClassificationReasonis consumed by the debug sidecar follow-up; including it here keeps the codegen machinery self-contained.classifyLayoutSegmentConfiginbuild/report.tsnow returnsLayoutBuildClassificationinstead of"static" | "dynamic" | null.classificationblock in the generated RSC entry threadsbuildTimeClassificationsthrough.generateBundlehook fails loudly if it sees__VINEXT_CLASSreferenced without the recognized stub body, so the codegen template and the plugin cannot silently drift.Context
This was originally PR 5 of a 6-PR stack (#838, #839, #840, #841, this PR, #843). #838 and #839 landed. #840 and #841 are closed: their server-side skip mechanism relied on rewriting the React Flight wire-format payload on egress, which proved too coupled to React-internal tag conventions for a framework that does not vendor React. The skip-mechanism design is being reworked separately around render-time omission rather than byte-level filtering.
This PR is unaffected by that decision. The classifier is needed for any approach. The runtime probe loop already short-circuits on its output, so this PR is useful even before the new skip mechanism lands.
#843 still depends on this PR for the
ClassificationReasonmachinery.Test plan
tests/route-classification-manifest.test.ts(44 tests)tests/build-time-classification-integration.test.ts(5 tests)tests/build-report.test.tstests/layout-classification.test.tstests/app-router.test.ts(build-time classification dispatch stub cases)tests/entry-templates.test.ts(snapshot regenerated)