feat(build): wire build-time layout classification into RSC entry#842
feat(build): wire build-time layout classification into RSC entry#842NathanDrake2406 wants to merge 7 commits intocloudflare:mainfrom
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
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)