fix(build): Promise.all() async module dependencies - #22704
Conversation
|
Updated 6:34 PM PT - Sep 26th, 2025
❌ @autofix-ci[bot], your commit cc796c6 has 4 failures in
🧪 To try this PR locally: bunx bun-pr 22704That installs a local version of the PR into your bun-22704 --bun |
This reverts commit f211c1e.
WalkthroughAdds Promise.all runtime wiring and tracking for top-level await; refactors StmtList into segmented lists with unified append/appendNonDependency APIs; converts many bundler functions to explicit bun.OOM or error-union returns and adds LinkerContext.LinkError; updates error propagation, allocation handling, and no-side-effect defines. Changes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (7)test/**📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Files:
test/bundler/**/*📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Files:
test/**/*.{js,ts}📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Files:
test/**/*.test.ts📄 CodeRabbit inference engine (test/CLAUDE.md)
Files:
test/**/*.test.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{js,ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
test/regression/issue/**📄 CodeRabbit inference engine (test/CLAUDE.md)
Files:
🧠 Learnings (25)📚 Learning: 2025-08-30T00:09:39.100ZApplied to files:
📚 Learning: 2025-08-30T00:09:39.100ZApplied to files:
📚 Learning: 2025-08-30T00:09:39.100ZApplied to files:
📚 Learning: 2025-08-30T00:12:56.803ZApplied to files:
📚 Learning: 2025-08-30T00:12:56.803ZApplied to files:
📚 Learning: 2025-08-30T00:09:39.100ZApplied to files:
📚 Learning: 2025-08-30T00:12:56.803ZApplied to files:
📚 Learning: 2025-09-07T05:41:52.563ZApplied to files:
📚 Learning: 2025-09-03T17:10:13.486ZApplied to files:
📚 Learning: 2025-08-30T00:12:56.803ZApplied to files:
📚 Learning: 2025-09-20T00:58:38.042ZApplied to files:
📚 Learning: 2025-08-30T00:12:56.803ZApplied to files:
📚 Learning: 2025-08-30T00:09:39.100ZApplied to files:
📚 Learning: 2025-09-20T00:57:56.685ZApplied to files:
📚 Learning: 2025-09-03T17:10:13.486ZApplied to files:
📚 Learning: 2025-09-03T17:10:13.486ZApplied to files:
📚 Learning: 2025-09-03T17:10:13.486ZApplied to files:
📚 Learning: 2025-09-03T17:10:13.486ZApplied to files:
📚 Learning: 2025-09-07T05:41:52.563ZApplied to files:
📚 Learning: 2025-09-07T05:41:52.563ZApplied to files:
📚 Learning: 2025-09-08T04:44:59.101ZApplied to files:
📚 Learning: 2025-08-30T00:09:39.100ZApplied to files:
📚 Learning: 2025-09-08T00:41:12.052ZApplied to files:
📚 Learning: 2025-09-08T04:44:59.101ZApplied to files:
📚 Learning: 2025-09-08T04:44:59.101ZApplied to files:
🧬 Code graph analysis (1)test/bundler/bundler_promiseall_deadcode.test.ts (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
src/bundler/bundle_v2.zig (1)
148-150: Reset this flag at the start of each build cycle.As written, once true it never resets, which can disable TLA fast-paths on later builds (e.g., DevServer). Reset when enqueuing entry points.
Example update (outside this hunk) at the top of enqueueEntryPoints():
// at start of enqueueEntryPoints(...) this.has_any_top_level_await_modules = false;src/defines-table.zig (1)
184-187: Consider covering more Promise statics.Optional: add Promise.any, Promise.allSettled, and Promise.race to the no-side-effect property access list for parity.
+ &[_]string{ "Promise", "any" }, + &[_]string{ "Promise", "allSettled" }, + &[_]string{ "Promise", "race" },src/bundler/linker_context/computeCrossChunkDependencies.zig (1)
1-1: Propagate OOM consistently in this module.Follow-up: make computeCrossChunkDependenciesWithChunkMetas also return bun.OOM!void for consistency, and prefer propagating over
catch unreachablewhere feasible.Additional change (outside this hunk):
// change signature pub fn computeCrossChunkDependenciesWithChunkMetas(c: *LinkerContext, chunks: []Chunk, chunk_metas: []ChunkMeta) bun.OOM!void { ... }src/bundler/linker_context/generateCodeForFileInChunkJS.zig (2)
43-48: Capacity calc LGTM; minor consistency nit on OOM handling.The pre-sizing for all_stmts using inside/outside lists plus one function wrapper looks correct. For consistency with surrounding code that funnels OOM through bun.handleOom, consider using it here instead of catch unreachable.
Apply:
- stmts.all_stmts.ensureUnusedCapacity(stmts.allocator, all_stmts_len) catch unreachable; + bun.handleOom(stmts.all_stmts.ensureUnusedCapacity(stmts.allocator, all_stmts_len));
294-299: Minor: unify OOM handling here too.Elsewhere you use bun.handleOom; consider doing the same for ensureUnusedCapacity to standardize error paths.
Apply:
- stmts.all_stmts.ensureUnusedCapacity(stmts.allocator, stmts.inside_wrapper_prefix.stmts.items.len + stmts.inside_wrapper_suffix.items.len) catch unreachable; + bun.handleOom(stmts.all_stmts.ensureUnusedCapacity(stmts.allocator, stmts.inside_wrapper_prefix.stmts.items.len + stmts.inside_wrapper_suffix.items.len));src/bundler/LinkerContext.zig (1)
1083-1117: In-place mutation of call.args assumes BabyList.at returns a pointer.call.args.at(0).data.e_array.items.append(...) relies on at(0) yielding a mutable reference. If at() returns by value, this will be a no-op. Confirm BabyList.at returns *Expr; otherwise, replace with pointer indexing to mutate in place.
If needed:
- try call.args.at(0).data.e_array.items.append(this.allocator, call_expr); + const first_arg_ptr = call.args.ptrAt(0); // or &call.args.items[0] + try first_arg_ptr.data.e_array.items.append(this.allocator, call_expr);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
src/bundler/LinkerContext.zig(14 hunks)src/bundler/LinkerGraph.zig(3 hunks)src/bundler/bundle_v2.zig(2 hunks)src/bundler/linker_context/computeChunks.zig(1 hunks)src/bundler/linker_context/computeCrossChunkDependencies.zig(1 hunks)src/bundler/linker_context/convertStmtsForChunk.zig(9 hunks)src/bundler/linker_context/convertStmtsForChunkForDevServer.zig(4 hunks)src/bundler/linker_context/generateCodeForFileInChunkJS.zig(10 hunks)src/bundler/linker_context/generateCodeForLazyExport.zig(4 hunks)src/bundler/linker_context/scanImportsAndExports.zig(11 hunks)src/defines-table.zig(1 hunks)src/runtime.js(1 hunks)src/runtime.zig(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/defines-table.zigsrc/bundler/linker_context/scanImportsAndExports.zigsrc/bundler/LinkerGraph.zigsrc/bundler/bundle_v2.zigsrc/runtime.zigsrc/bundler/linker_context/generateCodeForFileInChunkJS.zigsrc/bundler/linker_context/convertStmtsForChunk.zigsrc/bundler/linker_context/generateCodeForLazyExport.zigsrc/bundler/linker_context/computeChunks.zigsrc/bundler/linker_context/convertStmtsForChunkForDevServer.zigsrc/bundler/linker_context/computeCrossChunkDependencies.zigsrc/bundler/LinkerContext.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup
Files:
src/defines-table.zigsrc/bundler/linker_context/scanImportsAndExports.zigsrc/bundler/LinkerGraph.zigsrc/bundler/bundle_v2.zigsrc/runtime.zigsrc/bundler/linker_context/generateCodeForFileInChunkJS.zigsrc/bundler/linker_context/convertStmtsForChunk.zigsrc/bundler/linker_context/generateCodeForLazyExport.zigsrc/bundler/linker_context/computeChunks.zigsrc/bundler/linker_context/convertStmtsForChunkForDevServer.zigsrc/bundler/linker_context/computeCrossChunkDependencies.zigsrc/bundler/LinkerContext.zig
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format JavaScript/TypeScript files with Prettier (bun run prettier)
Files:
src/runtime.js
🧠 Learnings (14)
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/{**/js_*.zig,bun.js/api/**/*.zig} : Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Applied to files:
src/bundler/linker_context/scanImportsAndExports.zigsrc/bundler/LinkerGraph.zigsrc/bundler/linker_context/generateCodeForLazyExport.zigsrc/bundler/LinkerContext.zig
📚 Learning: 2025-09-06T03:37:41.154Z
Learnt from: taylordotfish
PR: oven-sh/bun#22229
File: src/bundler/LinkerGraph.zig:0-0
Timestamp: 2025-09-06T03:37:41.154Z
Learning: In Bun's codebase, when checking import record source indices in src/bundler/LinkerGraph.zig, prefer using `if (import_index >= self.import_records.len)` bounds checking over `isValid()` checks, as the bounds check is more robust and `isValid()` is a strict subset of this condition.
Applied to files:
src/bundler/linker_context/scanImportsAndExports.zigsrc/bundler/LinkerGraph.zigsrc/bundler/linker_context/generateCodeForLazyExport.zigsrc/bundler/LinkerContext.zig
📚 Learning: 2025-09-08T00:41:12.052Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-09-08T00:41:12.052Z
Learning: Applies to src/bun.js/bindings/v8/src/symbols.dyn : Add new V8 API mangled symbols (with leading underscore and semicolons) to src/symbols.dyn
Applied to files:
src/bundler/linker_context/scanImportsAndExports.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS<ClassName> and re-export toJS/fromJS/fromJSDirect
Applied to files:
src/bundler/linker_context/scanImportsAndExports.zigsrc/bundler/linker_context/generateCodeForLazyExport.zigsrc/bundler/LinkerContext.zig
📚 Learning: 2025-09-07T05:41:52.563Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-09-07T05:41:52.563Z
Learning: Follow the build pipeline: Source TS/JS → Preprocessor → Bundler → C++ Headers; IDs assigned A–Z; `$` replaced with `__intrinsic__`; `require("x")` replaced with `$requireId(n)`; `export default` converted to `return`; `__intrinsic__` replaced with `@`; inlined into C++; modules loaded by numeric ID
Applied to files:
src/bundler/linker_context/scanImportsAndExports.zig
📚 Learning: 2025-09-07T05:41:52.563Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-09-07T05:41:52.563Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{js,ts} : Author modules as CommonJS-style with `require(...)` and export via `export default {}` (no ESM `import`/named exports)
Applied to files:
src/bundler/linker_context/scanImportsAndExports.zig
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/dev/esm.test.ts : esm.test.ts should cover ESM feature behavior in development mode
Applied to files:
src/bundler/linker_context/scanImportsAndExports.zig
📚 Learning: 2025-09-07T05:41:52.563Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-09-07T05:41:52.563Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{js,ts} : Use string-literal `require("...")` only (no dynamic or non-literal specifiers)
Applied to files:
src/bundler/linker_context/scanImportsAndExports.zigsrc/bundler/LinkerContext.zig
📚 Learning: 2025-09-08T00:41:12.052Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-09-08T00:41:12.052Z
Learning: Applies to src/bun.js/bindings/v8/src/napi/napi.zig : Add new V8 API method mangled symbols to the V8API struct in src/napi/napi.zig for both GCC/Clang and MSVC
Applied to files:
src/bundler/LinkerGraph.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/bindings/generated_classes_list.zig : Update src/bun.js/bindings/generated_classes_list.zig to include new classes
Applied to files:
src/bundler/linker_context/generateCodeForFileInChunkJS.zigsrc/bundler/linker_context/generateCodeForLazyExport.zig
📚 Learning: 2025-09-08T04:44:59.101Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-08T04:44:59.101Z
Learning: Applies to src/js/**/*.{js,ts} : In built-in modules (src/js), use export default (converted to a return statement by the preprocessor)
Applied to files:
src/bundler/linker_context/generateCodeForLazyExport.zig
📚 Learning: 2025-08-30T09:09:18.384Z
Learnt from: Jarred-Sumner
PR: oven-sh/bun#22231
File: src/bundler/bundle_v2.zig:48-48
Timestamp: 2025-08-30T09:09:18.384Z
Learning: In Zig, when a module exports a top-level struct, import("./Module.zig") directly returns that struct type and can be used as a type alias without needing to access a field within the module. This is a common pattern in the Bun codebase.
Applied to files:
src/bundler/LinkerContext.zig
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to **/*.zig : Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Applied to files:
src/bundler/LinkerContext.zig
📚 Learning: 2025-09-08T04:44:59.101Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-08T04:44:59.101Z
Learning: Applies to src/js/**/*.{js,ts} : In built-in modules (src/js), require() must use string literals resolved at compile time
Applied to files:
src/bundler/LinkerContext.zig
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
🔇 Additional comments (34)
src/bundler/bundle_v2.zig (1)
3660-3661: LGTM: accurate TLA detection.Using top_level_await_keyword emptiness to update the aggregate flag is correct and cheap.
src/bundler/linker_context/computeChunks.zig (1)
392-396: Better error reporting for dir path resolution.Clear message and BuildFailed return path look good; allocator use is correct.
src/runtime.js (1)
179-179: Approve bound__promiseAllexport
All references inruntime.zigandLinkerContext.zigare correctly wired.src/runtime.zig (1)
320-338: Import for __promiseAll correctly wired and consumed
Field and all-list entry align with the JS export, andpromise_all_runtime_refis referenced in LinkerContext.src/bundler/linker_context/computeCrossChunkDependencies.zig (1)
1-1: All callers already handle the new OOM error
The sole call site in LinkerContext.zig usestry this.computeCrossChunkDependencies(chunks);, so no further updates are required.src/bundler/linker_context/generateCodeForLazyExport.zig (5)
1-1: LGTM!The function signature correctly returns
bun.OOM!voidto align with the broader shift to OOM-aware public APIs in the LinkerContext. This change ensures proper error propagation for out-of-memory conditions.
47-47: Proper error handling withtry.Good replacement of
catch unreachablewithtryfor BitSet allocation, allowing OOM errors to properly propagate upward.
366-370: LGTM for OOM propagation.The
allocPrintcall properly usestryfor error propagation, and the multi-line formatting improves readability.
373-373: Consistent OOM handling.All allocations now properly use
tryfor error propagation, maintaining consistency with the new OOM-aware API.Also applies to: 448-448, 461-461
336-336: Drop allocator consistency check
Outer scope correctly callsthis.allocator(), and visitor methods consistently use the injectedvisitor.allocator; there is no bareallocatorvariable to unify.Likely an incorrect or invalid review comment.
src/bundler/LinkerGraph.zig (4)
75-93: LGTM - Proper OOM error propagation.Both runtime symbol generation methods correctly return
bun.OOM!voidand properly propagate errors. The changes align with the broader shift to OOM-aware APIs across the linker context.
153-160: LGTM - Correct error propagation.The
generateSymbolImportAndUsefunction signature and implementation correctly handle OOM errors viatrypropagation instead ofcatch unreachable.
169-169: Good OOM handling improvement.Replacing
getOrPut(g.allocator, ref) catch unreachablewithtry uses.getOrPut(g.allocator, ref)properly propagates allocation failures.
283-283: Load() errors propagate correctly
All callers of the updatedload()are already wrapped withtry, ensuring OOM errors flow through.src/bundler/linker_context/scanImportsAndExports.zig (5)
1-3: Good error union design.The
ScanImportsAndExportsErrortype properly combinesbun.OOMwithImportResolutionFailed, providing a clear error contract for the function. The function signature update to return this error union is appropriate.
268-269: Good error handling for lazy export generation.The
generateCodeForLazyExportcall properly usestryto propagate OOM errors, consistent with that function's new signature.
283-283: Proper OOM propagation in export star context.The ArrayList initialization now correctly uses
tryinstead of potentially panicking on allocation failure.
448-448: Consistent OOM handling for allocations.All allocator calls properly use
tryfor error propagation, maintaining consistency throughout the function.Also applies to: 461-461
519-526: Comprehensive OOM propagation for runtime functions.All calls to
generateSymbolImportAndUseandgenerateRuntimeSymbolImportAndUsecorrectly usetryfor error propagation. This is a thorough update that ensures OOM conditions are properly handled throughout the import/export binding process.Also applies to: 634-640, 718-725, 740-747, 768-775, 800-807, 812-819, 828-859
src/bundler/linker_context/convertStmtsForChunk.zig (3)
90-91: Consistent adoption of new StmtList API.The changes correctly migrate from direct field access (e.g.,
stmts.outside_wrapper_prefix.append) to the unified API (stmts.append(.outside_wrapper_prefix, stmt)). This aligns with the centralized statement management in StmtList.Also applies to: 123-124, 202-203, 329-330, 344-345
175-175: Proper use of appendNonDependency for non-dependent statements.The migration to
appendNonDependencyfor statements that don't create dependencies is appropriate. This helps distinguish between dependency-creating statements and pure side-effect statements during the linking phase.Also applies to: 211-211, 261-261
522-522: Complete API migration.The final statement append also correctly uses the new unified API pattern. All statement appends in the file are now consistent.
src/bundler/linker_context/convertStmtsForChunkForDevServer.zig (2)
57-57: Consistent API adoption for dev server.The changes correctly adopt the unified StmtList API pattern, maintaining consistency with the broader codebase changes.
Also applies to: 116-116
81-81: Appropriate use of appendNonDependency.The use of
appendNonDependencyfor variable declarations and side-effect expressions that don't create module dependencies is correct. This maintains the distinction between dependency-creating and non-dependency statements in the dev server context.Also applies to: 123-123, 138-138
src/bundler/linker_context/generateCodeForFileInChunkJS.zig (2)
156-161: Unified append APIs usage LGTM.Switching to appendSlice(.outside_wrapper_prefix, ...) and appendNonDependencySlice(...) matches the new StmtList API and improves consistency.
387-396: Hoist-to-outside wrapper changes LGTM.Emitting hoisted locals/functions/classes and wrapper initializers via stmts.append(.outside_wrapper_prefix, ...) is correct with the new StmtList structure.
Also applies to: 472-478, 502-511, 549-555, 583-598
src/bundler/LinkerContext.zig (8)
343-347: Broadened error surface LGTM.LinkError as OOM || error{BuildFailed, ImportResolutionFailed} and returning LinkError![]Chunk makes failure modes explicit.
Also applies to: 354-355
372-401: TLA validation + async propagation order LGTM.Validating all sources first, then propagate async dependencies matches semantics and avoids late surprises.
Please confirm propagateAsyncDependencies() sets flags.is_async_or_has_async_dependency in meta consistently for transitive importers, as downstream code relies on it when deciding between appendSyncDependency vs appendAsyncDependency.
423-425: Early exit on post-chunk errors LGTM.Returning BuildFailed after computeChunks aligns with the new LinkError contract.
454-480: OOM propagation and try-usage LGTM.generateNamedExportInFile now cleanly bubbles alloc failures and updates overlays with try.
561-622: Signature change to OOM!void LGTM.Callers updated to try; internal allocs already using try/bun.handleOom appropriately.
1191-1219: Import replacement logic LGTM; async dependency wiring is correct.Replacing wrapped ESM imports with init() and routing async ones through appendAsyncDependency(__promiseAll) is sound and matches the new runtime shape.
Please verify that promise_all_runtime_ref is stable during renaming/minification (i.e., referenced via symbol ref, not by name), which it is here.
Also applies to: 1234-1250, 1265-1276
2638-2638: Local alias for OOM LGTM.Helps keep signatures concise and consistent across the file.
22-24: __promiseAll is always exported by the runtime
__promiseAllis declared and included in the runtime’snamed_exports, so the force‐unwrap cannot fail.
| stmts.inside_wrapper_prefix.appendNonDependency(Stmt.alloc(S.Directive, .{ | ||
| .value = "use strict", | ||
| }, Logger.Loc.Empty)) catch unreachable; | ||
| } |
There was a problem hiding this comment.
Directive ordering bug: "use strict" can lose directive semantics.
You append the directive via inside_wrapper_prefix.appendNonDependency, but appendSync/AsyncDependency insert at index 0 by default. This can push "use strict" away from the first slot, degrading it to a string literal (breaking intended strict mode where required). Fix in StmtList.InsideWrapperPrefix: advance sync_dependencies_end when appending non-dependencies before any dependency is seen.
Apply in src/bundler/LinkerContext.zig (InsideWrapperPrefix):
pub fn appendNonDependency(this: *InsideWrapperPrefix, stmt: Stmt) OOM!void {
- try this.stmts.append(this.allocator, stmt);
+ try this.stmts.append(this.allocator, stmt);
+ if (!this.has_async_dependency) {
+ this.sync_dependencies_end += 1;
+ }
}
pub fn appendNonDependencySlice(this: *InsideWrapperPrefix, stmts: []const Stmt) OOM!void {
- try this.stmts.appendSlice(this.allocator, stmts);
+ try this.stmts.appendSlice(this.allocator, stmts);
+ if (!this.has_async_dependency) {
+ this.sync_dependencies_end += stmts.len;
+ }
}This preserves directive-first semantics while still allowing dependencies to be inserted right after the non-dependency prefix. As per coding guidelines
| const InsideWrapperPrefix = struct { | ||
| allocator: std.mem.Allocator, | ||
| stmts: std.ArrayListUnmanaged(Stmt), | ||
|
|
||
| sync_dependencies_end: usize, | ||
|
|
||
| // if true it will exist at `sync_dependencies_end` | ||
| has_async_dependency: bool, | ||
|
|
||
| pub fn init(alloc: std.mem.Allocator) InsideWrapperPrefix { | ||
| return .{ .stmts = .{}, .allocator = alloc, .sync_dependencies_end = 0, .has_async_dependency = false }; | ||
| } | ||
|
|
||
| pub fn deinit(this: *InsideWrapperPrefix) void { | ||
| this.stmts.deinit(this.allocator); | ||
| this.sync_dependencies_end = 0; | ||
| this.has_async_dependency = false; | ||
| } | ||
|
|
||
| pub fn reset(this: *InsideWrapperPrefix) void { | ||
| this.stmts.clearRetainingCapacity(); | ||
| this.sync_dependencies_end = 0; | ||
| this.has_async_dependency = false; | ||
| } | ||
|
|
||
| pub fn appendNonDependency(this: *InsideWrapperPrefix, stmt: Stmt) OOM!void { | ||
| try this.stmts.append(this.allocator, stmt); | ||
| } | ||
|
|
||
| pub fn appendNonDependencySlice(this: *InsideWrapperPrefix, stmts: []const Stmt) OOM!void { | ||
| try this.stmts.appendSlice(this.allocator, stmts); | ||
| } | ||
|
|
||
| pub fn appendSyncDependency(this: *InsideWrapperPrefix, call_expr: Expr) OOM!void { | ||
| try this.stmts.insert(this.allocator, this.sync_dependencies_end, Stmt.alloc(S.SExpr, .{ .value = call_expr }, call_expr.loc)); | ||
| this.sync_dependencies_end += 1; | ||
| } | ||
|
|
||
| pub fn appendAsyncDependency(this: *InsideWrapperPrefix, call_expr: Expr, promise_all_ref: Ref) OOM!void { | ||
| if (!this.has_async_dependency) { | ||
| this.has_async_dependency = true; | ||
| try this.stmts.insert( | ||
| this.allocator, | ||
| this.sync_dependencies_end, | ||
| Stmt.alloc(S.SExpr, .{ .value = Expr.init(E.Await, .{ .value = call_expr }, .Empty) }, .Empty), | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| all_stmts: std.ArrayList(Stmt), | ||
| const first_dep_call_expr = this.stmts.items[this.sync_dependencies_end].data.s_expr.value.data.e_await.value; | ||
| const call = first_dep_call_expr.data.e_call; | ||
|
|
||
| if (call.target.data.e_identifier.ref.eql(promise_all_ref)) { | ||
| // `await __promiseAll` already in place, append to the array argument | ||
| try call.args.at(0).data.e_array.items.append(this.allocator, call_expr); | ||
| } else { | ||
| // convert single `await init_` to `await __promiseAll([init_1(), init_2()])` | ||
|
|
||
| const promise_all = Expr.init(E.Identifier, .{ .ref = promise_all_ref }, .Empty); | ||
|
|
||
| var items: BabyList(Expr) = try .initCapacity(this.allocator, 2); | ||
| items.appendSliceAssumeCapacity(&.{ first_dep_call_expr, call_expr }); | ||
|
|
||
| var args: BabyList(Expr) = try .initCapacity(this.allocator, 1); | ||
| args.appendAssumeCapacity(Expr.init(E.Array, .{ .items = items }, .Empty)); | ||
|
|
||
| const promise_all_call = Expr.init(E.Call, .{ .target = promise_all, .args = args }, .Empty); | ||
|
|
||
| // replace the `await init_` expr with `await __promiseAll` | ||
| this.stmts.items[this.sync_dependencies_end] = Stmt.alloc(S.SExpr, .{ .value = Expr.init(E.Await, .{ .value = promise_all_call }, .Empty) }, .Empty); | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
Fix dependency insertion boundary to preserve prologue order.
As implemented, appendSync/AsyncDependency insert at index 0 unless sync_dependencies_end has been advanced by non-dependencies. Without advancing it when appending non-dependencies (e.g., "use strict"), dependencies can precede the directive. Update appendNonDependency/appendNonDependencySlice to advance sync_dependencies_end before any dependency is present.
Apply:
pub fn appendNonDependency(this: *InsideWrapperPrefix, stmt: Stmt) OOM!void {
try this.stmts.append(this.allocator, stmt);
+ if (!this.has_async_dependency) {
+ this.sync_dependencies_end += 1;
+ }
}
pub fn appendNonDependencySlice(this: *InsideWrapperPrefix, stmts: []const Stmt) OOM!void {
try this.stmts.appendSlice(this.allocator, stmts);
+ if (!this.has_async_dependency) {
+ this.sync_dependencies_end += stmts.len;
+ }
}Additionally, consider guarding union access in appendAsyncDependency with a debug assert since it assumes the target is an identifier:
- if (call.target.data.e_identifier.ref.eql(promise_all_ref)) {
+ bun.debugAssert(call.target.data == .e_identifier);
+ if (call.target.data.e_identifier.ref.eql(promise_all_ref)) {As per coding guidelines
Also applies to: 1155-1169
dylan-conway
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
test/bundler/bundler_promiseall_deadcode.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/bundler/bundler_promiseall_deadcode.test.ts
test/bundler/**/*
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place bundler/transpiler/CSS/bun build tests under test/bundler/
Files:
test/bundler/bundler_promiseall_deadcode.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/bundler/bundler_promiseall_deadcode.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Name test files*.test.tsand usebun:test
Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Never hardcode port numbers in tests; useport: 0to get a random port
When spawning Bun in tests, usebunExe()andbunEnvfromharness
Preferasync/awaitin tests; for a single callback, usePromise.withResolvers()
Do not set explicit test timeouts; rely on Bun’s built-in timeouts
UsetempDir/tempDirWithFilesfromharnessfor temporary files and directories in tests
For large/repetitive strings in tests, preferBuffer.alloc(count, fill).toString()over"A".repeat(count)
Import common test utilities fromharness(e.g.,bunExe,bunEnv,tempDirWithFiles,tmpdirSync, platform checks, GC helpers)
In error tests, assert non-zero exit codes for failing processes and usetoThrowfor synchronous errors
Usedescribeblocks for grouping,describe.eachfor parameterized tests, snapshots withtoMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach); track resources for cleanup inafterEach
Useusing/await usingwith Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Files:
test/bundler/bundler_promiseall_deadcode.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must be placed under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or use custom random port functions
In tests, use normalizeBunSnapshot when asserting snapshots
Never write tests that merely assert absence of "panic" or "uncaught exception" in output
Avoid shell commands (e.g., find, grep) in tests; use Bun.Glob and built-ins instead
Prefer snapshot tests over exact stdout equality assertions
Files:
test/bundler/bundler_promiseall_deadcode.test.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format JavaScript/TypeScript files with Prettier (bun run prettier)
Files:
test/bundler/bundler_promiseall_deadcode.test.ts
🧠 Learnings (10)
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/dev/bundle.test.ts : bundle.test.ts should contain DevServer-specific bundling tests
Applied to files:
test/bundler/bundler_promiseall_deadcode.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/dev/esm.test.ts : esm.test.ts should cover ESM feature behavior in development mode
Applied to files:
test/bundler/bundler_promiseall_deadcode.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/dev/css.test.ts : css.test.ts should contain CSS bundling tests in dev mode
Applied to files:
test/bundler/bundler_promiseall_deadcode.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/js/bun/**/*.{js,ts} : Place Bun API tests under test/js/bun/, separated by category (e.g., test/js/bun/glob/)
Applied to files:
test/bundler/bundler_promiseall_deadcode.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/**/*.{js,ts} : Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Applied to files:
test/bundler/bundler_promiseall_deadcode.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/dev/ecosystem.test.ts : ecosystem.test.ts should focus on concrete library integration bugs rather than whole-package coverage
Applied to files:
test/bundler/bundler_promiseall_deadcode.test.ts
📚 Learning: 2025-09-07T05:41:52.563Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-09-07T05:41:52.563Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{js,ts} : Author modules as CommonJS-style with `require(...)` and export via `export default {}` (no ESM `import`/named exports)
Applied to files:
test/bundler/bundler_promiseall_deadcode.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/bundler/**/* : Place bundler/transpiler/CSS/bun build tests under test/bundler/
Applied to files:
test/bundler/bundler_promiseall_deadcode.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : Name test files `*.test.ts` and use `bun:test`
Applied to files:
test/bundler/bundler_promiseall_deadcode.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/js/third_party/**/*.{js,ts} : Place third-party npm package tests under test/js/third_party/
Applied to files:
test/bundler/bundler_promiseall_deadcode.test.ts
🧬 Code graph analysis (1)
test/bundler/bundler_promiseall_deadcode.test.ts (1)
test/harness.ts (1)
tempDirWithFiles(260-267)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
| ]); | ||
|
|
||
| // Should not have syntax errors | ||
| expect(stderr).not.toContain('await" can only be used inside an "async" function'); |
There was a problem hiding this comment.
Fix the syntax-error sentinel string.
The guard is checking for await" can only be used inside an "async" function, which never matches Bun’s actual diagnostic ('await' can only be used inside an 'async' function). As written, the assertion can’t fail even if that syntax error regresses. Please update all three occurrences to the correct message.
Apply this diff:
@@
- expect(stderr).not.toContain('await" can only be used inside an "async" function');
+ expect(stderr).not.toContain(`'await' can only be used inside an 'async' function`);
@@
- expect(stderr).not.toContain('await" can only be used inside an "async" function');
+ expect(stderr).not.toContain(`'await' can only be used inside an 'async' function`);
@@
- expect(stderr).not.toContain('await" can only be used inside an "async" function');
+ expect(stderr).not.toContain(`'await' can only be used inside an 'async' function`);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(stderr).not.toContain('await" can only be used inside an "async" function'); | |
| expect(stderr).not.toContain(`'await' can only be used inside an 'async' function`); |
🤖 Prompt for AI Agents
In test/bundler/bundler_promiseall_deadcode.test.ts around line 213 (and the two
other occurrences in the same file), the sentinel string is incorrect — it
currently checks for await" can only be used inside an "async" function. Replace
each occurrence with the exact Bun diagnostic: 'await' can only be used inside
an 'async' function (i.e., update the expect(...).not.toContain(...) argument to
"'await' can only be used inside an 'async' function").
3 participants
What does this PR do?
Currently bundling and running projects with cyclic async module dependencies will hang due to module promises never resolving. This PR unblocks these projects by outputting
await Promise.allwith these dependencies.Before (will hang with bun, or error with unsettled top level await with node):
After:
How did you verify your code works?
Manually and tests