Repository navigation
bundler: wrap a split import() with __toESM when the target is CommonJS at link time - #41237
Merged
Jarred-Sumner merged 1 commit intoSep 3, 2026
Merged
Conversation
Collaborator
Author
|
Status: rebased on main (1d1f431), after #41231 and #41243. No conflicts. Waiting for CI.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review. WalkthroughChangesThe linker now applies standard CommonJS interop handling to external dynamic imports of CommonJS dynamic import interop
Suggested reviewers: Merge Risk: ⚪ Minimal · up to Split dynamic imports of CommonJS modules now preserve named exports and default namespace behavior instead of returning an unusable bare chunk namespace. The covered bundler scenarios pass with no remaining concrete merge-blocking risk. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the problem, cause, fix, scope, regression tests, verification results, and excluded changes. It does not use the template headings exactly, but it provides the required information and is complete. Comment |
Collaborator
Author
Collaborator
|
@robobun fix conflicts |
robobun
force-pushed
the
robobun/f0abc94a/splitting-import-cjs-at-link-time
branch
from
September 3, 2026 03:44
3b23e6d to
b55d5a5
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
robobun
force-pushed
the
robobun/f0abc94a/splitting-import-cjs-at-link-time
branch
from
September 3, 2026 03:53
b55d5a5 to
b36f032
Compare
Collaborator
Author
|
Fixed. The branch is rebased on main (dcd8569), and the PR merges cleanly.
|
Jarred-Sumner
pushed a commit
that referenced
this pull request
Sep 3, 2026
…at link time (#41243) ### Problem - With `--splitting --target=bun`, a split `require()` of a file that is CommonJS at link time returns the chunk namespace `{ default: module.exports }` and not `module.exports`. In the repro of #41236, `require("react-dom").version` is `undefined`. With npm react-dom 18.3.1 in development, `createPortal` is `undefined`. - The cause: since #41188 the parser lifts `module.exports = require("./impl")` into `export * from "./impl"`, so the file is ESM at parse time and `ReachableFileVisitor` splits the `require()` (`src/bundler/bundle_v2.rs:1998`). The linker then wraps the file again because `./impl` is CommonJS (`scanImportsAndExports.rs:422`). Its chunk is `export default require_x()`, and `import.meta.require()` of that chunk returns the namespace. This is a regression on main only. No release has it. ### Fix - `scan_imports_and_exports` sets a new record flag, `CROSS_CHUNK_REQUIRE_DEFAULT`, on a split `require()` whose target has `exports_kind == Cjs` at link time. The printer then prints `import.meta.require("./chunk.js").default`. - Correct because splitting is ESM output only, where a CommonJS entry chunk exports exactly one thing: `default: module.exports`. A target that stays ESM at link time keeps the bare call and gets its namespace, as before. - The split output now matches the unsplit bundle: `typeof m, m.version, typeof m.default` print `object 19.0.0 function` in both. - Verified: two new cases in `test/bundler/bundler_splitting.test.ts` (`SplitRequireOfRewrappedLiftedCommonJS#41236` fails on main). Also bundler_splitting, bundler_cjs2esm, bundler_cjs, esbuild/splitting, bundler_compile_splitting, bundler_edgecase, bundler_regressions, bundler_bun, all green with the debug build. ### Background - A split `require()` (target bun, `--splitting`) makes its target an entry point with its own chunk and prints the call as `import.meta.require(path)`. The linker decides this at parse time from `ExportsKind::Esm`. - Lifting: in an ESM bundle the parser turns top-level `exports.foo = ...` or `module.exports = require()` into ES exports. The lifted file can still become CommonJS at link time, for example when the target of its export star has no static exports. - #41237 fixes the same shape for a split `import()`. This PR changes only `require()` records. <details><summary>Notes</summary> Repro from the issue, in an empty directory: ```sh mkdir -p node_modules/react-dom printf "console.log('side effect');\nmodule.exports = require('./impl');\n" > node_modules/react-dom/index.js printf 'module.exports = function render() { return "rendered"; };\nmodule.exports.version = "19.0.0";\n' > node_modules/react-dom/impl.js printf 'let m;\ntry {\n m = require("react-dom");\n} catch {}\nconsole.log(m.version);\n' > entry.js bun build ./entry.js --splitting --target=bun --outdir=out && bun out/entry.js ``` Main prints `side effect` then `undefined`. With this change it prints `19.0.0`. The entry chunk now has `m = import.meta.require("./index-<hash>.js").default`. `module.exports` of the re-wrapped lifted file is `__toESM(require_impl())`, an object with `default` set to the function. That is the #41188 lift behavior and is the same in the unsplit bundle, so the test reads `m.default()`. The second test, `SplitRequireOfLiftedCommonJSStaysEsm`, pins the other side: when `./impl` uses `exports.x = ...` and is lifted too, the chunk stays ESM and the call has no `.default`. The alternative, to not split a `require()` of a lifted file with an export star, would keep the wrapper in the importer's chunk. The link-time check is the smaller change and covers every way a parse-time ESM target becomes CommonJS at link time, not only the lift case. `cargo clippy -p bun_bundler -p bun_js_printer` is clean. </details>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Collaborator
|
@robobun fix conflicts |
…JS at link time With code splitting, a cross-chunk import() skipped the __toESM wrap when the target had FORCE_CJS_TO_ESM. The parser sets that flag on each file in the unwrap list and on each file whose exports.foo = ... it lifted. Such a file can still be CommonJS at link time: a require() of it wraps it, it assigns module.exports, or the target of its lifted module.exports = require() is CommonJS. Its chunk is then `export default require_x()`, so the importer saw the bare chunk namespace and named exports were undefined. Remove the skip. Since #41231 it did nothing else. The branch below already wraps a cross-chunk import() when the target is CommonJS at link time, and leaves every other target alone.
robobun
force-pushed
the
robobun/f0abc94a/splitting-import-cjs-at-link-time
branch
from
September 3, 2026 04:22
b36f032 to
74e951b
Compare
Collaborator
Author
|
Fixed again. The branch is rebased on main (1d1f431), and the PR merges cleanly.
|
Jarred-Sumner
deleted the
robobun/f0abc94a/splitting-import-cjs-at-link-time
branch
September 3, 2026 04:39
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
--splitting, a cross-chunkimport()of a module that is CommonJS at link time resolves to the bare chunk namespace{ default: module.exports }. With npm react-dom 18.3.1,(await import("react-dom/client")).createRootisundefined. Unsplit builds give the function.src/bundler/linker_context/scanImportsAndExports.rs:1116drops the__toESMwrap for everyimport()target withFORCE_CJS_TO_ESM. Such a target can still be CommonJS at link time, with the chunkexport default require_x().Fix
.then((m) => __toESM(m.default))for a CommonJS target, and nothing for an ESM target.exports_kind == Cjsmeans the chunk exports onlydefault: module.exports. Since bundler: give a split import() of a lifted CommonJS module its namespace as default #41231 the skip did nothing else, so ESM targets print the same.test/bundler/bundler_cjs2esm.test.tsfail on 1.4.1 and on main, and pass with this change.Background
exports.foo = ...into ES module exports and setsFORCE_CJS_TO_ESM. Every file of the unwrap list (react, react-dom, ...) gets the flag, lifted or not.module.exports, when arequire()of it wraps it, or when the target of its liftedmodule.exports = require()is CommonJS (bundler: lift module.exports = require() after side effects in unwrapped packages to export * #41188).import()target gets its own entry point chunk.Notes
No issue reports this. It was found during work on the nearby CommonJS lifting code.
An earlier version of this PR narrowed the skip to
exports_kind != Cjs. After the rebase on #41231, the body of the skip was onlycontinue, so the narrowed skip did nothing. This version removes it. The self-review found a wrong comment aboutFORCE_CJS_TO_ESM(now removed with the code). It also asked for a test outside the unwrap list and for a fuller description.Real packages (react 18.3.1, react-dom 18.3.1, scheduler 0.23.2), entry:
--splitting, browser or bun target, development or productionundefined undefined undefinedfunction function function--splitting --minify, productionundefined function undefinedfunction function function--splitting --minify, developmentundefined undefined undefinedfunction function function--splittingfunction function functionfunction function functionbun runof the entry printsfunction function function.Minimal repro in an empty directory. Same output on 1.4.1 and main:
Before:
TypeError: m.useState is not a function. After:1 true. The importer now printsawait import("./index-<hash>.js").then((m)=>__toESM(m.default,1)).The three new cases, one for each way to be CommonJS at link time:
cjs2esm/DynamicImportSplittingOfWrappedCommonJS:react/index.jsis a run-timeifover twomodule.exports = require()calls, so it is never lifted.cjs2esm/DynamicImportSplittingOfRewrappedLiftedCommonJS: theReactSpecificUnwrappingTargetIsCommonJSfixture from bundler: lift module.exports = require() after side effects in unwrapped packages to export * #41188. The linker wrapsreact-dom/index.jsagain becauseimpl.jsassignsmodule.exports = function. It printsm.default.version, nottypeof m.default, so it does not depend on bundler: skip __toESM for unwrapped require() that lands on a CJS wrapper #35722.cjs2esm/DynamicImportSplittingOfRequiredLiftedCommonJS: a user file withexports.foo = ..., outsidenode_modules. The entryrequire()s it andimport()s it. Before:foo undefined true. The unsplit build andbun runprintfoo foo true.The
require()side of the same shape was a regression from #41188 (#41236). #41243 fixed it on main, in the same block. This PR changes onlyimport()records.Not changed (each reproduces with and without this change):
const { useState } = require("react")throwsReferenceError: exports is not definedin a bundle, split or not. bundler: read destructured require() of an unwrapped package from its import namespace #39184 fixes it.--splitting,import()of a file that doesexport * from "<cjs>"readsundefinedfor the names of the CommonJS module. This happens for any CommonJS package.default. It keeps the skip for a CommonJS target, so it does not cover this bug. This branch is rebased on it, and its tests pass here.The unwrap list is
DEFAULT_UNWRAP_COMMONJS_PACKAGESinsrc/bundler/options.rs: react, react-dom, scheduler, react-is, react-refresh, react-client, react-server.Also checked with the debug build under
--splitting:module.exports = { ... },module.exports = function, an importer whose only__toESMuse is theimport()(it gets the runtime import), and a.jsimporter (__toESM(m.default)without the node-mode flag, as on the existing path). A user file that is onlyimport()ed, and a realmodule.exportsfile, print the same before and after.Suites run with the debug build on main 1d1f431 (after #41231 and #41243), with this version of the fix: bundler_cjs2esm and bundler_splitting (188 pass), and bundler_cjs, bundler_dynamic_import_dce, esbuild/splitting (358 pass, 0 fail). Before those rebases, on main e8c8d81: the same suites plus esbuild/default, bundler_edgecase, bundler_regressions, bundler_npm, bundler_compile_splitting, bundler_bun, bundler_browser (904 pass, 0 fail).
cargo clippy -p bun_bundleris clean.[human-review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 0
evidence per changed file