fix: handle cjs export assignment side effects#14260
Conversation
📦 Binary Size-limit
❌ Size increased by 4.00KB from 62.60MB to 62.60MB (⬆️0.01%) |
Rsdoctor Bundle Diff AnalysisFound 5 projects in monorepo, 1 project with changes. 📊 Quick Summary
📋 Detailed Reports (Click to expand)📁 ui-componentsPath:
📦 Download Diff Report: ui-components Bundle Diff 🤖 AI Degradation Analysis (Click to expand)📊 Size Changes
Win: Total bundle size decreased by 880 bytes (5,079,179 → 5,078,299). 🔍 Root Cause Analysis
|
Merging this PR will degrade performance by 23.03%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | WallTime | bundle@threejs-10x-development |
237.1 ms | 308 ms | -23.03% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing fy/cjs-export-assignment-side-effects (ccaf6c4) with main (b04d9f4)
Footnotes
-
40 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
JSerFeng
force-pushed
the
fy/cjs-export-assignment-side-effects
branch
3 times, most recently
from
7f086af to
1b48b04
Compare
![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: 1b48b0460d
ℹ️ 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".
| unresolved_ctxt, | ||
| comments, | ||
| ), | ||
| Expr::Assign(assign_expr) if is_common_js_export_assignment(assign_expr) => { |
There was a problem hiding this comment.
This branch runs in the generic side-effects analyzer without checking parser.is_esm, so an ESM file such as export {}; exports.foo = 1; imported only for side effects is now classified as side-effect-free when the RHS is pure. In ESM that assignment is not a CommonJS export write: it can mutate a global exports object or throw a ReferenceError, both of which are observable, so the optimizer can incorrectly drop the module.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR improves tree-shaking accuracy for CommonJS modules by treating static export assignments (e.g. exports.foo = ..., module.exports = ...) as evaluation-side-effect-free for the assignment target, while still preserving RHS initializer side effects and preserving true CommonJS reads/calls that should keep modules alive.
Changes:
- Adjust side-effects analysis to consider CommonJS export assignments pure w.r.t. the LHS target and only evaluate/preserve RHS side effects.
- Track “assignment-target context” during AST walking and propagate it into CommonJS self-reference dependencies to avoid keeping modules alive due to LHS self-references.
- Add a dedicated tree-shaking fixture + snapshots covering pure export assignments vs. an impure RHS that must remain.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/rspack-test/treeShakingCases/cjs-export-assignment-side-effects/rspack.config.js | Adds a tree-shaking test config enabling sideEffects optimization. |
| tests/rspack-test/treeShakingCases/cjs-export-assignment-side-effects/pure-named-exports.js | Fixture: pure exports.* assignments intended to be droppable. |
| tests/rspack-test/treeShakingCases/cjs-export-assignment-side-effects/pure-module-exports.js | Fixture: pure module.exports = ... assignment intended to be droppable. |
| tests/rspack-test/treeShakingCases/cjs-export-assignment-side-effects/impure-named-exports.js | Fixture: RHS side effect that must be preserved. |
| tests/rspack-test/treeShakingCases/cjs-export-assignment-side-effects/index.js | Entry importing fixtures to validate elimination/preservation behavior. |
| tests/rspack-test/treeShakingCases/cjs-export-assignment-side-effects/snapshots/treeshaking.snap.txt | Snapshot asserting only RHS side effects remain and pure export assignments are dropped. |
| tests/rspack-test/statsOutputCases/side-effects-optimization/snapshots/stats.txt | Updates stats snapshot to reflect improved side-effects detection output. |
| crates/rspack_plugin_javascript/src/visitors/dependency/parser/mod.rs | Adds in_assign_target parser state flag. |
| crates/rspack_plugin_javascript/src/visitors/dependency/parser/walk.rs | Sets/restores in_assign_target while walking assignment targets. |
| crates/rspack_plugin_javascript/src/parser_plugin/side_effects_parser_plugin.rs | Treats CommonJS export assignments as pure w.r.t. LHS and evaluates RHS for side effects. |
| crates/rspack_plugin_javascript/src/parser_plugin/common_js_exports_parse_plugin.rs | Threads in_assign_target into CommonJsSelfReferenceDependency creation. |
| crates/rspack_plugin_javascript/src/dependency/commonjs/common_js_self_reference_dependency.rs | Adds evaluation_side_effect_free and implements evaluation side-effects state accordingly. |
| crates/rspack_plugin_javascript/src/dependency/commonjs/common_js_exports_dependency.rs | Marks CommonJS export dependencies as evaluation side-effect-free. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| true | ||
| } | ||
|
|
||
| fn is_common_js_export_assignment(expr: &AssignExpr) -> bool { |
| match current_member.obj.as_ref() { | ||
| Expr::Ident(ident) => { | ||
| members.push(ident.sym.clone()); | ||
| break; | ||
| } |
| Expr::Assign(assign_expr) if is_common_js_export_assignment(assign_expr) => { | ||
| is_pure_expression( |
JSerFeng
force-pushed
the
fy/cjs-export-assignment-side-effects
branch
from
1b48b04 to
61c0669
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61c066974d
ℹ️ 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".
| parser.module_has_side_effects = true; | ||
| } | ||
|
|
||
| if !has_side_effects && parser.side_effects_item.is_none() { |
There was a problem hiding this comment.
When experiments.pureFunctions is enabled, identifier calls such as doWork() are first treated as pure and only become a bailout/deferred check in the callees loop below; however a preceding pure CJS export assignment now sets parser.side_effects_item while leaving module_has_side_effects false, so this guard skips that loop for subsequent statements. A module like exports.foo = 1; doWork(); can therefore stay side_effect_free and be dropped when imported only for side effects, losing doWork()'s effects.
Useful? React with 👍 / 👎.
JSerFeng
force-pushed
the
fy/cjs-export-assignment-side-effects
branch
from
61c0669 to
8c4262c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c4262c2b7
ℹ️ 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".
|
|
||
| if compiler_options.optimization.side_effects.is_true() { | ||
| let has_side_effects = side_effects_item.is_some(); | ||
| let has_side_effects = module_has_side_effects; |
There was a problem hiding this comment.
Using only module_has_side_effects here drops side effects that are reported from the module_declaration hook: that hook still sets only parser.side_effects_item for impure export default / export const initializers and never sets the new flag. With optimization.sideEffects, a module such as export default console.log("keep") can therefore be marked side_effect_free and be removed when imported only for side effects, even though the export initializer runs during module evaluation.
Useful? React with 👍 / 👎.
| matches!(members.as_slice(), [first, ..] if first == "exports" && members.len() > 1) | ||
| || matches!(members.as_slice(), [first, second, ..] if first == "module" && second == "exports") |
There was a problem hiding this comment.
This treats every chain beginning with exports or module.exports as a pure export target, but deep writes like exports.foo.bar = 1 and module.exports.foo.bar = 1 first read exports.foo / module.exports.foo; when that property is missing (or has a getter), the read can throw or run user code. If such a module is imported only for side effects, it can now be classified as side-effect-free and removed, hiding that observable read/throw. The special case should be limited to direct export writes unless the intermediate object access is also proven safe.
Useful? React with 👍 / 👎.
JSerFeng
force-pushed
the
fy/cjs-export-assignment-side-effects
branch
3 times, most recently
from
78a265f to
7bde384
Compare
JSerFeng
force-pushed
the
fy/cjs-export-assignment-side-effects
branch
from
7bde384 to
ccaf6c4
Compare
Summary
module.exports/exports.fooassignments and an impure RHS assignment that must remain.Root cause
CommonJS export assignments such as
exports.foo = valueandmodule.exports = valuewere conservatively treated as side-effectful because the assignment/member access participated in module evaluation side-effect checks. Formodule.exports = value, walking the assignment target can also create a self-reference dependency for the left-handmodule.exports; that dependency must be evaluation side-effect-free only in assignment-target position, otherwise normal CommonJS self references can be incorrectly dropped.Validation
cargo fmt --package rspack_plugin_javascriptpnpm run build:binding:devpnpm --dir tests/rspack-test run test -- TreeShaking.test.js -t cjs-export-assignment-side-effectspnpm --dir tests/rspack-test run test -- Config.part2.test.js -t "entry/depend-on-bug|library/modern-module-force-concaten"pnpm --dir tests/rspack-test run test -- StatsOutput.test.js -t side-effects-optimization