Repository navigation
Conversation
…ple async ESM wrappers
When an entry (or any unwrapped file) statically imports two or more
top-level-await modules that are also dynamically imported, the linker
emits `await __promiseAll([init_a(), init_b()])` at the top level of the
chunk. The `__promiseAll` runtime helper was only marked as used by
`create_wrapper_for_file`, which runs for wrapped files and hits
`WrapKind::None => {}` for the file that actually emits the call, so
tree-shaking dropped the helper's definition and the bundle threw
`ReferenceError: __promiseAll is not defined` at load time.
Count static imports of async ESM-wrapped modules per file in step 6 of
`scan_imports_and_exports` (alongside the existing `__toESM` /
`__toCommonJS` / `__reExport` tracking) and pull in `__promiseAll`
from the part that observes the second such import.
|
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 selected for processing (2)
WalkthroughChangesThe linker now counts async wrapped-ESM initializations and conditionally emits Async ESM initialization
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Duplicate of #33337, which already carries the same |
There was a problem hiding this comment.
LGTM — targeted fix that mirrors the existing __toESM/__toCommonJS/__require/__reExport counting pattern in step 6.
What was reviewed:
- Condition matches the codegen path in
should_remove_import_export_stmt→append_async_dependency(Stmt kind,WrapKind::Esm, validwrapper_ref,is_async_or_has_async_dependency). is_async_or_has_async_dependencyandwrapare both finalized before step 6 runs (propagate_async_dependencies→scan_imports_and_exports).generate_runtime_symbol_import_and_useearly-returns oncount == 0, so the extra call is free on the common path.- Cross-part counter can only over-mark (harmless extra bytes), never under-mark; the single-import negative test guards the common case.
Extended reasoning...
Overview
Fixes ReferenceError: __promiseAll is not defined in bundled output when an unwrapped file (WrapKind::None) statically imports two or more async ESM-wrapped modules. Codegen coalesces the resulting await init_*() calls into await __promiseAll([...]), but the runtime helper dependency was only registered from create_wrapper_for_file, which is a no-op for WrapKind::None. The fix adds a per-file counter in step 6 of scan_imports_and_exports and calls generate_runtime_symbol_import_and_use(b"__promiseAll", ...) on the part where the counter reaches 2 — exactly the pattern already used for __toESM, __toCommonJS, __require, and __reExport in the same loop.
Correctness
I traced the ordering: validate_tla + propagate_async_dependencies run at LinkerContext.rs:819-832 before scan_imports_and_exports at :835, and wrap is set in steps 1-2, so by step 6 both other_flags.wrap and other_flags.is_async_or_has_async_dependency are final. The gate (kind == ImportKind::Stmt && wrap == WrapKind::Esm && is_async && wrapper_ref.is_valid()) matches the exact codegen path at LinkerContext.rs:2165-2200 that emits the __promiseAll reference. generate_runtime_symbol_import_and_use early-returns on count == 0 (LinkerGraph.rs:563), so the extra call adds no work in the vast majority of parts.
The counter is per-file and cumulative across parts, while promise_all_uses is per-part. If the same import record appears in multiple parts, or if the two async imports live in different parts and one is later tree-shaken, the worst outcome is that __promiseAll is included when only one await init_*() survives — a ~30-byte code-size no-op, never a runtime error. I could not construct a case where two live async init_* calls are emitted without the mark landing on at least one live part.
Security risks
None. This only affects which runtime helper stubs are copied into bundler output.
Level of scrutiny
Bundler linker is a hot/critical path, but the change is ~15 lines that slot into a well-established local pattern with four existing precedents in the same block. The risk surface is narrow: either the helper is still missing (caught by the four new run: tests, which execute the bundle) or it is over-included (harmless).
Other factors
Four new itBundled tests all execute the bundled output and assert exact stdout — the strongest possible check for this bug class. The negative test (single async import → not.toContain("__promiseAll")) guards against regressing tree-shaking. Existing __promiseAll tests in the same file are untouched.
|
Closed as duplicate of #33337, which carries the same scan_imports_and_exports fix for __promiseAll plus the import-order change and wider test coverage. |
What does this PR do?
Fixes
bun buildproducing a bundle that references the__promiseAllruntime helper without defining it, so the build exits 0 but the artifact throwsReferenceError: __promiseAll is not definedon first evaluation.Repro
Reproduces on any target, with or without
--minify, via CLI andBun.build(success: true, emptylogs).--splittingis unaffected. esbuild 0.28.1 builds and runs the same input correctly.Cause
The helper exists but is never marked used at the failing emission site:
src/runtime.js:328and resolved topromise_all_runtime_refatsrc/bundler/LinkerContext.rs:546-547.should_remove_import_export_stmt(LinkerContext.rs:2073, call at:2192-2195) hands each static import of an async ESM-wrapped module toInsideWrapperPrefix::append_async_dependency(:4300), which on the second async dependency rewritesawait init_a()intoawait __promiseAll([init_a(), init_b()])(:4354-4400). This runs for every importer, wrapped or not.promise_all_runtime_refis marked used iscreate_wrapper_for_file(:3120) inside theWrapKind::Esmarm (count at:3239-3260,generate_symbol_import_and_useat:3328-3339), i.e. only when the importer is itself lazily wrapped.WrapKind::None => {}(:3342) does nothing, so an unwrapped importer (the entry, or a statically linked intermediate) references the helper but never pulls its runtime part in, and tree-shaking drops the definition.Fix
In step 6 of
scan_imports_and_exports, where each part's import records are already scanned to count__toESM/__toCommonJS/__require/__reExportuses, also count static imports of async ESM-wrapped modules across the file's parts. When the second such import is seen, generate a__promiseAllruntime symbol use on that part so the helper survives tree-shaking. A single async wrapped import still emits a bareawait init_x()and does not pull the helper in.Trigger / control matrix
Fires when an unwrapped module statically imports two or more modules that are both top-level-await (transitively async) and lazily ESM-wrapped (because they are also reached by
import()somewhere).await init_a()with no__promiseAll; the existing wrapped-importer cases in this file keep passing.Relationship to #33337
#33337 contains the same step-6 counting as part of a larger change that also reworks
InsideWrapperPrefixto preserve import order and removes thecreate_wrapper_for_filecounting. This PR is the minimal__promiseAllfix on its own; if #33337 lands first this one is redundant.How did you verify your code works?
New tests in
test/bundler/bundler_promiseall_deadcode.test.tscover the three trigger shapes and the single-dependency control, and each runs the bundled output as well as asserting on its contents. The three existing__promiseAlltests in that file andbundler_edgecase.test.ts/bundler_splitting.test.ts/bundler_regressions.test.tscontinue to pass.