Repository navigation
Conversation
…ESM wrappers An unwrapped file with two or more import statements of lazily wrapped modules that contain top-level await prints await __promiseAll([init_a(), init_b()]), but only create_wrapper_for_file's WrapKind::Esm arm marked the helper as used. The bundle then threw ReferenceError: __promiseAll is not defined at load. Count these imports per importing file in step 6 of scan_imports_and_exports, where the other runtime helper uses are registered, and drop the wrapper-only count it supersedes.
|
Updated 2:30 PM PT - Sep 8th, 2026
❌ @robobun, your commit bfba7d2 has 1 failures in 🧪 To try this PR locally: bunx bun-pr 42046That installs a local version of the PR into your bun-42046 --bun |
|
Status Reproduced on 1.4.2 and 1.4.3-canary.1+f42e98025 with the three-file input in the PR body: Test proof: CI (build 113226) at bfba7d2: the new tests and the rest of |
|
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 (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review. WalkthroughChangesPromise aggregation linking
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The bundler now emits the promise aggregation helper when an unwrapped module statically initializes multiple async ESM wrappers, preventing the prior runtime ReferenceError. Regression coverage includes helper generation and tree-shaking cases, with no current merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this and didn't find any bugs; the counter relocation follows the neighboring to_esm_uses/preload_uses pattern exactly and the soundness argument (every part holding a 2nd+ qualifying import carries the use, so a printed __promiseAll always has a live definition) holds. Given this is linker tree-shaking liveness bookkeeping and overlaps with the in-flight #41601 rework, a maintainer glance would still be worthwhile.
What was reviewed:
- Verified
generate_runtime_symbol_import_and_useearly-returns oncount == 0(LinkerGraph.rs:585), so the unconditional call withpromise_all_usesis safe. - Checked the new guard (
ImportKind::Stmt+WrapKind::Esm+is_async_or_has_async_dependency+ validwrapper_ref) matches the printer'sappend_async_dependencyreach condition at LinkerContext.rs:2301. - Traced the file-scoped counter vs. per-part use split against part-liveness edge cases — over-inclusion only, never under-inclusion.
- New
itBundledcases assert both runtime output and emitted-code shape in both directions (helper present/absent).
Extended reasoning...
Overview
The PR relocates where the bundler's linker records a use of the __promiseAll runtime helper. Previously only create_wrapper_for_file's WrapKind::Esm arm counted async import records and marked the helper as used on the wrapper part; that block is deleted. Instead, step 6 of scan_imports_and_exports now maintains a file-wide async_esm_init_count and, from the second qualifying static import of an async ESM wrapper onward, sets promise_all_uses = 1 on the current part, registered via the same generate_runtime_symbol_import_and_use call chain used for __toESM/__toCommonJS/__require/__preload. This makes the helper survive tree-shaking when an unwrapped importer (entry or intermediate) statically imports two or more async ESM wrappers — the case where the printer emits await __promiseAll([...]) at chunk top level but the old bookkeeping had no wrapper part to hang the use on, producing a ReferenceError at runtime. Five new itBundled cases cover both directions.
Security risks
None. This is compile-time bundler bookkeeping deciding whether a 45-byte runtime helper definition is emitted. No untrusted input parsing, no auth/crypto/permissions, no network or filesystem effects beyond what itBundled already sandboxes.
Level of scrutiny
Moderate. The Rust change is small (two hunks) and mechanically follows the established local pattern — I confirmed generate_runtime_symbol_import_and_use no-ops on count == 0 at LinkerGraph.rs:585, so the unconditional call is safe. The new condition sits inside the other_flags.wrap != WrapKind::None && wrapper_ref.is_valid() block and gates on ImportKind::Stmt + WrapKind::Esm + is_async_or_has_async_dependency, which matches exactly what the printer checks before calling append_async_dependency at LinkerContext.rs:2301-2304. That said, this is linker tree-shaking liveness — a subsystem where part-liveness, wrap decisions, and code-splitting interact subtly, and where an alternative design (#41601, replacing the helper with unbound Promise.all) is in flight. A maintainer familiar with that rework should confirm this doesn't conflict.
Other factors
I traced the file-scoped-counter / per-part-use split against tree-shaking edge cases: since every part containing the 2nd or later qualifying import carries the use, and a part must be live to be printed, a printed __promiseAll call always has a live definition. The reverse — a defined but unused helper when one of the counted import parts is later tree-shaken or when the second qualifying record is an un-aliased export * from — is a benign 45-byte over-inclusion the PR description already acknowledges. The old code also over-counted by including dynamic import() records; the new ImportKind::Stmt gate is strictly more precise, and the fifth test verifies a wrapped importer whose second async dep is import() no longer emits the unused helper. Tests follow test/CLAUDE.md conventions (existing file, itBundled, both run and onAfterBundle shape assertions, no ports/timeouts/network). No CODEOWNERS entry covers src/bundler/. No outstanding reviewer objections in the timeline.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This branch has not been deployed
1 participant
Problem
import()ed printsawait __promiseAll([...]), but the bundle never defines the helper.bun buildexits 0 and the output throwsReferenceError: __promiseAll is not defined.should_remove_import_export_stmt(src/bundler/LinkerContext.rs:2301) joins the awaits for any importer. Only theWrapKind::Esmarm ofcreate_wrapper_for_file(LinkerContext.rs:3541) marked__promiseAllas used, so for an unwrapped importer tree shaking dropped the helper.Fix
scan_imports_and_exportsnow counts, per importing file, theimportstatements whose target is an async ESM wrapper. From the second one on, the part gets a__promiseAllruntime use, next to the__toESMand__reExportuses.create_wrapper_for_fileis removed. The step 6 count covers wrapped importers too, and skipsimport()andrequire()records, which never join theawait.test/bundler/bundler_promiseall_deadcode.test.ts(5 new cases, 4 fail on stock bun), plus six neighbouring suites (see Notes).Promise.alland delete the helper, the shape bundler: evaluate a wrapped module's dependencies in source order and await every async one #41601 proposes, see Notes).Background
--splitting, a module that somethingimport()s is wrapped:var init_a = __esm(async () => {...}). A staticimportof it becomesinit_a(), awaited when the module or a dependency has top-level await.__promiseAll(src/runtime.js).generate_runtime_symbol_import_and_use).Notes
Repro (1.4.2 and 1.4.3-canary.1+f42e98025):
With this change the same bundle defines
var __promiseAll = (args) => Promise.all(args);and printsp,q,entryunder bun and node, with and without--minify.Relationship to #41601: that open PR reworks the order in which a wrapper evaluates its dependencies and replaces
__promiseAllwith an unboundPromise.all, which removes this bug as a side effect. This PR is the standalone fix for theReferenceErrorand does not change evaluation order. If #41601 lands first, this one is redundant and I will close it. Earlier standalone attempts were #36189 (closed in favour of #33337) and #33337 (closed in favour of #41601), so the error is still onmain.Alternative shape, considered and not taken here: print
await Promise.all([...])through an unboundPromisesymbol (the renamer already reservesPromise, andunbound_module_refis the pattern) and delete__promiseAllfromsrc/runtime.jsandsrc/ast/runtime.rs. That removes the liveness bookkeeping instead of adding a counter, but it drops the helper #22704 introduced and renumbers the runtime import table, and #41601 carries exactly that change. This PR keeps the helper so that the fix stays two hunks.Precision of the count. The count runs over all parts of a file before tree shaking. The printer joins the
awaits within one printed part range. The two differ only toward a defined but unused 45-byte helper, in two cases: one of the two import parts is tree-shaken or lands in a separate part range, or one of the statements is an un-aliasedexport * fromof an async wrapper. That last form does not go throughshould_remove_import_export_stmt:convertStmtsForChunk.rs:273prints a bareinit_x()for it with noawait(esbuild prints the same), so a consumer of such a re-export can read its bindings before the module finished. That is a separate, pre-existing ordering bug which #41601 addresses, and this PR leaves that site alone. The reverse mismatch, a printed__promiseAllwith no definition, cannot happen: every part that holds the second or a later qualifying import carries the use, and a part must be live to be printed.New tests, all in
test/bundler/bundler_promiseall_deadcode.test.ts:import(): stock bun emitted an unusedvar __promiseAll, now it does notThe three existing cases in that file (wrapped importers from #22704) are unchanged and pass, including the inline snapshot that contains
var __promiseAll.Other suites run with the debug build, all green:
bundler_edgecase,bundler_dynamic_import_dce,bundler_cjs2esm,bundler_splitting,bundler_regressions,esbuild/default.[human-review] gate passed · iteration 0 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file