Skip to content

fix(build): preserve lazy RSC framework chunks - #2074

Merged
james-elicx merged 6 commits into
mainfrom
codex/fix-rsc-framework-lazy-chunks
Jun 16, 2026
Merged

fix(build): preserve lazy RSC framework chunks#2074
james-elicx merged 6 commits into
mainfrom
codex/fix-rsc-framework-lazy-chunks

Conversation

@james-elicx

Copy link
Copy Markdown
Member

Summary

  • preserve dynamic entry boundaries within the Vite 8/Rolldown RSC framework chunk group
  • prevent lazy react-dom/server.edge stubs from being evaluated at Worker startup
  • retain the framework splitting needed for global-not-found CSS ordering
  • add production regression coverage for importing the built RSC entry

Root cause

Rolldown's framework group merged every matching React module into one chunk by default. A lazy import("react-dom/server.edge") could therefore place React's throwing server.react-server.js stub in the same framework chunk statically imported by the RSC entry.

Setting entriesAware: true keeps matching framework modules grouped by the entry points that actually use them. Lazy-only React modules remain behind their dynamic import instead of moving onto the Worker startup path.

Next.js comparison

Next.js does not create its client-style framework chunk for App Router server builds. It also aliases legacy react-dom/server.edge imports in server-component builds. This change addresses vinext's additional eager-chunk regression without attempting to resolve the separate RSC export-condition behavior tracked in #1237 and #1891.

Validation

  • vp test run tests/build-optimization.test.ts tests/app-router-global-not-found-css-order.test.ts — 116 tests passed
  • vp check — formatting, lint, and type checks passed
  • rebuilt vinext and examples/fumadocs-docs-template
  • verified the throwing React stub is emitted in a separate lazy framework subgroup
  • verified importing dist/server/index.js does not throw
  • verified wrangler deploy --dry-run --no-bundle accepts the Fumadocs artifact

Fixes #2073

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@pkg-pr-new

pkg-pr-new Bot commented Jun 16, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/@vinext/cloudflare@2074
npm i https://pkg.pr.new/vinext@2074

commit: adc1287

@github-actions

Copy link
Copy Markdown
Contributor
Example Preview Production Original
app-router-cloudflare preview production
pages-router-cloudflare preview production
app-router-playground preview production original
realworld-api-rest preview production
nextra-docs-template preview production
benchmarks preview production
hackernews preview production original
workers-cache preview production
web preview production

@ask-bonk

ask-bonk Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

@james-elicx Bonk workflow was cancelled.

View workflow run · To retry, trigger Bonk again.

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

1 similar comment
@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@ask-bonk

ask-bonk Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

/bigbonk review

Reviewed the fix end-to-end, including reproducing the regression locally. This is a correct, well-targeted fix and I recommend merging.

Verification performed

  • Confirmed entriesAware is a real, documented rolldown codeSplitting group option in the bundled @voidzero-dev/vite-plus-core Vite 8 build (define-config-*.d.mts, line 1135), with semantics matching the PR description: "matching modules are grouped by which entries actually import them."
  • Reverted entriesAware: true locally and confirmed the regression reproduces — the built framework-*.js chunk evaluates React's throwing react-server stub (Error: react-dom/server is not supported in React Server Components), matching the worker-startup crash in vinext 0.1.1 regression: RSC framework-chunk split makes a lazy react-dom/server import eager → Cloudflare Worker deploy fails (code: 10021) #2073.
  • Restored the fix and confirmed all three production tests pass, plus createRscFrameworkChunkOutputConfig unit tests and the full static-image-emission.test.ts suite. vp check is clean on the modified source.
  • Confirmed the original App Router: CSS ordering wrong for global-not-found.tsx #1549 CSS-ordering guarantee still holds (route-miss 404 test passes) — entriesAware keeps the CSS-free framework chunk split intact.

Findings

1. The new dedicated regression test does not actually catch the regression on its own (test design weakness).

When I reverted the fix, the suite failed at beforeAllpreview() (line 87), because the preview server eagerly evaluates the framework chunk before any it() runs. The new it("does not eagerly evaluate dynamically imported React server stubs") block (line 118) was reported as skipped, not failed — it never executed because setup crashed first.

So the regression is caught by this suite, but via the beforeAll, not the dedicated test. The dedicated test only adds value if it can fail independently. As written, importing dist/server/index.js after a successful preview() boot is somewhat redundant with what preview() already proves. Consider either:

  • Building the RSC entry in a standalone test that does not stand up a preview server, so the import() assertion is the sole thing exercising the eager-evaluation path, or
  • Adding a comment noting that preview() is the primary guard and this it() is a defense-in-depth check on direct entry import.

This is a polish suggestion, not a blocker — the regression is covered.

2. react-dom/server.edge lazy import is an RSC-export-condition concern (acknowledged in PR).

The PR correctly scopes out the separate react-dom/server.edge resolution behavior (#1237/#1891) and only addresses the eager-chunk regression. Good call keeping the scope tight.

Non-issues confirmed

  • static-image-emission.test.ts changes are unrelated flakiness hardening: polling for the expected watch output instead of resolving on the first END, with a checkingOutput reentrancy guard and proper off() cleanup. Logic is correct; if isExpectedOutput() never returns true the 20s timeout still rejects as before. Suite passes.

Nice fix. The root-cause analysis matches #2073 exactly and the entriesAware mechanism is the right tool.

@ask-bonk

ask-bonk Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Posted my review to PR #2074. Summary of findings:

Verdict: Correct, well-targeted fix — recommend merging.

What I verified:

Findings:

  1. Test design weakness (non-blocking): When I reverted the fix, the suite failed at beforeAll/preview(), and the new dedicated it("does not eagerly evaluate...") test was reported as skipped, never executing. The regression is caught — but via setup, not the dedicated test. The import(dist/server/index.js) assertion is somewhat redundant with what preview() already proves. Suggested making it a standalone test (no preview server) or documenting it as defense-in-depth.
  2. Scope (acknowledged in PR): Correctly defers the separate react-dom/server.edge RSC export-condition behavior (Error: react-dom/server is not supported in React Server Components. #1237/Discussion: route handlers need a dedicated Vite env to fix #1237 + #1246 #1891).

Non-issues confirmed: the static-image-emission.test.ts watch-mode changes are sound flakiness hardening (poll for expected output, reentrancy guard, proper off() cleanup, timeout fallback preserved).

github run

@james-elicx

Copy link
Copy Markdown
Member Author

Addressed the test-design finding in a655ef5. The production build now completes in beforeAll, but preview startup is lazy and only happens inside the CSS tests. This guarantees the direct dist/server/index.js import test runs independently before any preview server evaluation. I also temporarily removed entriesAware: true to verify the negative case: the named does not eagerly evaluate dynamically imported React server stubs test now fails directly with react-dom/server is not supported in React Server Components instead of being skipped during setup. Restored the fix; focused tests and vp check pass.

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@ask-bonk

ask-bonk Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

/bigbonk review

The fix is correct and well-targeted: entriesAware: true keeps lazy-only React modules (e.g. the throwing react-dom/server.edgeserver.react-server.js stub) out of the framework chunk that's statically imported on the Worker startup path. I confirmed the entriesAware semantics against the bundled rolldown types (define-config-*.d.mts:1110-1128) and they match the PR description. One nice property worth calling out: entriesAwareMergeThreshold defaults to 0, so the small lazy framework subgroup won't get merged back into the main chunk — the fix is robust against that.

ask-bonk's earlier review and the author's a655ef5 follow-up already covered the test-design concern, so I focused on what's left. Two findings, neither a blocker.

1. The Vite 7 manualChunks branch still has the exact bug this PR fixes (latent)

packages/vinext/src/build/client-build-config.ts:232-235:

return {
  manualChunks(id: string): string | undefined {
    return isRscFrameworkModule(id) ? "framework" : undefined;
  },
};

This unconditionally assigns every matching framework module to a single "framework" chunk regardless of which entry imports it — i.e. the "merge everything" behavior that entriesAware: false produces and that #2073 is caused by. Vite 7 is still a supported branch (index.ts:1985 explicitly handles the Vite 7 esbuild path), so on Vite 7 the lazy react-dom/server.edge stub can still land in the eager framework chunk and re-introduce the worker-startup crash.

The repo runs Vite 8 today so this isn't a live regression, but it's a real parity gap. Rollup's manualChunks has no entriesAware equivalent, so a true fix may not be trivial — at minimum, add a comment noting the Vite 7 path does not get the lazy-chunk isolation, and ideally a tracking issue. The new regression test only exercises the Vite 8 path, so this gap is invisible to CI.

2. The entriesAware: true line carries no explanation of why

client-build-config.ts:211-217 — the doc comment on createRscFrameworkChunkOutputConfig still only references the #1549 CSS-ordering motivation. The newly added entriesAware: true is the entire substance of this PR (preventing the #2073 eager-evaluation regression), but a future maintainer reading the function sees an unexplained boolean and could plausibly "simplify" it away while keeping the CSS split working in the happy path. A one-line comment next to entriesAware: true linking #2073 would prevent that, and would also flag the asymmetry with the Vite 7 branch from finding 1.

Minor / non-issues confirmed

  • Fixture top-level globalThis mutation (tests/fixtures/global-not-found-css-order/app/page.tsx): assigning __loadReactDomServerForChunkingTest at module scope is a slight smell but is the right way to keep the dynamic import("react-dom/server.edge") reachable (so it isn't tree-shaken) without eagerly evaluating it. Fine as-is.
  • static-image-emission.test.ts watch changes: the reentrancy guard (checkingOutput), off() cleanup, and preserved 20s timeout reject are all correct; if isExpectedOutput() never returns true the timeout still fires. Sound flakiness hardening.
  • Test ordering: the new it("does not eagerly evaluate...") relies on running before startPreviewServer() is first called. Vitest runs it blocks in declaration order, so the guarantee holds, but it is an implicit ordering dependency.

Verdict: correct fix, recommend merging after considering a comment for the Vite 7 gap.

@ask-bonk

ask-bonk Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Posted my review to PR #2074. Summary:

Verdict: Correct, well-targeted fix — recommend merging.

I confirmed entriesAware semantics against the bundled rolldown types (define-config-*.d.mts:1110-1128) and noted that entriesAwareMergeThreshold defaults to 0, so the lazy framework subgroup won't be merged back into the eager chunk — the fix is robust.

Since ask-bonk and the author already resolved the test-design concern, I focused on what was left. Two non-blocking findings:

  1. Vite 7 manualChunks branch still has the exact vinext 0.1.1 regression: RSC framework-chunk split makes a lazy react-dom/server import eager → Cloudflare Worker deploy fails (code: 10021) #2073 bug (latent). client-build-config.ts:232-235 unconditionally assigns every framework module to one chunk regardless of entry — the "merge everything" behavior that causes the crash. Vite 7 is still a supported branch (index.ts:1985), so on Vite 7 the regression returns. Not live today (repo runs Vite 8), and Rollup has no entriesAware equivalent, so I recommended at minimum a comment + tracking issue. The new regression test only covers Vite 8.

  2. entriesAware: true has no explanatory comment. The function's doc comment still only references the App Router: CSS ordering wrong for global-not-found.tsx #1549 CSS motivation; the new line is the whole substance of the PR but a maintainer could "simplify" it away. A one-line comment linking vinext 0.1.1 regression: RSC framework-chunk split makes a lazy react-dom/server import eager → Cloudflare Worker deploy fails (code: 10021) #2073 would prevent that.

Confirmed as non-issues: the globalThis fixture mutation (correct way to keep the dynamic import reachable without eager eval), the static-image-emission watch hardening (reentrancy guard + off() cleanup + timeout preserved), and the implicit-but-valid it-ordering dependency in the new test.

github run

@james-elicx

Copy link
Copy Markdown
Member Author

Addressed both findings in 4e5d25c. The Vite 7 manualChunks path now uses Rollup’s getModuleInfo() graph to assign a framework module only when it is statically reachable from a real entry. Framework modules reachable only through dynamic imports are left to automatic chunking, which mirrors the relevant entriesAware behavior instead of merely documenting the gap. Added Vite 7 unit coverage for eager React/RSC modules versus a lazy react-dom/server.react-server.js module. Also documented the #2073 reason next to entriesAware: true and in the function comment. Full tests/build-optimization.test.ts, the production startup/CSS regression suite, and vp check pass.

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@ask-bonk

ask-bonk Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

/bigbonk review

Reviewed and verified the fix end-to-end, including the negative case. This is a correct, well-targeted fix and I recommend merging.

Verification performed

  • Reproduced the negative case for the dedicated test. Commented out entriesAware: true and ran tests/app-router-global-not-found-css-order.test.ts -t "does not eagerly evaluate". The dedicated it("does not eagerly evaluate dynamically imported React server stubs") test now fails directly at line 111 with Error: react-dom/server is not supported in React Server Components (thrown from dist/server/_next/static/framework-*.js), while the two preview-based tests are skipped. This confirms the a655ef5 test-isolation fix works: the regression is caught by the dedicated direct-import() assertion before any preview server boots, exactly as the author described. Restored entriesAware: true afterward.
  • Confirmed all tests pass with the fix in place: full tests/build-optimization.test.ts (113 passed) and tests/app-router-global-not-found-css-order.test.ts (3 passed).
  • Confirmed the codeSplitting under output shape is unchanged from main — the only Vite 8 change is the additive entriesAware: true line, so there is no risk of changing the existing App Router: CSS ordering wrong for global-not-found.tsx #1549 CSS-split behavior on the happy path.
  • Traced the Vite 7 isStaticallyReachableFromEntry logic against Rollup's ModuleInfo semantics (importers = static importers only; dynamic importers live on the separate dynamicImporters field). The DFS correctly returns false for a framework module reachable only through a dynamically-imported intermediate (entry --dynamic--> mid --static--> reactmid.importers is empty → not reachable), which mirrors entriesAware behavior. The visited set guards against import cycles.

Findings (all non-blocking)

1. Vite 7 transitive static-reachability path is untested. The new Vite 7 unit test (build-optimization.test.ts:2825-2848) only covers single-hop cases: react/index.js imported directly by the entry, and react-dom/server.react-server.js imported by a lazy module with empty importers. The recursive branch in isStaticallyReachableFromEntry (client-build-config.ts:231-233) — a framework module reached via a chain of static importers (entry → middleman → react) — is never exercised. Consider adding a multi-hop case so the recursion can't silently break. Minor, since the Vite 8 path is what ships today.

2. meta parameter typing relies on Rollup always passing the second arg. createRscFrameworkChunkOutputConfig's Vite 7 manualChunks(id, meta) assumes meta.getModuleInfo is always present. Rollup does always pass { getModuleInfo, getModuleIds }, so this is correct — just noting the implicit dependency for future readers.

Non-issues confirmed

  • static-image-emission.test.ts watch changes are sound flakiness hardening: the checkingOutput reentrancy guard prevents overlapping async output polls, off() is properly invoked in cleanup(), and the 20s timeout reject is preserved. If isExpectedOutput() returns false, the promise correctly stays pending until a later END or the timeout fires.
  • Fixture top-level globalThis mutation (tests/fixtures/global-not-found-css-order/app/page.tsx) is the right way to keep the dynamic import("react-dom/server.edge") reachable (so it isn't tree-shaken) without eagerly evaluating it.
  • react-dom/server.edge RSC export-condition behavior is correctly scoped out (deferred to Error: react-dom/server is not supported in React Server Components. #1237/Discussion: route handlers need a dedicated Vite env to fix #1237 + #1246 #1891).

Nice fix — the root-cause analysis matches #2073, the entriesAware mechanism is the right tool, and the test now fails for the right reason when the fix is removed.

@ask-bonk

ask-bonk Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Posted my review to PR #2074.

Verdict: Correct, well-targeted fix — recommend merging.

Key verification I performed beyond the prior reviews:

  • Reproduced the negative case for the dedicated test specifically. I commented out entriesAware: true and confirmed the new it("does not eagerly evaluate dynamically imported React server stubs") test now fails directly at line 111 with react-dom/server is not supported in React Server Components (thrown from the built framework-*.js), while the preview-based tests are skipped. This validates the author's a655ef5 test-isolation fix — the regression is caught by the direct import(dist/server/index.js) assertion before any preview boots.
  • Confirmed all tests pass with the fix restored (113 in build-optimization, 3 in the CSS-order suite).
  • Confirmed the codeSplitting-under-output shape is unchanged from main, so the only Vite 8 change is the additive entriesAware: true.
  • Traced the Vite 7 isStaticallyReachableFromEntry DFS against Rollup's ModuleInfo.importers (static-only) semantics — it correctly excludes framework modules reachable only via a dynamically-imported intermediate, and the visited set guards cycles.

Two non-blocking findings:

  1. The Vite 7 unit test only covers single-hop reachability; the recursive transitive branch (entry → middleman → react) is untested.
  2. The meta param typing relies on Rollup always passing the second arg (it does — just an implicit dependency worth noting).

Confirmed as non-issues: the static-image-emission watch hardening, the fixture globalThis mutation, and the deferred react-dom/server.edge export-condition scope.

I left the working tree clean (restored entriesAware: true after the negative-case verification).

github run

@james-elicx

Copy link
Copy Markdown
Member Author

Addressed the remaining coverage finding in e0335f5. The Vite 7 manualChunks unit graph now includes a multi-hop static path (entry -> middleman -> react/index.js), so the recursive isStaticallyReachableFromEntry branch is exercised directly while the lazy-only React DOM stub remains excluded. Focused chunking tests and targeted vp check pass.

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@ask-bonk

ask-bonk Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

/bigbonk review

Reviewed end-to-end and reproduced the negative case for the dedicated test. Correct, well-targeted fix — recommend merging. Prior bonk reviews and the author's a655ef5/4e5d25c/e0335f5 follow-ups already resolved the substantive findings (test isolation, Vite 7 parity, comments, transitive coverage). I verified those are genuinely fixed rather than just claimed.

Verification performed

  • entriesAware semantics confirmed against the shipped bundler. It is a real rolldown codeSplitting group option in the bundled rolldown 1.0.0 types (define-config-5HJ1b9vG.d.mts:1128), @default false. The adjacent entriesAwareMergeThreshold is @default 0 (no subgroup merging), so the lazy framework subgroup will not be merged back onto the eager startup path — the fix is robust against that.
  • Reproduced the negative case for the dedicated test specifically. Set entriesAware: false and ran tests/app-router-global-not-found-css-order.test.ts -t "does not eagerly evaluate". The dedicated it(...) now fails directly at line 111 with react-dom/server is not supported in React Server Components (thrown from the built framework-*.js), while the two preview-based tests are skipped. This confirms a655ef5 works: the regression is caught by the direct import(dist/server/index.js) assertion before any preview server boots. Restored entriesAware: true afterward (working tree left clean).
  • All tests pass with the fix in place: tests/app-router-global-not-found-css-order.test.ts (3 passed), createRscFrameworkChunkOutputConfig unit tests (2 passed).
  • Traced the Vite 7 isStaticallyReachableFromEntry DFS against Rollup's ModuleInfo.importers (static-only; dynamic importers live on the separate dynamicImporters field). It correctly returns false for a framework module reachable only via a dynamically-imported intermediate, and the multi-hop unit case (entry → middleman → react/index.js) added in e0335f5 exercises the recursion. The shared visited set only guards cycles and does not introduce false negatives for diamond graphs (.some() still explores every branch; any real static path to an entry is found).

Findings (all minor / non-blocking)

1. getPackageName is not Windows-safe, but the Vite 7 framework regex is — slight asymmetry. The Vite 8 path matches via RSC_FRAMEWORK_CHUNK_TEST (uses [\\/], Windows-safe). The Vite 7 path routes through isRscFrameworkModulegetPackageName, which splits on / only (client-build-config.ts:72-75). On Windows-style paths the Vite 7 predicate could miss. This is pre-existing (the old manualChunks already used isRscFrameworkModule), not introduced here — flagging only because this PR touches the same path. Not a blocker.

2. The dedicated test relies on an implicit it-ordering dependency. does not eagerly evaluate... must run before startPreviewServer() is first called. Vitest runs it blocks in declaration order so the guarantee holds today, but it is implicit. The verified negative-case behavior (other tests skip when this one throws) is acceptable. A one-line comment noting the ordering requirement would harden it against future reordering.

Non-issues confirmed

Nice fix. Root-cause analysis matches #2073, entriesAware is the right tool, and the test now fails for the right reason when the fix is removed.

@ask-bonk

ask-bonk Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Posted my review to PR #2074.

Verdict: Correct, well-targeted fix — recommend merging.

I verified the fix end-to-end rather than relying on prior reviews:

  • Confirmed entriesAware against the shipped bundler — it's a real rolldown codeSplitting option (define-config-5HJ1b9vG.d.mts:1128, @default false), and entriesAwareMergeThreshold defaults to 0, so the lazy framework subgroup won't be merged back onto the Worker startup path.
  • Reproduced the negative case for the dedicated test specifically — set entriesAware: false and confirmed it("does not eagerly evaluate...") fails directly at line 111 with react-dom/server is not supported in React Server Components, while the preview-based tests skip. This validates the author's a655ef5 test-isolation fix. Restored the fix and left the tree clean.
  • All targeted tests pass with the fix (3 CSS-order + 2 unit).
  • Traced the Vite 7 isStaticallyReachableFromEntry DFS against Rollup's static-only ModuleInfo.importers — correct for dynamic-only intermediates, with multi-hop coverage from e0335f5, and the shared visited set introduces no false negatives in diamond graphs.

Two minor, non-blocking findings:

  1. getPackageName is not Windows-safe while the Vite 8 regex is — but this is pre-existing, not introduced by the PR.
  2. The dedicated test has an implicit it-ordering dependency (must run before the preview server boots); holds today, a comment would harden it.

Confirmed as non-issues: the static-image-emission watch hardening, the fixture globalThis mutation, the deferred react-dom/server.edge scope, and the unchanged output shape preserving #1549.

github run

@james-elicx

Copy link
Copy Markdown
Member Author

Addressed both minor findings in adc1287. getPackageName() now normalizes Windows separators before extracting package names, with Windows-path framework matching coverage. The production regression suite now explicitly guards preview startup behind successful completion of the direct RSC-entry import assertion, so the test-order dependency is enforced rather than implicit. Focused suites (116 tests) and targeted vp check pass.

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

1 similar comment
@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@ask-bonk

ask-bonk Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

/bigbonk review

Reviewed end-to-end and independently reproduced the negative case. Correct, well-targeted fix — recommend merging. The prior bonk reviews plus the author's a655ef5/4e5d25c/e0335f5/adc1287 follow-ups resolved every substantive finding (test isolation, Vite 7 parity, comments, transitive coverage, Windows paths, enforced test ordering). I verified those are genuinely fixed rather than just claimed, and looked for anything still open.

Verification performed

  • Reproduced the negative case for the dedicated test. Flipped entriesAware: false and ran tests/app-router-global-not-found-css-order.test.ts -t "does not eagerly evaluate". The dedicated it(...) failed directly at line 115 with react-dom/server is not supported in React Server Components (thrown from the built framework-*.js), while the two preview-based tests were skipped. This confirms a655ef5 + the adc1287 startupImportValidated guard work as intended: the regression is caught by the direct import(dist/server/index.js) assertion before any preview server boots. Restored entriesAware: true (working tree left clean).
  • All targeted tests pass with the fix in place: tests/app-router-global-not-found-css-order.test.ts (3 passed), createRscFrameworkChunkOutputConfig + RSC framework package matching unit tests (all green). vp check clean on client-build-config.ts.
  • Single-entry RSC build confirmed. The RSC environment builds with input: { index: VIRTUAL_RSC_ENTRY } (index.ts:2221) — a single entry. So entriesAware keeps all eager React in one CSS-free framework chunk (preserving App Router: CSS ordering wrong for global-not-found.tsx #1549) while isolating the lazy-only react-dom/server.edgeserver.react-server.js stub into a separate subgroup. The route-miss 404 test passing proves the App Router: CSS ordering wrong for global-not-found.tsx #1549 split still holds.
  • Audited the Vite 7 isStaticallyReachableFromEntry DFS against Rollup's ModuleInfo contract (importers = static-only; dynamic importers live on the separate dynamicImporters field). Worked through the shared-visited concern: reachability is an intrinsic property of a node, not of the traversal path, so a node first visited on a dead-end branch correctly memoizes false and cannot produce a false negative on a later path (its own subtree would have found the entry on first visit). The multi-hop unit case (entry → middleman → react/index.js) added in e0335f5 exercises the recursion. No false negatives in diamond graphs.

Findings (all minor / non-blocking)

1. The RSC build does not set experimentalMinChunkSize, but if it ever does, the Vite 7 lazy subgroup could merge back onto the eager path. The Vite 8 path is robust here because entriesAwareMergeThreshold defaults to 0. The Vite 7 path returns undefined for lazy-only framework modules and relies on Rollup's default auto-chunking to keep them behind the dynamic import — which it does today (the client build's experimentalMinChunkSize: 10_000 is scoped to the client output, not this RSC output). This is fine as shipped; flagging only so a future change that adds a min-chunk-size to the RSC output is reviewed against the #2073 regression, which the Vite-8-only regression test would not catch.

2. react-server-dom-webpack is matched by FRAMEWORK_PACKAGES but is RSC-only. It's correctly grouped into framework on both paths and is statically reachable from the RSC entry, so behavior is right. No action needed — noting only that the framework list is shared with the client build where react-server-dom-webpack would never appear, which is harmless.

Non-issues confirmed

  • static-image-emission.test.ts watch changes are sound flakiness hardening: the checkingOutput reentrancy guard prevents overlapping async polls, off() is invoked in cleanup(), and the 20s timeout reject is preserved. A dropped END during an in-flight check is harmless because the poll reads current filesystem state and the timeout is the backstop.
  • Fixture top-level globalThis mutation (tests/fixtures/global-not-found-css-order/app/page.tsx) is the right way to keep the dynamic import("react-dom/server.edge") reachable (top-level global assignment is a preserved side effect, so it isn't tree-shaken) without eagerly evaluating it. Verified the lazy import survives bundling (the entry imports without throwing).
  • getPackageName Windows normalization (replaceAll("\\", "/")) plus the [\\/] separator in RSC_FRAMEWORK_CHUNK_TEST are covered by the new C:\app\node_modules\react-dom\server.js case in both predicate tests.
  • react-dom/server.edge RSC export-condition behavior is correctly scoped out (deferred to Error: react-dom/server is not supported in React Server Components. #1237/Discussion: route handlers need a dedicated Vite env to fix #1237 + #1246 #1891).
  • createRscFrameworkChunkOutputConfig shape under output is unchanged from main apart from the additive entriesAware: true, so the App Router: CSS ordering wrong for global-not-found.tsx #1549 happy-path CSS split is preserved.

Nice fix. Root-cause analysis matches #2073, entriesAware is the right tool, the Vite 7 path mirrors it rather than just documenting the gap, and the dedicated test fails for the right reason when the fix is removed.

@ask-bonk

ask-bonk Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Posted my review to PR #2074.

Verdict: Correct, well-targeted fix — recommend merging.

I verified the fix end-to-end independently rather than relying on prior reviews:

  • Reproduced the negative case for the dedicated test specifically — flipped entriesAware: false and confirmed it("does not eagerly evaluate...") fails directly at line 115 with react-dom/server is not supported in React Server Components, while the preview-based tests skip. This validates both the a655ef5 test isolation and the adc1287 startupImportValidated ordering guard. Restored the fix; left the tree clean.
  • All targeted tests pass with the fix (3 CSS-order + the unit suites). vp check clean.
  • Confirmed the RSC build is single-entry (input: { index }), so entriesAware keeps eager React in one CSS-free chunk (preserving App Router: CSS ordering wrong for global-not-found.tsx #1549) while isolating the lazy stub — the route-miss 404 test proves App Router: CSS ordering wrong for global-not-found.tsx #1549 still holds.
  • Audited the Vite 7 DFS against Rollup's static-only ModuleInfo.importers contract and worked through the shared-visited concern: reachability is an intrinsic node property, so no false negatives in diamond graphs. Multi-hop recursion is covered by e0335f5.

Two minor, non-blocking findings:

  1. The Vite 7 lazy-subgroup isolation relies on Rollup default auto-chunking since the RSC output sets no experimentalMinChunkSize — fine today, but a future min-chunk-size on that output wouldn't be caught by the Vite-8-only regression test.
  2. react-server-dom-webpack shares the framework list with the client build (harmless, RSC-only).

Confirmed as non-issues: the static-image-emission watch hardening, the fixture globalThis side-effect mutation (verified the lazy import survives bundling), the Windows getPackageName normalization coverage, the deferred react-dom/server.edge export-condition scope, and the unchanged output shape preserving #1549.

github run

@james-elicx
james-elicx merged commit 2f8b0ef into main Jun 16, 2026
56 checks passed
@james-elicx
james-elicx deleted the codex/fix-rsc-framework-lazy-chunks branch June 16, 2026 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

vinext 0.1.1 regression: RSC framework-chunk split makes a lazy react-dom/server import eager → Cloudflare Worker deploy fails (code: 10021)

1 participant