fix(side-effects): respect DefinePlugin purity evaluation#13628
fix(side-effects): respect DefinePlugin purity evaluation#13628
Conversation
801e940 to
e11536d
Compare
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a regression test and updates the JS side-effects parser logic so expressions deemed pure by DefinePlugin (via evaluation) are respected during side-effect analysis, improving tree-shaking behavior.
Changes:
- Add a new config case under
side-effects/define-pluginto validate pruning of side-effect-free reexport chains underDefinePlugin. - Update
_is_pure_expressionto consultevaluate_expression(...).could_have_side_effects()before falling back tomay_have_side_effects. - Introduce fixtures (
lib/*,reexport.js) that exercise purity evaluation and package"sideEffects": false.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/rspack-test/configCases/side-effects/define-plugin/rspack.config.js | Adds a dedicated config enabling sideEffects optimization + DefinePlugin to set NODE_ENV. |
| tests/rspack-test/configCases/side-effects/define-plugin/reexport.js | Adds a reexport + conditionally-dead side effect to test purity evaluation. |
| tests/rspack-test/configCases/side-effects/define-plugin/lib/package.json | Marks the fixture package as side-effect-free ("sideEffects": false). |
| tests/rspack-test/configCases/side-effects/define-plugin/lib/module.js | Adds a simple exported symbol used by the test. |
| tests/rspack-test/configCases/side-effects/define-plugin/lib/index.js | Reexports module.js and exports an additional unused symbol. |
| tests/rspack-test/configCases/side-effects/define-plugin/index.js | Adds an assertion that certain modules are not present in the output bundle. |
| crates/rspack_plugin_javascript/src/parser_plugin/side_effects_parser_plugin.rs | Changes purity detection to respect plugin-driven could_have_side_effects evaluation overrides. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@codex review |
Rsdoctor Bundle Diff AnalysisFound 6 projects in monorepo, 1 project with changes. 📊 Quick Summary
📋 Detailed Reports (Click to expand)📁 popular-libsPath:
📦 Download Diff Report: popular-libs Bundle Diff Generated by Rsdoctor GitHub Action |
Merging this PR will degrade performance by 37.57%🎉 Hooray!
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | js@Traverse module graph by dependencies |
530.2 µs | 849.2 µs | -37.57% |
| ❌ | Simulation | js@collect imported identifiers |
210 µs | 212.6 µs | -1.25% |
Comparing fix-define-env-side-effects (0070cda) with main (c8abe3c)
Footnotes
-
28 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. ↩
📦 Binary Size-limit
🎉 Size decreased by 4.34KB from 49.34MB to 49.34MB (⬇️0.01%) |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Summary
Respect DefinePlugin purity evaluation for side effects plugin
Related links
Checklist