fix: track CJS re-export import records to fix inline const and tree-shaking#8925
Conversation
How to use the Graphite Merge QueueAdd the label graphite: merge-when-ready to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
✅ Deploy Preview for rolldown-rs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for rolldown-rs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
Fixes incorrect export resolution for CommonJS “re-export” modules (module.exports = require(...)) in conditional patterns by tracking the specific import records involved, so the linker follows all possible re-export targets. This improves correctness for both inline-const and tree-shaking in mixed/conditional CJS re-export scenarios (e.g. React jsx-dev-runtime-style branching).
Changes:
- Record the import record indices corresponding to
module.exports = require(...)during AST scanning and persist them onEcmaView. - In link stage, follow all recorded CJS re-export targets when building
resolved_exports, and track CJS export name conflicts to avoid unsafe inline-const. - Simplify the CJS tree-shaking bailout heuristic and add/extend integration tests + snapshots covering the regressions.
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/rolldown_common/src/types/resolved_export.rs | Adds cjs_conflicting_symbol_refs to represent conditional CJS export name conflicts. |
| crates/rolldown_common/src/ecmascript/ecma_view.rs | Stores cjs_reexport_import_record_ids on EcmaView for downstream link/tree-shaking logic. |
| crates/rolldown/src/stages/link_stage/tree_shaking/include_statements.rs | Uses recorded re-export import record ids to decide when to bail out for conditional CJS re-exports. |
| crates/rolldown/src/stages/link_stage/bind_imports_and_exports.rs | Builds resolved_exports using recorded CJS re-export targets; records ambiguities/conflicts. |
| crates/rolldown/src/module_loader/runtime_module_task.rs | Initializes new EcmaView field for the runtime module. |
| crates/rolldown/src/module_finalizers/mod.rs | Skips inline-const when exports have recorded CJS conflicts. |
| crates/rolldown/src/ecmascript/ecma_module_view_factory.rs | Plumbs scanner output into EcmaView (re-export import record ids). |
| crates/rolldown/src/ast_scanner/mod.rs | Tracks module.exports = require(...) require spans and resolves them to import record indices post-scan. |
| crates/rolldown/src/ast_scanner/impl_visit.rs | Updates pattern match for CommonJsAstType::Reexport variant shape. |
| crates/rolldown/src/ast_scanner/cjs_export_analyzer.rs | Extends Reexport classification to carry the require() call span used for mapping to import records. |
| crates/rolldown/tests/rolldown/tree_shaking/cjs_reexport_conditional/reexporter.js | New conditional CJS re-export fixture (ESM vs CJS targets). |
| crates/rolldown/tests/rolldown/tree_shaking/cjs_reexport_conditional/main.js | New tree-shaking test entry importing a named export from the re-exporter. |
| crates/rolldown/tests/rolldown/tree_shaking/cjs_reexport_conditional/esm-impl.mjs | New ESM implementation branch fixture. |
| crates/rolldown/tests/rolldown/tree_shaking/cjs_reexport_conditional/cjs-impl.js | New CJS implementation branch fixture. |
| crates/rolldown/tests/rolldown/tree_shaking/cjs_reexport_conditional/artifacts.snap | Snapshot asserting both branches remain and exports are accessible. |
| crates/rolldown/tests/rolldown/tree_shaking/cjs_reexport_conditional/_test.mjs | Runtime assertion validating branch behavior under default env. |
| crates/rolldown/tests/rolldown/tree_shaking/cjs_reexport_conditional/_config.json | Enables treeshake for the new test case. |
| crates/rolldown/tests/rolldown/topics/hmr/inline_const_cjs_reexport/main.js | New HMR + inline-const regression fixture for jsx-dev-runtime-like conditional re-export. |
| crates/rolldown/tests/rolldown/topics/hmr/inline_const_cjs_reexport/jsx-dev-runtime.production.js | Production branch fixture exporting void 0. |
| crates/rolldown/tests/rolldown/topics/hmr/inline_const_cjs_reexport/jsx-dev-runtime.js | Conditional module.exports = require(...) re-exporter fixture. |
| crates/rolldown/tests/rolldown/topics/hmr/inline_const_cjs_reexport/jsx-dev-runtime.development.js | Development branch fixture exporting a function. |
| crates/rolldown/tests/rolldown/topics/hmr/inline_const_cjs_reexport/artifacts.snap | Snapshot ensuring jsxDEV is not incorrectly inlined to void 0. |
| crates/rolldown/tests/rolldown/topics/hmr/inline_const_cjs_reexport/_config.json | Enables devMode and inlineConst for the new HMR regression. |
| crates/rolldown/tests/rolldown/optimization/inline_const/cjs_reexport_first_import_record/reexporter.js | New regression: multiple requires, only one is the module.exports = require(...) target. |
| crates/rolldown/tests/rolldown/optimization/inline_const/cjs_reexport_first_import_record/real-exports.js | Fixture providing the actual export value. |
| crates/rolldown/tests/rolldown/optimization/inline_const/cjs_reexport_first_import_record/main.js | Entry importing named export from the re-exporter. |
| crates/rolldown/tests/rolldown/optimization/inline_const/cjs_reexport_first_import_record/debug.js | Fixture “decoy” require that should not be treated as the re-export target. |
| crates/rolldown/tests/rolldown/optimization/inline_const/cjs_reexport_first_import_record/artifacts.snap | Snapshot asserting inline-const selects the real export target, not the first import record. |
| crates/rolldown/tests/rolldown/optimization/inline_const/cjs_reexport_first_import_record/_test.mjs | Runtime assertion for the inline-const regression. |
| crates/rolldown/tests/rolldown/optimization/inline_const/cjs_reexport_first_import_record/_config.json | Enables inlineConst for the new optimization regression. |
crates/rolldown/tests/rolldown/tree_shaking/commonjs_reexport_conditional/_config.json
Show resolved
Hide resolved
Merging this PR will not alter performance
Comparing Footnotes
|
a5331f8 to
eda6437
Compare
crates/rolldown/tests/rolldown/tree_shaking/commonjs_reexport_conditional/_test.mjs
Outdated
Show resolved
Hide resolved
crates/rolldown/tests/rolldown/topics/hmr/inline_const_cjs_reexport/main.js
Outdated
Show resolved
Hide resolved
eda6437 to
2313351
Compare
2313351 to
5b5bdf0
Compare
crates/rolldown/tests/rolldown/tree_shaking/commonjs_reexport_conditional/_test.mjs
Outdated
Show resolved
Hide resolved
387dec1 to
8ca08e3
Compare
Merge activity
|
…shaking (#8925) ## Summary When a CJS module conditionally re-exports from multiple sources (e.g., React's `jsx-dev-runtime.js`), the linker previously only followed the first import record. This could lead to incorrect inline const values and incomplete tree-shaking. This PR: - Records which import records correspond to `module.exports = require(...)` during scanning, rather than relying on `import_records.first()` - Follows all CJS re-export targets when building `resolved_exports` - Tracks conflicting CJS exports when multiple targets provide the same name - Skips inline const for exports with CJS conflicts, since the runtime value depends on which branch executes - Uses the recorded import record indices to simplify the tree-shaking bailout heuristic ## Background ```js // jsx-dev-runtime.js if (process.env.NODE_ENV === 'production') { module.exports = require('./production'); // exports.jsxDEV = void 0 } else { module.exports = require('./development'); // exports.jsxDEV = function() {...} } ``` Previously, `add_exports_for_export_star` used `import_records.first()` to find the re-export target. This meant only the first `require()` target's exports entered `resolved_exports` — the rest were silently dropped. This caused `jsxDEV` to be incorrectly inlined as `void 0`. ## Related issues - vitejs/vite#21884 — the `void 0` inlining case fixed by this PR - #8345 (comment) — related discussion on the underlying issue - vitejs/vite#21843 — related Vite issue, worked around by vitejs/vite#21865 (disabling `inlineConst` for affected packages). This PR fixes the root cause in rolldown so we can reenable `inlineConst` in Vite. cc @sapphi-red ## Test plan - [x] New `inline_const_cjs_reexport` - validates inline const works with multiple cjs re-exports - [x] New `tree_shaking/cjs_reexport_conditional` — validates ESM+CJS mixed re-export (ESM first, CJS second) - [x] `just test-rust` all green
bf42ed3 to
724f077
Compare

Summary
When a CJS module conditionally re-exports from multiple sources (e.g., React's
jsx-dev-runtime.js), the linker previously only followed the first import record. This could lead to incorrect inline const values and incomplete tree-shaking.This PR:
module.exports = require(...)during scanning, rather than relying onimport_records.first()resolved_exportsBackground
Previously,
add_exports_for_export_starusedimport_records.first()to find the re-export target. This meant only the firstrequire()target's exports enteredresolved_exports— the rest were silently dropped. This causedjsxDEVto be incorrectly inlined asvoid 0.Related issues
void 0inlining case fixed by this PRplatform: "node"causes CJS module exports to be silently dropped (immer) #8345 (comment) — related discussion on the underlying issueinlineConstoptimization vitejs/vite#21865 (disablinginlineConstfor affected packages). This PR fixes the root cause in rolldown so we can reenableinlineConstin Vite. cc @sapphi-redTest plan
inline_const_cjs_reexport- validates inline const works with multiple cjs re-exportstree_shaking/cjs_reexport_conditional— validates ESM+CJS mixed re-export (ESM first, CJS second)just test-rustall green