fix(federation): startup and export propagation through MF async startup; RSC edge cases; review cleanups - #134
Conversation
…ync startup (#132) Browser: React Router imports each browser route-module entry synchronously and reads its exports; under MF asyncStartup the entry's startup is a promise and Rspack's module-library export snapshotted it (`startup.default` -> undefined), so hydration never ran. Route-module entries (and entry.client) are now made async via a post-SWC transform, so Rspack emits `(await startup).default` and importers wait for evaluation. Each federation container also gets its own runtime chunk: sharing the app's single runtime made importing the container run the app entries' share consumes before the host initialized the share scope, yielding a second React. Server: @module-federation/node replaces Rspack's readFileVm loader with one that tracks loaded chunks privately, so Rsbuild's initial split chunks on a multi-entry server build never satisfied __webpack_require__.O and the awaited server build resolved to undefined. Server code splitting is now async-only under federation. asyncStartup stays enforced on every compiler; shared dependencies stay non-eager. RSC: manifest alignment rebases relative references from an empty browser assetPrefix; filename validation moves to the emitted web output (entry files and client chunks, function filenames, tools.rspack overrides). Tests: federation fixture is a real pass (awaited valid Node ServerBuild, SSR of the remote over HTTP, hydration, container from another origin, lazy JS/CSS via 'auto', CORS); real-config suite asserts asyncStartup on both compilers, non-eager shared, isolated container runtime, async-only server split chunks, and node-only environment selection (compiler closed); RSC output-filename acceptance builds; RSC browser test uses the shared observer with no .js exemption and a styled initial client component; dead jsDistPath inputs and the rsbuild-inspect subprocess step removed; owned tests included in the tests typecheck. Example: CORS scoped to the remote's asset handlers.
A route with only server exports compiles to an empty browser module; the
async-entry transform now appends `export {}` alongside the top-level await
so Rspack parses it as an ES module. Covered by a server-only route in the
federation fixture; both Epic Stack federation apps build again.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
commit: |
Benchmark results
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac44e3fea1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Merging this PR will not alter performance
Comparing Footnotes
|
ScriptedAlchemy
left a comment
There was a problem hiding this comment.
Review of ac44e3fe: two targeted corrections before approval
I independently checked the nine green checks and read the successful build-test log (run). It reports 675 passing unit tests, successful publint, 1034 passing / 45 skipped in the non-HMR ecosystem invocation, successful HMR invocations, and successful example E2E suites. The federation scenario is actually executed, not skipped. I am not treating that verification as missing or asking to redo the already-covered defaults.
The main federation changes are coherent and should stay: mark browser entry/route modules async so ESM exports await startup, isolate container runtimes from the application's shared runtime, and remove unsupported initial Node chunk dependencies. The fixture retains experiments.asyncStartup: true on all four compiler configurations and non-eager sharing, renders the remote on the server, hydrates, and loads the lazy remote CSS from the relocated asset origin. No eager-sharing or disabled-startup workaround is acceptable.
Remaining findings
Both existing inline findings are valid; I replied in their current threads rather than creating duplicates:
-
P2 — The Node async-only invariant is not enforced on the effective split configuration. A late
tools.rspackoverride can restorechunks: 'all'. More importantly, the ordinary Rsbuildsingle-vendorpreset creates a cache group withchunks: 'all'andenforce: trueeven while the parent sayschunks: 'async'. That explicit group filter wins in Rspack. Moving only the parent assignment later does not fix the preset case. Complete this at one final configuration/dependency boundary, preservingsplitChunks: falseand either preventing or clearly rejecting unsupported initial dependencies. Add the preset to the existing real-config test and reuse the existing Node fixture for a representative case. -
P2 — The emitted RSC validator still misses arbitrary script filenames.
isManifestJsAsset(file)returns false for emitted JavaScript named.bundle,.jsx, or without an extension, so the new validator accepts names the native RSC manifest drops. Identify JS by compilation/render metadata and then require the emitted reference to end in.js; do not expand a filename-extension blacklist. One additional case in the existing RSC filename fixture is sufficient.
I executed isolated copies of the current collector and pinned Rsbuild/Rspack configuration logic to confirm those predicate/precedence gaps. They are source-level counterexamples, not a claim that I reran the repository's full builds or browser suites locally. Container DNS prevents obtaining the dependencies here.
Scope and simplification
The cleanup requested on #131 is materially addressed: the private RSC request collector/error exemption is gone, initial client-reference CSS is exercised, the redundant browser inspect subprocess is removed, compiler cleanup is in finally, dead jsDistPath inputs are removed, and CORS is scoped to asset responses. Keep the small real-config fixture and the single end-to-end federation scenario; do not reintroduce the fake Rsbuild replay machinery or duplicate HTTP passes.
Manifest-based remote loading, the upstream Node installed-chunks report, #133's install-layout investigation, and repo-wide typecheck debt can remain separate. I am not adding them as merge conditions. The two findings above are completion of the new invariants, not a request for another architectural rewrite.
… boundary; classify RSC scripts by chunk metadata - enforceAsyncOnlyServerSplitChunks runs in the final tools.rspack callback (after user overrides) and sets chunks: 'async' on the global filter and on every cache group that declares one (Rsbuild's single-vendor preset adds an enforced chunks: 'all' group that Rspack prefers over the global filter); a disabled splitChunks stays disabled. The environment-config-stage assignment is removed. Real-config tests cover a late tools.rspack override, the single-vendor preset, and splitChunks: false. - collectUnsupportedRscScriptAssets identifies JavaScript-emitting chunks from compilation metadata (javascript content hash / modules) and derives each chunk's emitted script name from its own filename template (entry or async; string or function) via compilation.getPath, then applies the plain-.js rule -- so scripts under .txt or no extension are reported and CSS-only chunks are ignored. Integration cases now use a filename function for the entry and a .txt async chunkFilename via tools.rspack.
|
Both addressed in 1. Async-only server splitting at the final boundary. 2. RSC scripts identified from chunk metadata. On the non-blocking |
Fixes #132 (Module Federation production startup) and addresses the four inline findings from the post-merge review of #131.
experiments.asyncStartupstays enforced on every compiler and shared dependencies stay non-eager throughout; nothing here bypasses async startup.Module Federation (#132)
Three defects, each individually necessary (removing any one fails the fixture):
import * as route0 from ".../root.js") and reads its exports; under async startup the entry's startup is a promise, and Rspack's module-library render didvar s = startup.default→undefinedforever, so hydration never ran (no fibers, no remote request, no errors). The plugin now makes browser route-module entries (andentry.client) async via a post-SWC transform, so Rspack emits(await startup).defaultand importers wait for evaluation.export {}is appended so a route with only server exports (empty client module) still parses as ESM.runtime-<name>); app entries keep the shared one.undefined.@module-federation/nodereplaces Rspack'sreadFileVmloader with one that tracks loaded chunks in a private map, so the initial chunks Rsbuild splits off a multi-entry server build (chunks: 'all') never satisfy__webpack_require__.O's gate. Server code splitting is now async-only under federation, so the async startup has no initial chunk dependencies and resolves to the real exports.federation-test.tsis a real pass now (nofixme): awaited valid NodeServerBuild, SSR of the remote over HTTP from an isolated filesystem, browser hydration, container from another origin + sub-path, exposed component's lazy JS/CSS via'auto'(origin A refuses the CSS so a baked prefix fails), CORS. The real-config suite asserts async startup on both compilers, sharing left as declared, the isolated container runtime, async-only server split chunks, and node-only environment selection (compiler closed).Review findings on the merged #131
assetPrefix: ''; absolute and protocol-relative URLs are untouched (unit-tested).output.filename.jsstring check with validation of the emitted web output (entry files and client chunks), so function filenames andtools.rspackchunkFilenameoverrides are covered.rsc-output-filename-test.tsbuilds real RSC fixtures: query-hash entry and.mjschunkFilenameviatools.rspackfail with the message; hashed.jspasses..jsexemption; the initially rendered client component now has its own stylesheet (client-manifestcssFilespath) verified before hydration.setHeaderson the static mounts; dev path gated to/static/and the MF manifest).Cleanups: dead
jsDistPathtest inputs removed;rsbuild inspectsubprocess step removed from the browser-prefix scenarios (real-config suite coverspublicPath: 'auto'); environment-selection folded into the real-config fixture; owned tests included intsconfig.tests.json(widening to all tests surfaces ~200 pre-existing errors — left for a separate cleanup); README states the classic/RSC filename-support distinction and the federation requirements.Verification
675 unit; typecheck + prettier clean; React Router integration 1032 passed / 45 skipped on the full run (5 fixtures failed only because I rebuilt
dist/concurrently — re-run green: 54 passed); all 8 example e2e suites; both Epic Stack federation apps build (remote emitsruntime-remote.*.js, server build has no initial numbered chunks). Negative controls: forced webpublicPathstill fails the federation and'auto'relocation scenarios; each of the three federation fixes removed individually fails the fixture (NC3 reproduces the exactServerBuilderror).Follow-ups not in this PR: manifest-based (
mf-manifest.json) remote loading coverage; an upstream report to@module-federation/nodeabout its chunk loader not marking Rspack'sreadFileVminstalled-chunks map; the repo-wide test typecheck debt.