Repository navigation
Conversation
The __esm helper cleared its init function before calling it, so a throw during evaluation left the module recorded as successfully initialized. A second import() then fulfilled with the partially built namespace instead of re-rejecting. ECMA-262 memoizes a failed module evaluation, so box the thrown value and re-throw it on every later call. __commonJS had the same defect: `mod` was installed before the callback ran, so a throw left the partial exports in the cache. Node removes a module from require.cache when its evaluation throws, so reset `mod` on throw and let the next require() re-evaluate. Matches esbuild 0.28.1 (evanw/esbuild#4461).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
Walkthrough
ChangesESM/CJS evaluation error semantics
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 6:32 AM PT - Jun 29th, 2026
❌ @robobun, your commit 0e9305c has 1 failures in
🧪 To try this PR locally: bunx bun-pr 33053That installs a local version of the PR into your bun-33053 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The minified __commonJS helper grew by 37 bytes, which shifts the output file size and every line-1 generated column by the same amount. The new values were re-derived from the generated sourcemap; the mapped text at each position is unchanged, and the pins on later lines are unaffected.
There was a problem hiding this comment.
Thanks for updating bundler_npm.test.ts — the +37 delta is consistent across the file size and both line-1 column offsets, so that looks right. The runtime.js change itself reads correctly and matches esbuild's fix, but since __esm/__commonJS are emitted into every bundle and this changes error-path semantics, I'd like a human to give it a quick look.
Extended reasoning...
Overview
This PR fixes spec-compliance bugs in two bundler runtime helpers in src/runtime.js:
__esmnow memoizes a thrown error in a boxed closure-local (err = [e]) and re-throws it on every later call, matching ECMA-262's requirement that a failed module evaluation rejects all subsequent imports with the same error.__commonJSnow resetsmod = 0on throw so the nextrequire()re-evaluates, matching Node's behavior of removing a throwing module fromrequire.cache.
Both changes mirror esbuild 0.28.1's fix for the same defect. Two new tests in bundler_edgecase.test.ts cover the behavior. The remaining changes are mechanical: regenerated inline snapshots in three test files, a bumped VLQ segment in mappingsExactMatch, and — added in commit ba49831 after my prior comment — updated pinned file size and line-1 column offsets in bundler_npm.test.ts. The +37-byte delta is internally consistent across the file size (221969→222006) and both column offsets (5623→5660, 7685→7722), which strongly suggests these were obtained by re-running the test rather than guessed.
Security risks
None. This is error-handling control flow in generated bundle preamble code; no auth, crypto, I/O, or untrusted input is involved.
Level of scrutiny
Moderate-to-high. src/runtime.js is emitted verbatim into every bundle Bun produces, so the blast radius is the entire user base. The logic is small and follows esbuild's proven implementation, but it changes observable runtime semantics (what happens on the second import()/require() of a module that threw). A human reviewer familiar with the bundler should confirm the helper signatures still match every emission site (the PR asserts both are only ever called with one argument, which I spot-checked and looks right) and that the TLA path — where fn is async and res becomes a rejected promise — is indeed unaffected as claimed.
Other factors
- My prior inline comment about
bundler_npm.test.tshas been addressed in the latest commit. - No bugs were found by the bug-hunting system on this revision.
- The new tests are well-targeted: they verify rejection on both attempts, error identity (
errors[0] === errors[1]) for ESM, and re-evaluation count (evaluations: 2) for CJS. - The vendored
__commonJScopies insrc/js/were intentionally left untouched per the PR description; that's reasonable since they wrap single non-throwing modules.
There was a problem hiding this comment.
Thanks for the bundler_npm.test.ts update — that resolves my earlier comment and I see no remaining issues, but since src/runtime.js ships in every bun build output I'd like a maintainer to sign off on the helper-semantics change.
Extended reasoning...
Overview
This PR rewrites the __esm and __commonJS runtime helpers in src/runtime.js so a module whose evaluation throws is not cached as a success. __esm now memoizes the thrown value (boxed in a closure-local array so falsy throws still count) and re-throws it on every later call, matching ECMA-262's module-evaluation semantics. __commonJS resets mod = 0 on throw so the next require() re-evaluates, matching Node's require.cache removal behavior. The fix mirrors esbuild 0.28.1 (evanw/esbuild#4461). Two new itBundled tests cover both helpers, and four snapshot/pin files were regenerated for the larger helper bodies. My earlier comment about bundler_npm.test.ts not being updated was addressed in ba49831 (filesize 221969→222006, line-1 columns +37) and is now resolved.
Security risks
None. The helpers wrap user-provided module bodies and only change error-caching behavior; no new attack surface, no auth/crypto/permission code touched.
Level of scrutiny
High. src/runtime.js is the bundler runtime preamble — these two helpers are inlined into every non-splitting bundle that contains a CJS module or a lazily-evaluated ESM module. A regression here would silently break all downstream bun build output. The change is small and the logic is sound (I verified the bundler only ever emits __esm(fn) / __commonJS(cb) with one argument, so the new closure-local parameters are always undefined initially), but the blast radius warrants a maintainer's eyes rather than bot-only approval.
Other factors
- New tests
edgecase/ESMEvaluationThrowIsMemoizedandedgecase/CommonJSEvaluationThrowIsNotCacheddirectly exercise the fixed paths and assert evaluation counts and error identity. - The lone CI failure (
test/js/node/test/parallel/test-net-connect-memleak.json x64/x64-baseline) is unrelated to bundler runtime; it's a known-flaky net test. - The vendored private
__commonJScopies insrc/js/{node/wasi.ts,node/querystring.ts,internal/fs/glob.ts}are intentionally untouched, as noted in the PR description. - No CODEOWNERS entry covers
src/runtime.js.
|
CI status: the change is green; both CI runs are blocked by a pre-existing flake unrelated to this PR. Across build 66710 and the clean re-run 66732, the only test failure is That is #33044 ("CI: test-net-connect-memleak.js fails on half of PR builds on linux-x64-musl since June 28 ~23:00 UTC"), which was opened before this PR existed. It is a For completeness, build 66710 also had one Everything related to this change passes locally against the debug build: the new This is ready for review; it just needs a maintainer to merge past (or re-kick) the #33044 flake. I am not going to keep pushing empty retriggers at it. |
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-06-29, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
1 participant
bun buildwithout--splittingemits a lazy evaluator for dynamically imported ESM modules that clears its init function before calling it:If the module body throws,
fnis already0, so the module is recorded as successfully initialized withres = undefined. The nextimport()skips evaluation and resolves with whatever part of the namespace existed at the throw. ECMA-262 memoizes a failed module evaluation, so every later import must reject with the same error. Node, Bun's own runtime on unbundled source, and the--splittingoutput all behave correctly; only the non-splitting bundle is wrong.__commonJShas the same defect:modis installed before the callback runs, so a CJS module that throws during evaluation hands its partialexportsto the nextrequire()as a success. Node removes a module fromrequire.cachewhen its evaluation throws, so a laterrequire()must re-evaluate it.Fix
Match esbuild, which fixed both helpers in 0.28.1 (evanw/esbuild#4461):
__esmboxes the thrown value in a third closure-local and re-throws it on every later call. Top-level-await modules were already correct (the rejected promise is memoized inres) and are unaffected.__commonJSresetsmodto0on throw so the nextrequire()re-evaluates.Both helpers are only ever emitted with one argument, so the added closure-local parameters are always
undefinedon the first call. The private__commonJScopies inside the vendored single-module bundles insrc/js/(wasi, querystring, glob's minimatch) are untouched: each runs once at module init and never throws.Three test files embed the runtime helper source in snapshots and were regenerated. Two tests pin values derived from the minified runtime preamble, which grew with the helper body; the new values were re-derived from the generated output and sourcemap, and in each case the mapped text at the new position is unchanged:
bundler_edgecase.test.ts: themappingsExactMatchstring's leading generated-column VLQ.bundler_npm.test.ts(npm/ReactSSR):expectExactFilesize(221969 -> 222006) and the two line-1 generated columns insnapshotSourceMap.mappings(+37 each). Pins on later output lines are unaffected since a newline resets the column.Tests
edgecase/ESMEvaluationThrowIsMemoizedandedgecase/CommonJSEvaluationThrowIsNotCachedintest/bundler/bundler_edgecase.test.ts. Both fail on the released bun (the second import reportsFULFILLED {"v":1}/loaded {"v":1}) and pass with the fix. With the fix, the bundled output matches node and esbuild for ESMimport(), CJSimport(), and CJSrequire(), including under--minify.