Skip to content

bundler: keep source order in a wrapper prefix that has no async dependency - #41601

Open
robobun wants to merge 49 commits into
mainfrom
robobun/46e46b5b/bundler-wrapper-init-order
Open

robobun wants to merge 49 commits into
mainfrom
robobun/46e46b5b/bundler-wrapper-init-order

Conversation

@robobun

@robobun robobun commented Sep 6, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • bun build runs a CommonJS import, an external require() or an export * after a wrapped ES import that the source lists later. reflect-metadata, tsyringe and one import(): tsyringe requires a reflect polyfill.
  • The cause is fix(build): Promise.all() async module dependencies #22704 (bun 1.2.23): append_sync_dependency (src/bundler/LinkerContext.rs) inserts each sync init_x() above the other statements.

Fix

  • The statements keep source order while no dependency in the list is async.
  • With one, the list is main's, byte for byte. A unit test checks every list of up to seven statements.
  • Verified: test/bundler/bundler_edgecase.test.ts, 43 new cases: 25 fail on main, 13 are todo. Self-reviewed: 23 concerns in the last round, 19 addressed (Notes).

Background

  • var init_x = __esm(() => {...}) wraps a module that import() or require() reaches. An importer calls init_x() before its own code.
  • A dependency is async when it has a top-level await or imports a module that has one.
  • Considered source order up to the first async dependency: a lazy route then threw the tsyringe error where main runs (Notes).

Downsides

  • A bundle that ran only because a later ES import ran first now runs as node runs its source: undefined or a throw. Six pinned cases differ from node, or from bun on the sources, where main does not (Notes). One throws TypeError: undefined is not an object (evaluating 'init_page().then') where main exits 0.
  • Not fixed: a list with an async dependency (6 of 10 orders, Notes), and --splitting (bundler(splitting): run an entry's earlier imports before shared code #44495).
  • --minify: +4 bytes where an init_x() now splits two var statements.
Notes

Where the cases come from. No issue on this tracker reports it. Differential runs of bundles against node and bun on the unbundled files found it. Other projects met it and worked around it:

  • edobry/minsky pull 1282 (merged 2026-05-25): a production crash, "Bun 1.2.23's bundler ... reorders import "reflect-metadata" AFTER other init_*() calls". It shows the 1.2.21 bundle beside the 1.2.23 bundle.
  • dennisofficial/atlas pull 1332 (merged 2026-10-20): "the compiled binary ran init_esm5() (tsyringe's runtime check) before require_Reflect()".
  • asocialmedia/social pull 129 (merged 2026-09-13): @simplewebauthn/server reaches tsyringe through @peculiar/x509, with no import order in user code.
  • Hugo-Dz/exe issue 21 (open, Bun 1.3.10).
  • vitejs/vite issue 21866 reports the same reflect-metadata and tsyringe inversion as a bug of another bundler.

It is a regression: bun 1.2.22 prints the right order. In #22704 the first commit (cd4417d01fa) still called sync dependencies in source order ("Non-async imports can be called directly"). The insertion cursor sync_dependencies_end came with 5c2dc264fc, with no test for the new order. The runtime had the same failure (#4677), and test/js/bun/resolve/resolve.test.ts holds its fix. The IsCommonJS row of the table in #44495 is this bug without code splitting.

#44823 (closed for the narrower #44876) listed this order first among its "five fixes that wrapped files need", all of which "fail on main without --splitting", and a review reply there names this PR for it (#44823 (comment)). Its fix was wider: source order in every list, each async dependency started in place, one await last. Of its five fixes, this PR has the call order for a list with no async dependency. #42046 has the __promiseAll helper, and #45949 the async flag on an import cycle.

A real package.

// entry.mjs
import "reflect-metadata";
import { start } from "./app.mjs";
start();
import("./lazy.mjs").then(m => m.run());

// app.mjs
import { container } from "tsyringe";
export const start = () => console.log("app ok", typeof container.resolve);

// lazy.mjs
import { start } from "./app.mjs";
export const run = () => start();

bun build entry.mjs on 1.2.23 to main, with --target=bun, browser and node: error: tsyringe requires a reflect polyfill. Please add 'import "reflect-metadata"' to the top of your entry point. The import() wraps app.mjs, and the entry prints init_app() above require_Reflect(). Node and bun on the sources, the bundle of bun 1.2.22 and the bundle of this branch print app ok function two times. An entry that imports tsyringe itself, below reflect-metadata, fails in the same way with --target=bun and browser.

The rule. Main prints the statements in three groups: every sync init_x(), one await of the async ones, then everything else (require_x(), __reExport, the init_x() of an export *, and in cjs and iife output the require("x") of an external or builtin import). This branch prints them in source order when the list has no async dependency:

var init_x = __esm(() => {
  import_c = __toESM(require_c()); // import c from "./c.cjs"
  init_s();                        // import "./s.js". Main prints this line first.
  // body of x
});

When the list has an import of an async module, or an export * of one, the three groups stay. The new statements in InsideWrapperPrefix only push in source order and, at the first async dependency, move the sync init_x() calls to the front. From there the code is main's.

The linker makes one list for each part range of a file. A wrapped file has one. An unwrapped file starts a new one after each import of an unwrapped module, so the rule applies to each list (edgecase/CommonJSImportBeforeWrappedESMImportAfterUnwrappedImport). A file that keeps a __commonJS wrapper (import and module.exports in one file) has the same list, with no import() in the bundle (edgecase/WrappedESMImportAfterCommonJSImportInCommonJSWrapper).

Why not source order up to the first async dependency. The previous revision of this branch did that: a CommonJS import that the source lists before the first async dependency ran before the await. A wrapper with two async dependencies starts both with Promise.all. The first one stops at its await, and the second one then runs its leading CommonJS import at once:

// route.mjs, loaded by import() after startup
import "./polyfill.mjs"; // import { settings } from "./config.mjs"; import "reflect-metadata";
import "./page.mjs";     // import container from "./di.cjs"; import { settings } from "./config.mjs";

di.cjs requires tsyringe, and config.mjs has a top-level await. Node, bun, main, 1.2.22 and esbuild run this. That revision threw tsyringe requires a reflect polyfill, because tsyringe ran before reflect-metadata. Three cases hold node's output for this family: edgecase/CommonJSImportBeforeAsyncImportRunsAfterEarlierSiblingBody (the route without packages), CommonJSImportBeforeAsyncImportRunsAfterWaitingImportCycleMember and CommonJSImportBeforeAsyncImportInImportCallTargetRunsAfterCallerCode. They have no sync ES import in a list, so they do not reach the new statements. They are guards for a later change of the lists with an async dependency.

The price. Four things run on main and change here. In each, bun 1.2.22 and esbuild print what this branch prints. Six cases pin them, and the first four also run node on the sources.

  1. An import() that runs while a list runs. app imports router.cjs, then store. router.cjs calls import() on a page with a top-level await. An import() of an async module of the bundle prints as init_page().then(...) (src/js_printer/lib.rs, esbuild's form), so the page starts inside the call. On main store ran first, out of source order, so the page read what store sets. Now router.cjs runs first, and the page reads undefined. Node prints ready, because node starts the page after the synchronous code. A sync export * in the place of the CommonJS import is the same. With an import of an ES module in that place, main prints undefined too. Cases: edgecase/ImportCallInCommonJSDependencyRunsBeforeLaterImport, the same after the code of an unwrapped module, and ImportCallInExportStarDependencyRunsBeforeLaterImport.
  2. That start can throw. When the page imports a file that calls import() on the page, the page is running at that call, and the bundle throws TypeError: undefined is not an object (evaluating 'init_page().then'). Main's bundle of the same files exits 0. The TypeError is on main for every import() of a module that is running. This PR makes one more graph reach it. Case: ImportCallInCommonJSDependencyThenImportCallOfRunningModule.
  3. A CommonJS file that requires the file that imports it. page imports legacy.cjs, then theme. legacy.cjs calls require("./page.js") and reads theme through it. bun runs the other imports of page inside that require(). In a bundle the wrapper of page is running, so the require() returns at once. On main theme ran first, so the bundle ran. Here it throws TypeError: undefined is not an object (evaluating 'theme.name'). Node rejects the sources (ERR_REQUIRE_CYCLE_MODULE). bun runs them when no package.json says "type": "module", and throws ReferenceError: Cannot access 'theme' before initialization when one does. Case: CommonJSImportThatRequiresItsImporterRunsBeforeLaterImport. An alternative is to keep main's order in a list whose CommonJS dependency requires its importer. It needs a graph check in the linker and is not built.
  4. JSX. The bundler puts the import of the JSX runtime after the other imports of the file. Unbundled it is the first statement. So in a wrapped JSX file a CommonJS import now runs before the runtime. Case: CommonJSImportRunsBeforeGeneratedJSXImport.

Apart from these, a bundle that ran only because a later ES import ran first now runs as node runs its source. That is the fix from the other side: needs.cjs reads what sets.mjs sets, the source lists needs.cjs first, and only the bundles of 1.2.23 to main print the value. No switch restores that order. To move the import up does, on every version.

What is not fixed. The order in which a file starts two dependencies, for a file with no wrapper and for a wrapped file. Node starts them in statement order in all ten rows, and so does the bundle of bun 1.2.22. edgecase/DependencyStartOrder has one case for each cell: 8 run, 12 are todo.

first statement second statement main this branch
1 CommonJS import sync ES import second starts first fixed
2 external import (require("x") in cjs output) sync ES import second starts first fixed
3 export * of CommonJS sync ES import second starts first fixed
4 export * of a sync ES module sync ES import second starts first fixed
5 CommonJS import async ES import second starts first as main
6 export * of CommonJS async ES import second starts first as main
7 export * of a sync ES module async ES import second starts first as main
8 export * of an async ES module sync ES import second starts first as main
9 export * of an async ES module async ES import second starts first as main
10 async ES import sync ES import second starts first as main

Row 10 is the init_z(); await init_y() order that the review reply on #44823 left to this PR. It is not fixed here. Rows 5 to 10 with real packages: reflect-metadata, tsyringe, then a module with a top-level await still throws (edgecase/CommonJSImportBeforeWrappedESMImportWithLaterAsyncImport, todo). Also as on main:

Rows 5 to 10 need three things that each change bundles that run on main: an import() of a running module that does not throw, an export * of an async module that is awaited, and a decision on how two async siblings start (together, as now, or one after the other). The todo case and the real-package entry have an importer with no wrapper. Source order before the first await only in such a file is not built or measured here.

Boards. Bundles are built by a debug build of main 3396aead750 and of this branch merged with main 3a9ea00437f. The fix is the same code since then.

2000 generated graphs (CommonJS modules, export *, import cycles, top-level await, import()), bundles and sources on node 26.3.0:

main's bundle this branch's bundle
exit code, all 2000 graphs the same as main's in each graph
stdout differs from main's 12 graphs
1001 graphs that node completes: same stdout as node 456 461
the same 1001: of 8773 module lines, the line sees the modules that node shows 5786 5809
the same 1001: a line right on main and wrong here 0

Two limits of this board. About 90% of its wrappers are async, so it draws few of the lists that change. And no file of it calls import() while a list runs, and no CommonJS file requires its importer, so it cannot draw the price above: the "exit code" and "0" rows hold only for such graphs.

The repo's graph generator (test/bundler/splitting-fuzz.ts) without code splitting, 300 seeds, bun as the reference: the bundle agrees with the source on every read and every order in 111 seeds on main and in 200 here. With main's binary as the baseline no seed is worse. That row cannot be anything else: the generator moves one setter whose only reader is the next import.

The graph fuzzer. splitting-fuzz.ts had one check: code splitting with and without chunk folding. checkUnsplit is a second check on the same generator. It adds two shapes from a random stream of their own, so the pinned seeds of the folding check keep their graphs: a CommonJS file before an ES import that reads what it sets, and an export *. It builds without code splitting and compares every read and every order of top-level effects with a run of bun on the sources. With a second binary as a baseline it reports only the facts that the baseline's bundle has and this bundle does not. bun scripts/splitting-fuzz.ts <bun> <graphs> --unsplit [--baseline=<bun>] runs it over many seeds. Each child process has a deadline now, and an entry whose unbundled run fails is listed apart. Four seeds are pinned in bundler_edgecase.test.ts. All four fail on main.

What stays different from node. All of these are the same on main.

  • import() of an async same-bundle module starts the module inside the call. Node starts it after the caller's synchronous code.
  • An export * of an async module is not awaited.
  • A wrapper continues one promise job after each await of a dependency, and later with Promise.all. Node runs the importers that are ready in import order. An importer of two async modules can so continue after a later importer of one.
  • An importer of one member of an async import cycle does not wait for the cycle root.
  • --splitting runs a shared chunk before the entry's body (bundler(splitting): keep import order when an entry point's chunk takes no fold #44028, bundler(splitting): run an entry's earlier imports before shared code #44495).
  • The dev server's module runtime (src/runtime/bake/hmr-module.ts) has its own order. This PR does not change it.

Not in this PR. Earlier revisions of this branch carried more. Each part left because it changed bundles that run on main with no regression to undo, or because another PR has it. The order change that stays also changes such bundles (above). It stays because it is the regression.

Other open PRs. #46578 adds InsideWrapperPrefix::append_directive to keep "use strict" above the init_x() calls. A directive is only printed in cjs and iife output, where no dependency is async, so after this PR it stays first with or without that method. #42105 and #42046 touch the __promiseAll code that this PR leaves as it is. Two lines of the body of #46786 say that this PR rewrites the export * call and orders it. It orders it only in a list with no async dependency. The draft #45529 is stacked on this branch.

Costs. A list with an async dependency prints the same bytes as on main. A list without one prints the same statements in another order. With --minify, main merges adjacent var statements, and an init_x() (or a run of them) that now stands between two of them splits one: +4 bytes. The first var is a CommonJS import or, in cjs and iife output (the default with --bytecode), the require() of an external or builtin import. The second is another such import or the first var, let or const of the file. Measured: 829 to 833 bytes in cjs output with builtin imports, 852 to 856 in iife output, 864 to 868 with one CommonJS import before the first const of the file, 929 to 933 in a __commonJS wrapper, and 0 in an __esm wrapper (919 bytes), where the names are declared outside. At link time the change is one index pushed for each sync dependency, and one rotation of the list at its first async dependency in place of main's insert for each sync dependency. The binary gains 628 bytes of machine code: the .text of the bun_bundler crate goes from 2,101,163 to 2,101,791 bytes (cargo build -p bun_bundler --release, x86_64 Linux, one codegen unit, before link-time optimization).

Self-review. Six rounds. The fifth reviewed a head that kept source order up to the first async dependency, and reproduced the route above. Its disposition was to keep source order only in a list with no async dependency. That is this revision. The sixth kept the source change as it is and asked for the record: the whole price with a pinned case for each part, the ten rows, node as a run reference, a unit test of the list, the third-party reports, and a question to the maintainers (in a comment below). 23 concerns: 19 addressed, 4 in part. Not done:

  • Shapes of the price in the graph generator (an import() while a list runs, a CommonJS file that requires its importer). Its reference is bun with the async transpiler off, which does not run CommonJS in node's order, so a verdict on those shapes needs node as the reference. The pinned cases have it.
  • A board with few top-level awaits. The review measured one and found more graphs fixed. It is not repeated here.
  • A second JSX case (export * before the runtime import) and an iife build of the external-import row.
  • A guard case for source order before the first await in a file with no wrapper. Nothing here builds that.

Mutations. The unit test (inside_wrapper_prefix_tests in LinkerContext.rs, run by bun run rust:test) fails for each of: reset() without in_source_order = true, reset() without the clear of the indices, append_async_dependency without leave_source_order(), and rotate_left for rotate_right. A build without the leave_source_order() call for an export * of an async module fails edgecase/ImportCallInAsyncExportStarDependencyRunsAfterLaterImport and no other case. Main is the mutant for append_sync_dependency: 25 cases fail.

Changes in existing tests.

  • bundler_bytecode_portable: the libraries.js pins (js eb0e8220…, jsc 21177816 bytes, main: 7b8ce5d1…, 21177808 bytes). That bundle has 22 __esm wrappers and 2882 __commonJS wrappers, and an external import prints as require() in it. Four lists change. In each, an init_x() now stands below the require() of a node builtin (url, inspector/promises, fs, path) that the source imports first. Both bundles print 64 libraries, failed: [].
  • test/aot/code-from-disk.test.ts, the case "a file of the program that requires the entry point then catches what a module of the graph throws" (AOT: fixes for modules, classes and Workers, WebKit upgrade, shorter compiler docs #47129): its main.ts imports asks.cjs, then throws.ts, and asks.cjs requires main.ts. It pinned the old order for the build without --aot ("A bundle runs the module that throws in front of the CommonJS file"). The CommonJS file runs first now, so prints starts with two more lines. differs, the output of the --aot build, is unchanged.
  • run() in splitting-fuzz.ts has a deadline of 60 s for each child.

Suites run with a debug build of this branch merged with main 9fe1cd6caa5: bundler_edgecase (239 pass, 24 todo), bundler_splitting, esbuild/default, bundler_promiseall_deadcode, cyclic-imports-async-bundler, bundler_dynamic_import_dce, dynamic-import-tla-cycle, bundler_cjs2esm, bundler_cjs, bundler_regressions, esbuild/splitting, esbuild/dce, esbuild/importstar, bundler_barrel, bundler_minify, bundler_bun, bundler_defer, bundler_compile_splitting (1543 pass and 0 fail together), bundler_bytecode_portable (22 pass), bundler_compile -t Bytecode (25 pass), the changed case of test/aot/code-from-disk.test.ts, and bun run rust:test -p bun_bundler --lib -- inside_wrapper_prefix (2 pass). bundler_bytecode_portable needs a longer --timeout with a debug binary.

The 43 new bundler_edgecase cases on a debug build of main 3396aead750: 19 fail because main is wrong, 6 fail because they pin output that differs from main's, 5 pass on both, 13 are todo. With --todo, each of the 12 todo cells of the table fails at the assertion on the bundle, after node printed the expected order. 12 more tests run node on the sources of a case. They need node 20 or later and are skipped without it.


no test proof · iteration 2 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/bundler/bundler_bytecode_portable.test.ts, test/aot/code-from-disk.test.ts

… await every async one

The statements a wrapper runs before its body (init_x() calls, require_x()
calls, __reExport calls) were grouped by kind: sync init calls first, then
one await of the async ones, then everything else. An export * of a wrapped
module was never awaited. Both reorder module evaluation against the module
graph. They now stay in source order. Once an async dependency is seen,
every later dependency joins that statement as
await __promiseAll([init_a(), init_b(), ns = require_c()]), so each one
starts before the wrapper suspends and the body runs after all settle.

import() of an async module in the same bundle printed init_a().then(...).
Called from inside a's own first synchronous segment, init_a() returns
undefined and the .then throws. It now defers the call to a microtask,
like import() of a sync module already did.

A module that imports an async module through an import cycle could miss
the async flag: the depth-first walk read a dependency's flag while that
dependency was still on the stack. The propagation is now a worklist over
reverse edges, so it reaches a fixpoint.

__promiseAll is marked live from the parts that hold the import statements
that join it, so an unwrapped entry point gets the helper too.
@coderabbitai

coderabbitai Bot commented Sep 6, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review.

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: oven-sh/bun/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Essentials
  • Run ID: 4ac461cc-f18d-444b-a0d4-678c898e5adb
📥 Commits

Reviewing files that changed from the base of the PR and between 990b9e9 and 6700468.

📒 Files selected for processing (8)
  • src/ast/expr.rs
  • src/ast/stmt.rs
  • src/bundler/LinkerContext.rs
  • src/bundler/linker_context/convertStmtsForChunk.rs
  • src/bundler/linker_context/generateCodeForFileInChunkJS.rs
  • test/bundler/bundler_edgecase.test.ts
  • test/bundler/bundler_promiseall_deadcode.test.ts
  • test/regression/issue/cyclic-imports-async-bundler.test.js
💤 Files with no reviewable changes (2)
  • src/ast/expr.rs
  • src/ast/stmt.rs

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review.

Walkthrough

The linker now joins async ESM dependencies without the __promiseAll runtime helper. Async status propagates through static imports, and internal dynamic imports gain a promise-based retry fallback. Bundler tests cover evaluation order, cycles, rejection, and related output.

Changes

Async ESM initialization

Layer / File(s) Summary
Build ESM wrapper awaits
src/ast/runtime.rs, src/runtime.js, src/bundler/LinkerContext.rs, src/bundler/linker_context/generateCodeForFileInChunkJS.rs
The linker removes the __promiseAll runtime helper and its import. The wrapper prefix queues async dependencies and later statements. It emits no await when there is no async dependency, a conditional await for one dependency, or Promise.all for multiple dependencies.
Propagate and register async dependencies
src/bundler/LinkerGraph.rs, src/bundler/linker_context/convertStmtsForChunk.rs, src/js_printer/lib.rs
Async status propagates through reverse static-import edges. Live ESM export-star dependencies use the wrapper dependency path. Async internal dynamic imports use a promise-based retry fallback.
Validate async initialization behavior
test/bundler/bundler_edgecase.test.ts, test/bundler/bundler_promiseall_deadcode.test.ts, test/bundler/bundler_dynamic_import_dce.test.ts, test/bundler/bundler_bytecode_portable.test.ts, test/regression/issue/cyclic-imports-async-bundler.test.js
Tests cover async dependencies, evaluation order, cycles, rejection, and dynamic imports. Snapshots and assertions expect direct Promise.all calls and conditional awaits.

AST mutable accessor removal

Layer / File(s) Summary
Remove mutable AST accessors
src/ast/expr.rs, src/ast/stmt.rs
The mutable call, await, and expression-statement accessors were removed.

Suggested reviewers: jarred-sumner

Priority: High

Merge Risk: Minimal · up to 67004

This change reorders and awaits bundled module dependencies so they run in source order and wait for async work. No unresolved defect has been identified, so the change appears ready to merge, subject to normal CI.

Pre-merge checks: Passed 4
Check name Status Explanation
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Title check Passed The title clearly identifies the primary change: preserving source order in wrapper prefixes without async dependencies.
Description check Passed The description follows the repository template. It has both required sections, "What does this PR do?" and "How did you verify your code works?". The first section explains the problem, the fix, the …
Full details: Description check

Explanation

The description follows the repository template. It has both required sections, "What does this PR do?" and "How did you verify your code works?". The first section explains the problem, the fix, the background, the downsides, and the scope limits. The second section lists the test suites, the new bundler_edgecase cases, the fuzzer checks, and the mutation checks. It also flags two test files that the author could not run locally. The description is very long and dense, with much detail in a collapsed Notes block. A reviewer can still find the required information, so the check passes.

  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Sep 6, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 7:34 PM PT - Oct 29th, 2026

❌ @robobun, your commit c9eff65 has 1 failures in Build #135479 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 41601

That installs a local version of the PR into your bun-41601 executable, so you can run:

bun-41601 --bun

…der change

The bundle of libraries.js now calls each module's dependencies in source
order. The same js and jsc hashes were produced on every CI platform.
Comment thread src/bundler/LinkerContext.rs Resolved
Comment thread src/bundler/LinkerContext.rs Resolved
Comment thread src/bundler/LinkerContext.rs Resolved
Comment thread src/bundler/LinkerContext.rs Resolved
Comment thread src/bundler/LinkerContext.rs Resolved
Comment thread src/bundler/LinkerGraph.rs Resolved
Comment thread src/bundler/LinkerGraph.rs Resolved
Comment thread src/js_printer/lib.rs Resolved
Comment thread src/bundler/LinkerContext.rs Resolved
Comment thread src/bundler/LinkerGraph.rs Resolved
Comment thread src/bundler/LinkerGraph.rs Resolved
Comment thread src/js_printer/lib.rs Resolved
… export star

The re-exporter appends __reExport(exports, exports_r) right after the
init_r() of the same record, so it joins the awaited list even though no
later record exists.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no issues

No high-confidence issues detected in this change.

…argets before deferring

Emit await Promise.all([...]) through an unbound Promise symbol instead of
the __promiseAll runtime helper. The helper needed a per-part liveness count
that mirrored the prefix's join rule, which broke twice; an unbound symbol
is reserved by the renamer and needs no bookkeeping. __promiseAll leaves
runtime.js and the runtime imports table.

import() of an async module calls init_foo() first and only defers when the
call returns undefined, which happens inside foo's own first synchronous
segment. This keeps a top-level await import() whose target imports the
awaiter back evaluating at once, the same self-deadlock skip the runtime
applies, while the re-entrant case resolves after foo settles instead of
throwing on undefined.then.
Comment thread src/bundler/LinkerContext.rs Resolved
Comment thread src/js_printer/lib.rs Resolved
@robobun robobun changed the title bundler: evaluate a wrapped module's dependencies in source order and await every async one bundler: keep source order in a wrapper prefix up to its first async dependency Oct 28, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beyond the inline findings, I also checked whether the new awaited export * in convertStmtsForChunk.rs puts a top-level await into an unwrapped file under --format=iife/cjs: it does, but only where should_remove_import_export_stmt already emits await init_x() for a plain import of the same async module on main, so this is the existing behavior extended to the export * twin rather than a new class.

Extended reasoning...

The output-format concern was traced through LinkerContext.rs append_dependency (unchanged for async static imports, which already produce the await in unwrapped files regardless of format) and the new can_await gate in convertStmtsForChunk.rs, which only excludes __commonJS and sync __esm wrappers. No security-sensitive surface is touched; the two inline findings are the open items.

Findings marked 🟡 are optional suggestions and need no follow-up push.

Comment thread src/bundler/LinkerContext.rs Resolved
…barrel's body

The barrel waits for its `export *` target, and that target calls
`import()` on a module that reads the barrel. The module starts inside
the call and reads `undefined`. main's bundle reads the value, because
its barrel does not wait.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review completed

Nothing new to post: everything this review found is already covered by existing comments on this pull request or didn't merit a separate one.

Still open from earlier reviews (2):

If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.

An awaited export star changed two shapes that run on main: a barrel
behind an awaited import() of its importer did not settle, and an importer
with one async import and one async export star continued after a sibling.
It waits for a decision on import cycles through import().
An `export *` of an async module is not awaited, as on main. From that
statement on, the prefix now has main's layout: the later sync
`init_x()` calls, the `await`, then the `init_x()` of the `export *`
and the other later statements. With that call in its source position,
7 of 2000 generated graphs that complete on main were left unsettled.

Tests: four cases hold main's output around an `export *` of an async
module. Two cases pin a CommonJS import before the first async
dependency: it runs where an ES import runs on main.
…endency

The statements that a wrapper runs before its body keep the order of the
import statements while none of them is for an async dependency. With an
async dependency (an `import` of an async module, or an `export *` of
one) the prefix is what main prints: each sync `init_x()`, the `await`,
then the other statements. No statement moves across an `await`.

The earlier cut kept source order up to the first async dependency. Two
sibling modules that share a top-level-await module then ran a CommonJS
import of the second one before the CommonJS import of the first one: a
lazy route with `reflect-metadata` and `tsyringe` threw where main runs.

The change is now made on main's `InsideWrapperPrefix`. Out of this
branch: the `(init_a() || Promise.resolve().then(init_a))` form of
`import()`, the `Promise.all` join (#42046 fixes `__promiseAll is not
defined`), the top-level await flag (#37770), and one call for two
imports of one async module.

Tests: the cases that depend on those parts are removed. New cases hold
main's output for a CommonJS import before an async import, and for an
`export *` of an async module. The unsplit mode of the graph fuzzer has
a deadline for each child, a list of entries with no verdict, and a
baseline binary.
Comment thread src/bundler/LinkerContext.rs Resolved
@robobun robobun changed the title bundler: keep source order in a wrapper prefix up to its first async dependency bundler: keep source order in a wrapper prefix that has no async dependency Oct 29, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

…irst

The case pinned the old order of the build without `--aot`: the module
that throws ran in front of the CommonJS file that the entry point
imports before it. The CommonJS file runs first now. The two builds
still differ in what `require()` of the running entry point does.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review completed

Nothing new to post: everything this review found is already covered by existing comments on this pull request or didn't merit a separate one.

… start order of two dependencies against node

`InsideWrapperPrefix` gets a unit test module. It appends every list of up
to seven statements and compares the result with the layout: source order
when no statement is for an async dependency, else each sync dependency,
one `await`, then the other statements. It also runs every pair of lists of
up to four statements through one prefix with `reset()` between them.

`bundler_edgecase` gets a table of the order in which a file starts two
dependencies, for a file with no wrapper and for a wrapped file. The four
pairs with no async dependency run. The six pairs with one are todo and
hold node's order. Where node is installed, each case that names node as
its reference also runs node on its sources.

New pinned cases: a sync `export *` that calls `import()`, an `import()` of
a module that is running, a CommonJS file that requires its importer, and a
CommonJS import before the generated JSX runtime import. One case expected
`a end` where node prints it later. That line is gone.
Comment thread src/bundler/LinkerContext.rs Resolved
@robobun

robobun commented Oct 29, 2026

Copy link
Copy Markdown
Collaborator Author

@dylan-conway you pointed at this PR for the call order in #44823 (comment). This head does less than the "Call order" fix of #44823, so two decisions are open.

What this head does. A list of dependency statements keeps source order only when no dependency in it is async. A list with an async dependency prints the same bytes as main, so init_z(); await init_y() is not fixed here. An earlier revision kept source order up to the first async dependency. A lazy route then threw tsyringe requires a reflect polyfill where main runs: two siblings start together, and the second one ran its CommonJS import before the code of the first one that follows its await. edgecase/CommonJSImportBeforeAsyncImportRunsAfterEarlierSiblingBody holds that route.

1. Is the price acceptable? These run on main and change here. In each, bun 1.2.22 and esbuild print what this head prints. Six cases pin them.

  • A CommonJS import, or a sync export *, calls import() on a module with a top-level await. That module starts inside the call, so it now starts before a later ES import. It reads undefined where node and main read the value.
  • When that module imports a file that calls import() on it, the bundle throws TypeError: undefined is not an object (evaluating 'init_page().then') where main exits 0.
  • A CommonJS file requires the file that imports it. The bundle throws a TypeError where main's bundle runs. Node rejects the sources, and bun runs them only when no package.json says "type": "module". An alternative is to keep main's order in a list whose CommonJS dependency requires its importer.
  • In a wrapped JSX file, a CommonJS import runs before the generated import of the JSX runtime.

2. Who owns the lists with an async dependency? 6 of the 10 orders in edgecase/DependencyStartOrder are todo with node's order. They need your layout from #44823 (each dependency started in source order, one await last), an awaited export *, and an import() of a running module that does not throw. Each of the three changes bundles that run on main.

@Jarred-Sumner this PR changes the expected output of one case of #47129 in test/aot/code-from-disk.test.ts: "a file of the program that requires the entry point then catches what a module of the graph throws". The build without --aot now runs asks.cjs before throws.ts, as the source lists them, so prints starts with two more lines. differs is unchanged. Is that change fine?

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Comment on lines +4848 to +4871
itBundled("edgecase/CommonJSImportThatRequiresItsImporterRunsBeforeLaterImport", {
files: {
"/entry.js": /* js */ `
const { render } = await import("./page.js");
console.log("entry got", typeof render);
`,
"/page.js": /* js */ `
import "./legacy.cjs";
import { theme } from "./theme.js";
export function render() {
return theme.name;
}
`,
"/legacy.cjs": /* js */ `
const { render } = require("./page.js");
console.log("legacy renders", render());
`,
"/theme.js": /* js */ `
console.log("theme");
export const theme = { name: "dark" };
`,
},
format: "esm",
run: { error: "TypeError: undefined is not an object (evaluating 'theme.name')" },

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 (optional) Users whose CommonJS file requires the ES module that imports it now get a bundle that exits 1 with TypeError: undefined is not an object (evaluating 'theme.name'), where main's bundle and bun run on the sources print theme, legacy renders dark, entry got function. With the source-order prefix (src/bundler/LinkerContext.rs:5164-5167) require_legacy() runs before init_theme(), so the require("./page.js") inside legacy.cjs calls render() while theme is still uninitialized. The PR lists this as a price; a working bundle becoming a startup crash is the consequence to weigh.

Why this was flagged

Input is the test fixture: entry.js await import("./page.js"), page.js import "./legacy.cjs"; import { theme } from "./theme.js"; export function render() { return theme.name }, legacy.cjs const { render } = require("./page.js"); console.log("legacy renders", render()), theme.js export const theme = { name: "dark" }. The new source-order branch at src/bundler/LinkerContext.rs:5164-5167 prints page's prefix as import_legacy = __toESM(require_legacy()); init_theme();. legacy.cjs runs first, its require("./page.js") prints as (init_page(), __toCommonJS(exports_page)), the re-entrant init_page() returns undefined (src/runtime.js:329-338), render is a hoisted function so it is callable, and theme.name reads the hoisted var theme before init_theme() assigned it, so a TypeError is thrown and the entry's await import() rejects. On the base branch append_sync_dependency inserted init_theme() at index 0, so theme ran before legacy and the bundle exited 0 printing theme, legacy renders dark, entry got function. No safeguard applies: validate_tla only rejects require of async modules, and this cycle is sync.

Verification: Branch: append_sync_dependency now does self.sync_dependency_indices.push(self.stmts.len()); self.stmts.push(stmt) (LinkerContext.rs:5164-5167), so the prefix becomes import_legacy = __toESM(require_legacy()); init_theme();. Inside require_legacy(), the re-entrant init_page() returns immediately because __esm (src/runtime.js:329-338) has already set fn = 0.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is right. It is item 3 of "The price" in the description, this case pins it, and #41601 (comment) asks the maintainers whether it is acceptable.

Who runs these files:

  • Node rejects the sources: ERR_REQUIRE_CYCLE_MODULE.
  • bun on the sources prints the three lines only when no package.json says "type": "module". With it, bun throws ReferenceError: Cannot access 'theme' before initialization.
  • The bundles of bun 1.2.22 and of esbuild throw the same TypeError as this branch.
  • The bundles of 1.2.23 to main run, because they print init_theme() above require_legacy().

The alternative is to keep main's order in a list whose CommonJS dependency is in an import cycle with the importer. The linker has no cycle information for files today, so that needs a new pass. It is not built. This thread stays open until a maintainer decides.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants