Skip to content

bundler: do not treat a __commonJS wrapper as async in import() - #46748

Open
robobun wants to merge 2 commits into
mainfrom
robobun/0788101d/import-call-of-commonjs-wrapper
Open

robobun wants to merge 2 commits into
mainfrom
robobun/0788101d/import-call-of-commonjs-wrapper

Conversation

@robobun

@robobun robobun commented Oct 24, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • import() of a bundled CommonJS file can throw at run time: TypeError: undefined is not an object (evaluating 'ns.default.t'). bun build exits 0. The output has var ns = await require_c();.
  • Trigger: tree shaking removes the file's import of a package with top-level await ("sideEffects": false). An export * from such a module is a second trigger.
  • Cause: require_or_import_meta_for_source (src/bundler/LinkerContext.rs:2574) reads the async flag of the file. It ignores which wrapper the file has.

Fix

  • is_wrapper_async is true only for a file in an __esm wrapper.
  • import() of a CommonJS file then prints Promise.resolve().then(() => __toESM(require_c())), as for every other CommonJS file.
  • Verified: test/bundler/bundler_cjs.test.ts (5 new tests, all fail on main). Other suites: Notes.
  • Self-reviewed: 1 concern raised (bundles that run today change), 1 addressed in Downsides and tests.

Background

  • The linker marks a file async when a module with top-level await is on its chain of static imports. It does that before tree shaking, for CommonJS files too.
  • A lazily loaded ES module gets var init_x = __esm(...). For an async file init_x() returns a promise, and import() prints init_x().then(...).
  • A CommonJS file gets var require_x = __commonJS(...). require_x() returns module.exports, never a promise.

Downsides

  • With a trigger above, some bundles that run today change. const f = await import("./c.js"); f() worked for a function in module.exports. The result is now the namespace, and f() throws. esbuild still prints await require_c().
  • The file body runs in a microtask, not at the import() expression.
  • Release .text +0 B, no new pass, no allocation.
Notes

Repro

// node_modules/async-pkg/package.json
{ "name": "async-pkg", "main": "index.js", "sideEffects": false }
// node_modules/async-pkg/index.js
await 0;
export const unused = 1;
// c.js
import { unused } from "async-pkg";
module.exports = { t: 5 };
// entry.js
const ns = await import("./c.js");
console.log(ns.default.t);

bun build entry.js --outfile=out.js && bun out.js. On main (179664e) the bundle has var ns = await require_c(); and throws TypeError: undefined is not an object (evaluating 'ns.default.t'). With this PR it has var ns = await Promise.resolve().then(() => __toESM(require_c())); and prints 5. Delete the import line of c.js, and main prints that second form too.

With import("./c.js").then(ns => ...) main prints require_c().then(...) and throws TypeError: require_c().then is not a function.

Origin. No user reported this. It was found by a read of the code, with #46749.

Why the flag is set. validate_tla and propagate_async_dependencies mark a file async when a static import leads to a top-level await. They run before tree shaking and for every file, so a CommonJS file keeps the mark when its import is removed. The printer (src/js_printer/lib.rs, print_require_or_import_expr) reads the mark through is_wrapper_async and prints the wrapper call as a promise. That is right for init_x() of an async __esm wrapper only.

What changes in a bundle that runs today. Only for import(), without --splitting, of a CommonJS file that has the mark. The value is now the one that the same file gets when the mark is absent.

entry.js main this PR
ns.default.t TypeError 5
import("./c.js").then(...) TypeError runs
const f = await import("./c.js"); f(), module.exports is a function 5 TypeError: f is the namespace
new (await import("./c.js"))(), module.exports is a class runs TypeError
typeof of the result, module.exports = "hello" string object
Object.keys(ns), module.exports = { t, u } t, u default, t, u
ns === c, where c is the default import true false, and ns.default === c
ns.t = 9 writes module.exports TypeError
a console.log in c.js, between two logs around import() before, c, after before, after, c

esbuild 0.21.5 and 0.25.1 print require_c() for the repro too. So they have the first two rows of main, and they run f().

Fixtures. Release builds of main (179664e, md5 105b11a1) and of this PR (0e6f008, md5 ceb94ddd), 259 builds of small graphs:

Measurements.

  • size -A: .text 82,164,617 to 82,164,617. No section changes size.
  • Instructions for a build of 20,000 CommonJS files (user mode, median of 10 interleaved runs): 1,969,884,550 to 1,969,983,496 (+0.005%). The main binary against a copy of itself: +0.024%.
  • The change is one compare on a byte that the function has already loaded. No struct changes.

Not in this PR.

Tests. test/bundler/bundler_cjs.test.ts: 67 pass with the debug build of this branch. With a release build of main the 5 new tests fail.

Suites. Release build of this branch: all 71 files of test/bundler/ and test/bundler/esbuild/ and test/regression/issue/cyclic-imports-async-bundler.test.js: 3,133 pass, 2 fail. The 2 are in bundler_bytecode_portable.test.ts and fail the same way with the build of main. I cannot run Windows or macOS here.

The linker marks a file async when a module with top-level await is on its
chain of static imports. It does that before tree shaking, and for
CommonJS files too. import() of such a CommonJS file was printed as if
require_x() returned a promise: `await require_c()` gave module.exports
and no namespace, and `require_c().then(...)` threw.

is_wrapper_async is now true only for a file in an __esm wrapper, so
import() of a CommonJS file prints
`Promise.resolve().then(() => __toESM(require_c()))` as for every other
CommonJS file.
@coderabbitai

coderabbitai Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between 179664e and 085bbaa.

Files selected for processing: 2
  • src/bundler/LinkerContext.rs
  • test/bundler/bundler_cjs.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.

Walkthrough

The linker now marks a wrapper async only when it is an ESM wrapper with an async dependency and a valid wrapper reference. New tests cover dynamic imports of marked-async bundled CommonJS modules.

Changes

CommonJS Dynamic Import

Layer / File(s) Summary
Async wrapper classification
src/bundler/LinkerContext.rs
The linker now requires an ESM wrapper, an async dependency, and a valid wrapper reference before marking a wrapper async.
Dynamic import behavior and coverage
test/bundler/bundler_cjs.test.ts
Tests cover namespace results for object and function exports, execution order, and re-exports from a top-level-await module. The added fixture has a side-effect-free top-level await and an unused export.

Possibly related PRs

  • oven-sh/bun#46736: Changes the bundled CommonJS dynamic-import path by adding TO_ESM_NODE_MODE and changing the __toESM call.

Suggested reviewers: jarred-sumner, dylan-conway

Priority: Normal

Merge Risk: Minimal · up to 085bb

No actionable merge-blocking issue remains; the change is ready for normal merge checks.

Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title clearly and concisely identifies the main change: preventing __commonJS wrappers from being treated as async during import().
Description check Passed The description explains the problem, root cause, fix, behavioral changes, limitations, and verification results. It does not use the exact template headings, but it provides the required content and …
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description explains the problem, root cause, fix, behavioral changes, limitations, and verification results. It does not use the exact template headings, but it provides the required content and is substantially complete.

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Oct 24, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status

Reproduced on main (179664e, release build) and on Bun 1.4.3:

  1. c.js is import { unused } from "async-pkg"; module.exports = { t: 5 };. The package async-pkg has "sideEffects": false, and its index.js is await 0; export const unused = 1;.
  2. entry.js is const ns = await import("./c.js"); console.log(ns.default.t);.
  3. bun build entry.js --outfile=out.js && bun out.js throws TypeError: undefined is not an object (evaluating 'ns.default.t'). The bundle has var ns = await require_c();.

With this PR the bundle prints 5.

PR: #46748

CI (build 132046, 085bbaa): the diff is green on every job that ran. 179 jobs passed. 7 test files failed once and passed on retry. One of them is test/bundler/bun-build-compile.test.ts: the case checks the order file of a compiled executable after SIGINT, and this change does not touch that. Two jobs cannot start: no agent takes darwin x64 - test-bun, in this build or in the other recent PR builds (29 checked), so the jobs expire and the build does not end.

@robobun

robobun commented Oct 24, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 10:23 AM PT - Oct 24th, 2026

✅ @robobun, your commit 085bbaa41c843259906726ae606bbc496dd4020b passed in Build #132046! 🎉


🧪   To try this PR locally:

bunx bun-pr 46748

That installs a local version of the PR into your bun-46748 executable, so you can run:

bun-46748 --bun

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Beyond the inline finding on the export-star test, I checked the sibling sites keyed on the async flag: postProcessJSChunk.rs:867 already gates on wrap == Esm, and generateCodeForFileInChunkJS.rs emits the async wrapper only in the WrapKind::Esm arm, so the new gate matches the only place a wrapper can be a promise; is_wrapper_async has a single consumer (js_printer/lib.rs:2791). The namespace-vs-module.exports behavior change for import() of a function/class export is intentional and documented in the description, so I did not flag it.

Extended reasoning...

The change adds a flags.wrap == WrapKind::Esm condition to is_wrapper_async in src/bundler/LinkerContext.rs and five itBundled cases in test/bundler/bundler_cjs.test.ts; no security-sensitive surface. The gate is consistent with the other async-wrapper sites, but one inline finding (the export-star test certifying a dropped re-export) stands, so a human should look.

Comment thread test/bundler/bundler_cjs.test.ts Resolved

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed the updated PR and didn't find any bugs; because the one-line fix deliberately diverges from esbuild's requireOrImportMetaForSource and changes what await import() resolves to for existing bundles of linker-marked CommonJS files, a human look is still worthwhile.

What was reviewed:

  • async is only ever emitted for the WrapKind::Esm arm in src/bundler/linker_context/generateCodeForFileInChunkJS.rs (~647-652); the __commonJS wrapper is a plain function, so gating is_wrapper_async on Esm matches the only async wrapper the linker prints, and src/js_printer/lib.rs:2791 is its sole consumer.
  • The sibling static-import site in src/bundler/LinkerContext.rs (~2344-2379) already awaits init() only inside the WrapKind::Esm arm, so no other is_async_or_has_async_dependency reader needed the same guard.
  • The amended export-star test (085bbaa) now asserts the full namespace including the re-exported t, addressing the earlier inline note; the test comment records that t is observable only because the entry's await lands a microtask after the un-awaited init_async().
Extended reasoning...

The diff adds one condition in require_or_import_meta_for_source in src/bundler/LinkerContext.rs so is_wrapper_async is true only for __esm-wrapped files, plus five itBundled cases in test/bundler/bundler_cjs.test.ts. It touches no security-sensitive surface. The change is small and consistent with where the linker actually emits async wrappers, and the follow-up commit addressed the prior inline comment, but it intentionally departs from esbuild's ported logic and alters runtime results for some bundles that currently run (a module.exports function now resolves to a namespace), which is a product-semantics call better confirmed by a maintainer. No third-party CHANGES_REQUESTED review is present and the changed paths are not covered by CODEOWNERS.

@robobun

robobun commented Oct 29, 2026

Copy link
Copy Markdown
Collaborator Author

#41601 changed at 9eda6aac4d6. It is an order-only change now. The dependency statements of a wrapper keep source order when none of them is for an async dependency, and a list with an async dependency is printed as on main. It no longer awaits an export *, no longer replaces __promiseAll, no longer changes the import() form, and no longer sets the top-level await flag.

So the line "#41601 makes that export * wait" in the body of this PR does not hold any more.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant