module loader: evaluate ESM-imported CommonJS at its post-order slot - #35971
module loader: evaluate ESM-imported CommonJS at its post-order slot#35971robobun wants to merge 24 commits into
Conversation
An ESM importing a CommonJS file previously ran the CommonJS body the instant its async transpile settled (inside the SyntheticSourceProvider generator called from JSModuleLoader::makeModule). That is before graph load completes, before link(), and before any ESM sibling's evaluate(), so CommonJS dependencies observed state from before earlier ESM siblings ran, and with multiple CommonJS siblings the relative order was transpile-completion order (nondeterministic). Instead, createCommonJSModule now returns a small ESM wrapper JSModuleRecord that only declares the static export-name table. InnerModuleEvaluation reaches it at the correct depth-first post-order position; GlobalObject::moduleLoaderEvaluate detects the backing JSCommonJSModule via the require map, executes the CommonJS body there, and writes the resulting values into the wrapper's module environment. The static export-name table is built by the transpiler at runtime from exports.X / module.exports.X assignments and Object.defineProperty(exports, "X", ...). Names that are only introduced at runtime (bracket assignments, __exportStar re-exports) are no longer directly named-importable, matching Node's cjs-module-lexer semantics; they remain reachable on the default export. When no static information is available (transpiler-cache hits, cached SourceProviders) the previous fetch-time path is kept.
WalkthroughChangesCommonJS export names are detected during parsing, propagated through resolved sources and runtime caches, and consumed by JSC-generated ESM wrappers. Deferred evaluation now populates ESM bindings, with tests covering export shapes, ordering, getters, re-exports, shims, and errors. CommonJS-to-ESM interoperability
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 8:02 AM PT - Jul 27th, 2026
✅ @robobun, your commit 0cc7e3f8dbe22c1842c1cd059d44d0748b5baa71 passed in 🧪 To try this PR locally: bunx bun-pr 35971That installs a local version of the PR into your bun-35971 --bun |
|
comment-cop: trimmed the first round in b7a8405. The remaining flagged comments are 2-3 line docs on FFI struct fields, the empty-vs-single-NUL sentinel encoding in |
Status at 08ada58Dropped Defers (ordering fixed, names from static scan): a CommonJS file whose export set is fully expressed by Falls back to eager (same named-import surface as main, same ordering as main): Node parity for the flagged packages is unchanged (tsyringe/@nestjs/common/@testing-library/react/react/lodash all import by name); the six fixture workarounds remain reverted. The full recursive name lexer (resolve + parse each re-export target with require semantics, declare each name as a local binding read from the parent's runtime |
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/js_parser/p.rs:5299-5303— The recursive peel here matches any single-argument call wrapping arequire, but the doc comment claims cjs-module-lexer's REQUIRE grammar, which only peels a fixed interop whitelist (_interopRequireDefault/_interopRequireWildcard/__importStar/__importDefault). Sovar x = anyFn(require('y'))recordsx → 'y'incommonjs_require_bindings, and a subsequentObject.keys(x).forEach(k => exports[k] = x[k])(or__exportStar(anyFn(require('y')), exports)) emitsexport * from 'y'—import { foo }then readsy's raw value whiledefault.fooreads the transformed one. Gate the peel oncall.targetbeing an identifier whoseoriginal_nameis in the interop whitelist.Extended reasoning...
What the bug is
require_specifierhandles the case where the expression is anECallwhose target is notrequireby unconditionally recursing on the sole argument:// Peel a wrapping call: `_interopRequireWildcard(require("x"))` let args = call.args.slice(); if args.len() == 1 { return self.require_specifier(&args[0]); }
Nothing checks the wrapper's name. The doc comment on the function says "cjs-module-lexer REQUIRE grammar" and the inline comment names
_interopRequireWildcardspecifically, but the code peels any single-arg call —transform(require('y')),freeze(require('y')),wrap(require('y'))all returnSome(b"y").Node's cjs-module-lexer restricts the tracked
var X = REQUIREbinding to barerequire(STRING)(and, in the EXPORT_STAR_LIB position, additionally accepts_interopRequireWildcard(require(STRING))). It does not peel arbitrary wrappers.The specific code path
require_specifierhas three call sites in this PR:- visit_stmt.rs:1156 —
var x = <expr>records(x_ref, spec)intocommonjs_require_bindingswhenrequire_specifier(&d.value)returnsSome. - visit_expr.rs:2048 —
__exportStar(<expr>, exports)recordsspecintocommonjs_reexport_specifierswhenrequire_specifier(&args[0])returnsSome. - visit_expr.rs:2086 —
Object.keys(<expr>).forEach(...)first triesrequire_specifier(&inner_args[0]), then falls back to looking up the identifier incommonjs_require_bindings.
Paths (1)+(3) and path (2) both reach
record_runtime_commonjs_reexport, which flows throughjoin_commonjs_export_names→m_reexportSpecifiers→ anexport*from"y";clause in the wrapper source.Step-by-step proof
Given
lib.cjs:var x = transform(require('./y.cjs')); Object.keys(x).forEach(function (k) { exports[k] = x[k]; });
- visit_stmt.rs visits
var x = transform(require('./y.cjs')).d.valueisECall{ target: EIdentifier(transform), args: [ERequireString('./y.cjs')] }.require_specifierseesECall, target is notERequireCallTargetand notrequire_ref, so it falls through to the peel:args.len() == 1→ recurse onargs[0]→ERequireStringarm returnsSome(b"./y.cjs").(x_ref, "./y.cjs")is pushed ontocommonjs_require_bindings. - visit_expr.rs visits the
Object.keys(x).forEach(...)call.dot.name == b"forEach",args.len() == 1, and (after 5448888)callback_writes_exportsis true — the body hasSExpr(EBinary{ op: BinAssign, left: EIndex{ target: exports } }).inner.targetisObject.keys,inner_args[0]isEIdentifier(x_ref), sorequire_specifierreturnsNoneand the.or_elselooks upx_refincommonjs_require_bindings→"./y.cjs".record_runtime_commonjs_reexport(b"./y.cjs")fires. - join_commonjs_export_names produces
\x01./y.cjs; assignStaticExportNames puts"./y.cjs"intom_reexportSpecifiers; commonJSModuleSyntheticSourceCode emitsexport*from"./y.cjs";. - At link time,
import { foo } from './lib.cjs'resolvesfooas an indirect binding into./y.cjs's wrapper — the rawy.foo. But at runtimelib.cjspopulatedexports.foo = transform(require('./y.cjs')).foo, so(await import('./lib.cjs')).default.foois the transformed value. The named import anddefault.foosilently diverge.
Node's cjs-module-lexer would not record
xas a require binding here (transform(...)is not in its grammar), so theObject.keysloop would not be detected as a re-export, andfoowould not be named-importable at all — matching pre-PR Bun.Why existing code doesn't prevent it
The
callback_writes_exportscheck added in 5448888 only verifies the forEach callback writes toexports; it doesn't (and shouldn't) verify that the source binding was a barerequire. The three callers ofrequire_specifierall trust its return value as "this expression is semanticallyrequire(spec)".Impact and severity
Nit rather than normal: no known toolchain emits a non-interop wrapper around
require()that then feeds anObject.keys().forEachre-export loop or__exportStar— Babel/TS/SWC/esbuild all use the whitelisted interop names. The trigger requires hand-written CJS combining both shapes. Additionally, the unresolved comment onJSCommonJSModule.cpp:1669already calls for reworking theexport * frommechanism to declare local bindings filled from the parent's runtimemodule.exports(Node's names-only approach), which would neutralize the wrong-value symptom here — the name set would be over-broad but values would agree withdefault. Still worth gating: the fix is one predicate, and the function's own doc comment claims a grammar it doesn't implement (REVIEW.md: "the reference implementation is the spec").How to fix
Before recursing, require the wrapper to be a known interop helper:
// Peel a wrapping interop call: `_interopRequireWildcard(require("x"))` if let ExprData::EIdentifier(id) = &call.target.data { let name = self.symbols.as_slice()[id.ref_.inner_index() as usize] .original_name .slice(); if matches!( name, b"_interopRequireDefault" | b"_interopRequireWildcard" | b"__importStar" | b"__importDefault" | b"__toESM" ) { let args = call.args.slice(); if args.len() == 1 { return self.require_specifier(&args[0]); } } } None
- visit_stmt.rs:1156 —
…robe; drop dead ESpecial::ModuleExports runtime arms
47034fb to
ff50b38
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/jsc/RuntimeTranspilerStore.rs (1)
974-990: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPersist the CommonJS predicate in cache metadata. A module that is CommonJS only because
has_commonjs_export_namesis true is written asEsm, so cache hits skipcommonjs_export_namesand take a different path from the initial run. Store the full predicate, not justexports_kind.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/jsc/RuntimeTranspilerStore.rs` around lines 974 - 990, Update the cache metadata write path associated with the resolved source and `is_commonjs_module` so it persists the complete CommonJS predicate, including `has_commonjs_export_names`, rather than deriving it only from `module_type == CacheModuleType::Cjs`; ensure cache hits retain and use the same CommonJS classification and `commonjs_export_names` behavior as initial runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js_parser/fold.rs`:
- Around line 397-424: Ensure unrecognisable CommonJS export shapes deopt
instead of being silently ignored: in src/js_parser/fold.rs lines 397-424, set
handled only when every object property is normal, non-computed, non-spread, and
has a non-UTF-16 string key; otherwise leave it false so
commonjs_module_exports_assigned_deoptimized is set. In
src/js_parser/visit/visit_expr.rs lines 2039-2044, change the fallback of the
Object.defineProperty export-name match from Action::None to Action::Deopt. In
src/js_parser/visit/visit_expr.rs lines 2058-2098, inspect nested callback
statement bodies for exports writes, or return Action::Deopt whenever an
Object.keys(...).forEach callback references exports.
In `@src/js_parser/p.rs`:
- Around line 5225-5241: Update record_runtime_commonjs_export_name so the
fallible CommonJSNamedExports::put result is handled without expect; route
allocation failure through bun_core::handle_oom, preserving the existing
early-return validation and successful insertion behavior.
In `@src/js_parser/visit/visit_expr.rs`:
- Around line 2039-2044: Update the defineProperty argument handling in the
visible match so UTF-16 EString names return Action::Deopt instead of falling
through to Action::None, while preserving Action::Record for non-UTF-16 strings
and Action::None for other argument types.
- Around line 1988-1991: Update the guard in the relevant visit-expression
branch to use p.should_unwrap_common_js_to_esm() consistently with the
name-recording branches in fold.rs, preserving the existing CommonJS,
control-flow, and other conditions. Ensure the gate matches the predicate used
when recording export names so deoptimization is triggered consistently.
In `@src/js_printer/lib.rs`:
- Around line 7745-7758: Separate the cache payloads used for ESM records and
CJS export names at the cache write site in the flow containing cjs_export_names
and esm_record, so the reader’s metadata.module_type interpretation cannot
deserialize one as the other. Prefer distinct cache fields or payload slots; if
the existing slot must remain shared, document the invariant tying
have_module_info to metadata.module_type directly at this write site.
In `@src/jsc/bindings/JSCommonJSModule.cpp`:
- Around line 1742-1752: In the static export loop that assigns each `$eN`
binding via symbolTablePutTouchWatchpointSet, validate putResult after the call
and add a debug-build assertion for symbol-table mismatches. Preserve the
existing exception handling and avoid changing the binding assignment behavior
in release builds.
In `@src/jsc/ResolvedSource.rs`:
- Around line 92-104: Update the key-joining logic around
ast.commonjs_named_exports to detect keys containing a NUL byte and skip them or
deopt before framing. Ensure no emitted joined buffer contains a key-internal
NUL, while preserving the existing separator behavior for valid export names.
In `@test/cli/run/esm-defineProperty.test.ts`:
- Line 43: Strengthen the assertion for CJSArrayLike.default[4] to verify the
getter throws the expected Error with message "4", rather than accepting any
thrown error. Update the test assertion around CJSArrayLike.default[4] while
preserving its existing access behavior.
In `@test/js/bun/resolve/esm-import-cjs-order.test.ts`:
- Around line 72-87: Strengthen the fixtures in the test named “two CJS siblings
evaluate in source order (was transpile-race nondeterministic)” by padding a.cjs
with sufficient work or content so b.cjs settles first under the old eager
transpilation path. Preserve the expected source-order result ["a", "b"] and
ensure the test reliably fails when the ordering fix is removed.
---
Outside diff comments:
In `@src/jsc/RuntimeTranspilerStore.rs`:
- Around line 974-990: Update the cache metadata write path associated with the
resolved source and `is_commonjs_module` so it persists the complete CommonJS
predicate, including `has_commonjs_export_names`, rather than deriving it only
from `module_type == CacheModuleType::Cjs`; ensure cache hits retain and use the
same CommonJS classification and `commonjs_export_names` behavior as initial
runs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fc0a239c-c369-4f31-aae9-8a6d1cf5c614
📒 Files selected for processing (18)
src/ast/transpiler_cache.rssrc/js_parser/fold.rssrc/js_parser/p.rssrc/js_parser/parse/parse_entry.rssrc/js_parser/visit/visit_expr.rssrc/js_printer/lib.rssrc/jsc/AsyncModule.rssrc/jsc/ResolvedSource.rssrc/jsc/RuntimeTranspilerCache.rssrc/jsc/RuntimeTranspilerStore.rssrc/jsc/bindings/JSCommonJSModule.cppsrc/jsc/bindings/JSCommonJSModule.hsrc/jsc/bindings/ZigGlobalObject.cppsrc/jsc/bindings/ZigSourceProvider.cppsrc/jsc/bindings/headers-handwritten.hsrc/runtime/jsc_hooks.rstest/cli/run/esm-defineProperty.test.tstest/js/bun/resolve/esm-import-cjs-order.test.ts
…mized should_unwrap_common_js_to_esm() is ESM-only, but ESpecial::ModuleExports is emitted under !commonjs_named_exports_deoptimized (ESM, IIFE, InternalBakeDev). Match the emission condition so --format=iife keeps tracking the reassign. The runtime path has commonjs_named_exports_deoptimized=true so the setter stays off there.
There was a problem hiding this comment.
No bugs found this pass, but this needs a maintainer's sign-off: it changes how ESM-imported CJS resolves named exports (runtime enumeration → static cjs-module-lexer-style scan) and knowingly drops cross-file __exportStar following, which the PR description itself notes regresses import { singleton } from "tsyringe" vs both Node and current Bun. That's a Node-compat / ecosystem trade-off a human should own.
What was reviewed:
evaluateDeferredCommonJSModuleForESMexception handling, GC rooting ofm_staticExportNames(Identifier is refcounted, not GC), and thesymbolTablePutTouchWatchpointSetwrite path.ResolvedSource.commonjs_export_namesownership on every path (createCommonJSModule,~SourceProvider,evaluateWithPotentiallyOverriddenCompile,OwnedResolvedSource::Drop) — balanced.- Parser gate changes in
parse_entry.rs/p.rsdon't reclassify pure-ESM files (recorders are behindcommonjs_at_runtimeand the newshould_unwrap_common_js_to_esm()guards). - Transpiler-cache version bumped for the repurposed
esm_recordblob.
Extended reasoning...
Overview
~830-line change across 18 files spanning the runtime JS parser (fold.rs, visit_expr.rs, p.rs, parse_entry.rs), the Rust→C++ ResolvedSource FFI struct, the runtime transpiler cache format (version 23→24), and the JSC module-loader bindings (JSCommonJSModule.cpp, ZigGlobalObject.cpp). The core mechanism replaces the fetch-time SyntheticSourceProvider for ESM-imported CJS with a real JSModuleRecord wrapper whose bindings are filled from moduleLoaderEvaluate, so CJS bodies run at their spec post-order slot.
Security risks
None identified. Untrusted input reaching the new code is JS source text via the existing parser; export names are filtered for empty/NUL at record time and JSON-quoted in the generated wrapper source. No auth/crypto/permissions surface.
Level of scrutiny
High. The module loader is a hot, correctness-critical path, and the change carries an explicit user-visible behavior change: named imports from CJS now come from a static scan rather than runtime enumeration. That is closer to Node's model but strictly narrower than pre-PR Bun for shapes the scanner doesn't follow (notably tslib __exportStar cross-file). Earlier iterations rewrote existing tests (fixtures/tsyringe.ts, esm-defineProperty.test.ts) to accommodate the new surface — a compat decision that per REVIEW.md ("never edit a test to route around a runtime bug it exposed") should be owned by a maintainer, not auto-approved.
Other factors
This PR has been through seven review iterations; every prior inline finding (double-getter-invocation, ~SourceProvider leak, cache-hit path missing names, is_deoptimized_commonjs misclassification, IIFE/bake exports-assign printer regression, zero-names lodash case, _compile path leak) has been addressed and every thread is resolved. The remaining reason to defer is not code quality — it's that the acknowledged __exportStar gap and the side-effect-only-CJS ordering carve-out are product decisions.
|
A shape that stays on the eager fallback in the current revision: side-effect-only CommonJS. // dep.cjs
require("./polyfill.mjs");
module.exports = {};fold.rs treats Since the zero-names fallback exists for UMD-alias files the deopt cannot see (lodash's Separately: once CJS bodies run in the evaluate walk, a body that |
Problem
When an ESM graph imports a CommonJS file, Bun runs the CommonJS body the instant its async transpile settles:
createCommonJSModulereturns aSyntheticSourceProviderwhose generator callsevaluateCommonJSModuleOnce, andJSModuleLoader::makeModuleinvokes that generator from theModuleRegistryFetchSettledmicrotask. That is before graph load completes, beforelink(), and before anyevaluate().SyntheticModuleRecord::evaluate()returnsundefined, so the spec'sInnerModuleEvaluationwalk only ever runs the ESM siblings.Two user-visible effects:
dep.cjsruns first and seesundefined, 20/20 under 1.4.0-canary. Theimport './load-env.mjs'; import { url } from './db.cjs'dotenv shape is the same failure.importfetches go to the concurrent transpiler pool; each settle immediately runs the CJS body, soa/bexecute in transpile-completion order instead of source order. Locally 18/20["a","b"], 2/20["b","a"].Node executes CJS-in-ESM during the evaluate walk (depth-first post-order over
[[RequestedModules]]), so both shapes are deterministic there.Fix
When the runtime transpiler can express a CommonJS file's export set statically,
createCommonJSModulereturns a tiny ESM wrapper (a realJSModuleRecord) that only declares the export-name table:InnerModuleEvaluationreaches it at the correct post-order position.GlobalObject::moduleLoaderEvaluate(whichJSModuleRecord::executealready calls) detects the backingJSCommonJSModulevia the require map, runsevaluateCommonJSModuleOncethere, and writes the real values into the wrapper's module environment withsymbolTablePutTouchWatchpointSet. No JavaScriptCore changes needed.The export-name table is built by the runtime transpiler from
exports.X/module.exports.Xassignments,Object.defineProperty(exports, "X", ...), andmodule.exports = { X, Y }. The names cross to C++ as a NUL-joinedBunStringonResolvedSource, threaded through the transpiler cache (version bumped).Files whose export set depends on runtime behaviour keep the existing fetch-time
SyntheticSourceProviderpath:module.exports = <non-object>(react/react-dom prod/dev shims,module.exports = fn/class),__exportStar(require(x), exports)/Object.keys(_).forEach(k => exports[k] = ...)(TypeScript/Babel barrel re-exports), and files with zero detected exports (lodash-style UMD that assigns via a localmodulealias). Named imports from those files stay on runtime enumeration, same surface as main.Behaviour change
For a file that takes the deferred path, named exports are resolved from the static scan (matching Node's
cjs-module-lexerfor those shapes).Object.defineProperty(exports, "c", {get})is detected where the eager path only sees enumerable own properties, soimport { c }from the fixture inesm-defineProperty.test.tsnow returns the getter's value instead ofundefined; that test is updated accordingly. Files that fall back to eager keep their current named-import surface.The motivating ordering scenarios (
import './setup.mjs'; import './dep.cjs'withdep.cjsreading fromsetup) are deferred and run in source order. Files on the eager fallback keep main's ordering; in practice those are function/class exports and prod/dev pass-through shims whose body is side-effect-free.The full Node-compatible follow-up (recursively lex re-export targets with require semantics, declare each name as a local binding read from the parent's runtime
module.exports) would let the__exportStarpopulation defer too; that needs the resolver threaded into the loader before wrapper generation.Verification
Fixes the ordering for #4677.