Optimize dev rebuild defaults - #86
Conversation
commit: |
Benchmark ResultsCompared PR head Reading benchmark confidenceRaw deltas are always shown. The signal label only indicates whether the observed median delta is larger than a robust run-to-run noise band; it does not erase or replace the measurement. The noise band is the larger of 2% or two combined robust standard deviations estimated from each side's relative median absolute deviation (rMAD). Fewer than three finite samples is reported as insufficient data. An inconclusive result should be rerun or investigated from the uploaded raw samples before drawing a performance conclusion.
Dev Rollup
Production Build BenchmarksRendered 7 production build benchmarks.
ci-small+ci-large Dev Fixture SummaryRendered 7 dev benchmark fixtures from the
large-355-ssr-esm Plugin Operations
synthetic-1024-ssr-esm Plugin Operations
synthetic-1024-ssr-esm-split Plugin Operations
synthetic-256-sourcemaps Plugin Operations
synthetic-256-ssr-esm Plugin Operations
synthetic-256-ssr-esm-split Plugin Operations
synthetic-48-ssr-esm Plugin Operations
Synthetic Rsbuild AppRendered 2 production build benchmarks.
Rendered 1 dev benchmark fixture from the embedded complex app.
Profile: |
5b459a4 to
d09dfb1
Compare
|
The current PR description appears stale in two places: the head keeps The latest CI comparison is still useful evidence for this work: update/HMR improves from 2.92 s to 2.63 s (-10.1%) across the seven dev fixtures, 1.72 s to 1.55 s (-9.9%) on the large fixture, and 3.37 s to 2.43 s (-27.9%) on the complex synthetic app. Ready time and several standard-fixture totals regress (all-fixture ready 19.60 s to 21.15 s, +7.9%), with The removal of forced |
|
One correctness gap in the installed-package development path is worth covering before this lands: A focused fix would thread the computed web React-Refresh/HMR-enabled value into the loader rule/options (or derive the same value from loader compiler options), forward it to the task, and add a loader-path regression using already-lowered JSX that asserts There is also a source-map correctness gap: the new loader is Finally, the loader's performance-profiler map/timer is process-local. With One option-semantics caveat for the description/benchmark output: production now always selects the API-transform path and its runner is inline, so |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e126dc301
ℹ️ 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".
| const parallel = getRouteModuleTransformParallel(parallelRouteTransform); | ||
| return { | ||
| loader: routeModuleTransformLoaderPath, | ||
| options, | ||
| ...(parallel ? { parallel } : {}), |
There was a problem hiding this comment.
When parallelRouteTransform is enabled explicitly, or automatically for 256+ routes, this emits Rspack's new parallel loader option unconditionally. The package still declares @rsbuild/core: ^2.0.0 as a peer, although this commit needed to upgrade its own toolchain to Rsbuild 2.1.5/Rspack 2.1.3 for this API; consumers on an accepted older peer version will therefore reject the rule during configuration instead of building. Raise the peer minimum or feature-detect this capability and fall back to a non-parallel loader.
Useful? React with 👍 / 👎.
| * Run route module transforms with Rspack's parallel loader workers. | ||
| * Pass `true` to use Rspack's default worker count, a positive integer to set | ||
| * the maximum worker count, or `false` to disable. | ||
| * @default Automatically enabled for 256+ resolved routes. |
There was a problem hiding this comment.
These public option semantics now delegate true to Rspack's default worker count and treat a number as a maximum, but README.md:87 still promises that development uses zero workers on machines with four or fewer cores and describes a number as the worker count. Users following the published README can consequently enable workers in environments where they expected inline transforms and see unexpected resource usage; update the README alongside this API change.
Useful? React with 👍 / 👎.
📝 WalkthroughWalkthroughThe route transformation pipeline now uses a Rspack loader and task runner instead of the previous worker executor. New rule registration handles route queries, route paths, source maps, performance reporting, and configurable parallel workers. Build and development wiring was updated, federation async startup was enabled, benchmark reporting gained worker metadata, and dependencies were upgraded. Tests cover loader behavior, transform rules, integration behavior, federation configuration, performance aggregation, and benchmark summaries. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/federation.ts`:
- Around line 20-27: Update the pluginName check in the federation plugin
detection flow to accept both the legacy “ModuleFederationPlugin” name and the
enhanced Rspack “RspackModuleFederationPlugin” name, while preserving the
existing option extraction. Add coverage for both plugin names, including the
enhanced constructor variant, and verify each enables experiments.asyncStartup.
In `@src/index.ts`:
- Around line 408-412: Update the route transform execution around
parallelRouteTransform and routeTransformRunner so useApiRouteModuleTransforms
mode also honors the configured parallelRouteTransform setting. Route API
transforms through the parallel task runner when enabled, or explicitly enforce
loader-only applicability for the option and preserve that contract consistently
at the additional referenced call site.
In `@tests/dev-runtime.integration.test.ts`:
- Around line 464-466: Update the hasDiskOutput assertion in the relevant
integration test to check build/client/virtual/react-router/browser-manifest.js,
removing the static/js segments so it validates the actual emitted
browser-manifest location.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: db524a5f-4104-4e2b-9bb1-30f181c3abd4
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (34)
benchmarks/synthetic-web-bundler-benchmark/package.jsonconfig/package.jsonexamples/client-only/package.jsonexamples/cloudflare/package.jsonexamples/custom-node-server/package.jsonexamples/default-template/package.jsonexamples/epic-stack/package.jsonexamples/federation/epic-stack-remote/package.jsonexamples/federation/epic-stack/package.jsonexamples/prerender/package.jsonexamples/spa-mode/package.jsonpackage.jsonrslib.config.tsscripts/benchmark/results.mtssrc/build-output-transforms.tssrc/dev-background-resources.tssrc/federation.tssrc/index.tssrc/parallel-route-transform-protocol.tssrc/parallel-route-transform-worker.tssrc/parallel-route-transforms.tssrc/performance.tssrc/route-module-transform-loader.tssrc/route-module-transform-rules.tssrc/route-transform-tasks.tssrc/types.tstests/benchmark-results.test.tstests/build-output-transforms.test.tstests/dev-runtime.integration.test.tstests/federation.test.tstests/index.test.tstests/parallel-route-transforms.test.tstests/route-module-transform-loader.test.tstests/route-module-transform-rules.test.ts
💤 Files with no reviewable changes (4)
- src/parallel-route-transform-worker.ts
- src/parallel-route-transform-protocol.ts
- tests/parallel-route-transforms.test.ts
- src/parallel-route-transforms.ts
| const pluginName = (plugin as ModuleFederationPluginLike).name; | ||
| if (pluginName !== 'ModuleFederationPlugin') { | ||
| continue; | ||
| } | ||
|
|
||
| const pluginOptions = | ||
| (plugin as ModuleFederationPluginLike)._options ?? | ||
| (plugin as ModuleFederationPluginLike).options; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate federation.ts =="
fd -a 'federation\.ts$' . || true
echo "== file excerpt =="
if [ -f src/federation.ts ]; then
nl -ba src/federation.ts | sed -n '1,140p'
fi
echo "== related federation symbols =="
rg -n "ensureFederationAsyncStartup|RspackModuleFederationPlugin|ModuleFederationPlugin|asyncStartup|_options|options" -S . --glob '!node_modules' --glob '!dist' --glob '!build' || trueRepository: rstackjs/rsbuild-plugin-react-router
Length of output: 456
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file excerpt =="
if [ -f src/federation.ts ]; then
awk '{printf "%6d\t%s\n", NR, $0}' src/federation.ts | sed -n '1,160p'
fi
echo "== related federation symbols =="
rg -n "ensureFederationAsyncStartup|`@rspack/`|RspackModuleFederationPlugin|ModuleFederationPlugin|asyncStartup|mangleInternalNames|_options|options" -S . --glob '!node_modules' --glob '!dist' --glob '!build' || true
echo "== package hints =="
for f in package.json pnpm-lock.yaml yarn.lock package-lock.json; do
[ -f "$f" ] && { echo "--- $f ---"; sed -n '1,220p' "$f"; }
doneRepository: rstackjs/rsbuild-plugin-react-router
Length of output: 50392
🌐 Web query:
@module-federation/extended rspack ModuleFederationPlugin construction plugin name RspackModuleFederationPlugin
💡 Result:
In the ecosystem of Rspack and Module Federation, the term RspackModuleFederationPlugin is not a standard export name for the primary Module Federation plugin [1][2]. Instead, you should use the ModuleFederationPlugin class, which is exported by the relevant packages depending on your required feature set [3][4]. For Rspack, there are two primary ways to implement Module Federation: 1. Using the built-in Rspack plugin: If you are using the native Module Federation capabilities built into Rspack, you import the plugin directly from Rspack's container namespace [1][4]: import { rspack } from '@rspack/core'; // Inside your plugins array new rspack.container.ModuleFederationPlugin({ // options }) 2. Using the @module-federation/enhanced package: For advanced features (often referred to as Module Federation v2.0), such as dynamic TypeScript type hints, improved runtime plugins, and better tree shaking for shared dependencies, you use the @module-federation/enhanced package [2][3][4]. The plugin is imported specifically for Rspack from this package [3][4]: import { ModuleFederationPlugin } from '@module-federation/enhanced/rspack'; // Inside your plugins array new ModuleFederationPlugin({ // options }) Regarding the name of the plugin class, the internal identifier (often used for logging or duplicate registration checks) is defined as PLUGIN_NAME within the source code of the @module-federation/core repository, but this is not the class name you use in your configuration [5]. Always refer to the class as ModuleFederationPlugin as shown above [3][4].
Citations:
- 1: https://rspack.rs/plugins/webpack/module-federation-plugin
- 2: https://rspack.rs/guide/features/module-federation
- 3: https://module-federation.io/integrations/bundler/rspack.html
- 4: https://rspack.rs/guide/features/module-federation.md
- 5: https://github.com/module-federation/core/blob/main/packages/rspack/src/ModuleFederationPlugin.ts
🌐 Web query:
@module-federation/enhanced rspack ModuleFederationPlugin construction plugin name RspackModuleFederationPlugin
💡 Result:
When using Module Federation with Rspack, particularly the modern implementation provided by the @module-federation/enhanced package, the relevant plugin is typically imported from @module-federation/enhanced/rspack [1]. Regarding your specific query about the construction of the plugin and the name RspackModuleFederationPlugin: 1. Plugin Class and Name: In the source code of the @module-federation/core repository, specifically within the package that provides Rspack integration, the class exported as ModuleFederationPlugin explicitly defines its internal name property as RspackModuleFederationPlugin [2]. This internal name is used by the plugin for logging, hook identification, and internal referencing during the build process [2]. 2. Usage: Users generally do not construct or reference RspackModuleFederationPlugin directly by that name in their configuration files. Instead, they import the ModuleFederationPlugin class from the @module-federation/enhanced/rspack entry point [1][3]. Example of standard construction: import { ModuleFederationPlugin } from '@module-federation/enhanced/rspack'; export default { plugins: [ new ModuleFederationPlugin({ // options }), ], }; 3. Context: While the class instance is named internally as RspackModuleFederationPlugin, it serves as the interface for Module Federation v2.0 (and 1.5 features) within Rspack, providing capabilities such as runtime plugins and improved tree shaking for shared dependencies [1]. Rspack also provides a built-in ModuleFederationPlugin (available via rspack.container.ModuleFederationPlugin) which implements Module Federation 1.5, and a legacy ModuleFederationPluginV1 [4][1][5].
Citations:
- 1: https://rspack.rs/guide/features/module-federation
- 2: https://github.com/module-federation/core/blob/main/packages/rspack/src/ModuleFederationPlugin.ts
- 3: https://medium.com/@fredlifourqoni9/membangun-microfrontend-dengan-rspack-perbandingan-module-federation-v1-5-vs-v2-0-f1241d83690e
- 4: https://rspack.rs/plugins/webpack/module-federation-plugin
- 5: https://rspack.rs/blog/module-federation-added-to-rspack
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== tests/federation.test.ts =="
if [ -f tests/federation.test.ts ]; then
awk '{printf "%6d\t%s\n", NR, $0}' tests/federation.test.ts
fi
echo "== fetch `@module-federation/enhanced` rspack source for Plugin name =="
node - <<'JS'
const https = require('https');
const url = 'https://unpkg.com/@module-federation/enhanced@2.5.1/dist/rspack/esm/ModuleFederationPlugin.js';
https.get(url, (res) => {
let data = '';
res.on('data', chunk => data += chunk);
res.on('end', () => {
const lines = data.split(/\r?\n/).filter(line =>
/class\s+ModuleFederationPlugin|static\s*#(?<name>|NAME|PLUGIN_NAME)|(?<name>name|constructor\.name|PLUGIN_NAME)\s*:|constructor?/
.test(line)
);
for (let i = 0; i < Math.min(lines.length, 120); i++) console.log(`${i+1} ${lines[i]}`);
});
}).on('error', e => {
console.error(e.message);
process.exit(1);
});
JS
echo "== fetch `@module-federation/rspack` source for legacy plugin constructor/name =="
node - <<'JS'
const https = require('https');
const url = 'https://unpkg.com/@module-federation/rspack@0.21.6/dist/plugin.esm.mjs';
https.get(url, (res) => {
let data = '';
res.on('data', chunk => data += chunk);
res.on('end', () => {
const lines = data.split(/\r?\n/).filter(line =>
/class\s+ModuleFederationPlugin|Name|name|constructor|PLUGIN_NAME|rspack/i
.test(line)
);
for (let i = 0; i < Math.min(lines.length, 120); i++) console.log(`${i+1} ${lines[i]}`);
});
}).on('error', e => {
console.error(e.message);
process.exit(1);
});
JS
echo "== deterministic probe of current selector =="
node - <<'JS'
function selects(plugin) {
const pluginName = plugin.name;
return pluginName === 'ModuleFederationPlugin';
}
const legacy = {
name: 'ModuleFederationPlugin',
options: { experiments: { asyncStartup: false } }
};
const enhanced = {
name: 'RspackModuleFederationPlugin',
options: { experiments: { asyncStartup: false } }
};
console.log(JSON.stringify({ legacySelected: selects(legacy), enhancedSelected: selects(enhanced), enhancedName: enhanced.name, enhancedConstructorName: enhanced.constructor?.name }));
JSRepository: rstackjs/rsbuild-plugin-react-router
Length of output: 2901
Match Rspack federation plugins by their actual plugin name.
@module-federation/enhanced/rspack’s ModuleFederationPlugin sets plugin.name = 'RspackModuleFederationPlugin', so this exact check keeps the enhanced examples from enabling experiments.asyncStartup. Add coverage for both the legacy ModuleFederationPlugin and the enhanced plugin name/constructor variant.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/federation.ts` around lines 20 - 27, Update the pluginName check in the
federation plugin detection flow to accept both the legacy
“ModuleFederationPlugin” name and the enhanced Rspack
“RspackModuleFederationPlugin” name, while preserving the existing option
extraction. Add coverage for both plugin names, including the enhanced
constructor variant, and verify each enables experiments.asyncStartup.
| harness.hasDiskOutput( | ||
| 'build/client/static/js/virtual/react-router/browser-manifest.js' | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Check the emitted browser-manifest path.
[name].js emits this entry at build/client/virtual/react-router/browser-manifest.js; the extra static/js makes this assertion pass even if the manifest is written to disk.
Proposed fix
- 'build/client/static/js/virtual/react-router/browser-manifest.js'
+ 'build/client/virtual/react-router/browser-manifest.js'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| harness.hasDiskOutput( | |
| 'build/client/static/js/virtual/react-router/browser-manifest.js' | |
| ) | |
| harness.hasDiskOutput( | |
| 'build/client/virtual/react-router/browser-manifest.js' | |
| ) |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/dev-runtime.integration.test.ts` around lines 464 - 466, Update the
hasDiskOutput assertion in the relevant integration test to check
build/client/virtual/react-router/browser-manifest.js, removing the static/js
segments so it validates the actual emitted browser-manifest location.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/route-module-transform-rules.ts (1)
24-42: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDo not silently ignore
parallelRouteTransformon the API path.This helper only configures the loader rules, but
src/index.tsbypasses those rules whenevershouldUseRouteModuleTransformApi()is true. As a result, the option has no effect on the inline/API path, including production. Preserve parallelism there or explicitly document and validate this option as development-only.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/route-module-transform-rules.ts` around lines 24 - 42, The parallelRouteTransform option is ignored when shouldUseRouteModuleTransformApi() selects the inline/API path. Update the API-path implementation in the surrounding route-module transform flow to preserve the configured boolean or worker-count parallelism, including production; otherwise explicitly reject non-default values with validation and document the development-only constraint.
🧹 Nitpick comments (1)
tests/route-module-transform-rules.test.ts (1)
48-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover both positive API-selection branches.
This test only proves the negative case. Add cases for
existsSync === trueandRSTEST === 'true'; otherwise regressions in the loader/API dispatch can go undetected.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/route-module-transform-rules.test.ts` around lines 48 - 63, Expand the route-module transform API tests around shouldUseRouteModuleTransformApi to cover both positive selection branches: mock fs.existsSync to return true with RSTEST unset, and set RSTEST to 'true' with existsSync false. Assert the function returns true in each case, while preserving environment restoration and spy cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/route-module-transform-rules.ts`:
- Around line 24-42: The parallelRouteTransform option is ignored when
shouldUseRouteModuleTransformApi() selects the inline/API path. Update the
API-path implementation in the surrounding route-module transform flow to
preserve the configured boolean or worker-count parallelism, including
production; otherwise explicitly reject non-default values with validation and
document the development-only constraint.
---
Nitpick comments:
In `@tests/route-module-transform-rules.test.ts`:
- Around line 48-63: Expand the route-module transform API tests around
shouldUseRouteModuleTransformApi to cover both positive selection branches: mock
fs.existsSync to return true with RSTEST unset, and set RSTEST to 'true' with
existsSync false. Assert the function returns true in each case, while
preserving environment restoration and spy cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e8e26ad3-a392-47a5-97aa-555a5ef8b41b
📒 Files selected for processing (11)
scripts/benchmark/results.mtssrc/build-output-transforms.tssrc/federation.tssrc/index.tssrc/route-module-transform-loader.tssrc/route-module-transform-rules.tstests/benchmark-results.test.tstests/build-output-transforms.test.tstests/dev-runtime.integration.test.tstests/route-module-transform-loader.test.tstests/route-module-transform-rules.test.ts
💤 Files with no reviewable changes (1)
- tests/dev-runtime.integration.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/benchmark-results.test.ts
- scripts/benchmark/results.mts
- tests/route-module-transform-loader.test.ts
- src/route-module-transform-loader.ts
- src/build-output-transforms.ts
- src/index.ts
- tests/build-output-transforms.test.ts
2 participants
Summary
Verification