Repository navigation
Conversation
…IRE_ASYNC_MODULE functionEsmLoadSync loaded, linked and evaluated the graph in one loadModuleSync() call and only inferred top-level await from a promise that was still pending afterwards. Module bodies therefore ran before the throw, graphs whose awaits settled on microtasks alone were returned, a graph that import() had already evaluated was returned, and the error was a bare TypeError without a code. Load the graph first, walk the loaded records' [[HasTLA]] flags, and only then link and evaluate. A graph with top-level await anywhere in it now throws ERR_REQUIRE_ASYNC_MODULE before any of it runs, also when it is already evaluated, as in Node. The registry entry is kept, so a later import() evaluates the module once and a retried require() fails the same way.
|
Status Reproduced on main // tla.mjs
console.log("side effect"); await 0; export const x = 1;
// main.cjs
try { require("./tla.mjs") } catch (e) { console.log(e.code ?? e.name) }Node prints The program with the import cycle in the PR notes ( The branch contains main again (head Not mergeable. Two things need a maintainer's decision.
The choice: adopt Node's rule (then the harness file and the engine change too), or keep the current behaviour and close this PR. |
|
No actionable comments were generated in the recent review. ℹ️ Recent review info
WalkthroughSynchronous CommonJS-to-ESM loading now passes parent filenames, drains dependency graphs, and rejects graphs with transitive top-level await using ChangesSynchronous ESM loading
Suggested reviewers: Priority: Merge Risk:
 |
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check |  |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Title check | |
The title "require(esm): reject top-level await before evaluating, with ERR_REQUIRE_ASYNC_MODULE" states the main change: require() of an ES module graph with top-level await is now rejected before an… |
| Description check | |
The description is complete and directly addresses the PR objective. It explains the problem, implementation, behavioral changes, limitations, compatibility differences, and verification results. It d… |
Full details: Title check
-
Explanation
The title "require(esm): reject top-level await before evaluating, with ERR_REQUIRE_ASYNC_MODULE" states the main change: require() of an ES module graph with top-level await is now rejected before any module evaluates, using the ERR_REQUIRE_ASYNC_MODULE code. The changeset matches this: ZigGlobalObject.cpp checks HasTLA before link and evaluate, ErrorCode.ts adds the code mapping, and the tests cover the new behavior. The title is 85 characters, slightly long, but it is specific and accurate.
Full details: Description check
-
Explanation
The description is complete and directly addresses the PR objective. It explains the problem, implementation, behavioral changes, limitations, compatibility differences, and verification results. It does not use the exact template headings, but it provides the required information under equivalent sections.
- Autofix · 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.
Comment @coderabbitai help to get the list of available commands.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it restructures functionEsmLoadSync into a two-phase load/evaluate pipeline and changes registry-entry retention semantics on the TLA throw path, a human familiar with the JSC module loader should confirm the approach.
What was reviewed:
- Exception-scope discipline in the new C++ paths —
RETURN_IF_EXCEPTIONaftertoWTFString/value(globalObject), queue restoration ordered before the exception check, and throws routed throughBun::throwError/throwVMTypeErrorwith the scope. findTopLevelAwaitInGraph— only entered whenisModuleGraphLoadedproved a non-null cyclic record; dependencies null-checked before enqueue; WTF containers used per convention.- Node compat —
ERR_REQUIRE_ASYNC_MODULEadded via ErrorCode.ts asError(notTypeError), message stem matches Node verbatim; the extra "top-level await is in" line replaces Node's--experimental-print-required-tlahint as the description states. - Tests —
test.concurrentsubprocess spawns with concurrent pipe drain,tempDir/bunEnv/bunExe, stderr asserted before exitCode, and the updated 24387 snapshot/existing transitive-TLA assertion reflect the intentional error-shape change rather than a weakening.
Extended reasoning...
Overview
This PR reworks require() of ES modules with top-level await to match Node.js semantics. The core change in src/jsc/bindings/ZigGlobalObject.cpp splits functionEsmLoadSync from a single load+link+evaluate step into two phases: first load the graph under a SynchronousModuleQueue without evaluating, walk [[LoadedModules]] via a new findTopLevelAwaitInGraph DFS checking hasTLA(), throw ERR_REQUIRE_ASYNC_MODULE if any record has TLA, and only then link+evaluate. The same walk runs on already-loaded registry entries so require() after a successful import() is rejected consistently. A parentFilename argument is threaded through $requireESM → $loadEsmIntoCjs → $esmLoadSync (builtins.d.ts, CommonJS.ts, arity bump) purely for the error's "From" line. ERR_REQUIRE_ASYNC_MODULE is added to ErrorCode.ts. Tests in require-esm-transitive-tla.test.ts are substantially expanded and 24387.test.ts is updated for the new error shape.
Security risks
None identified. This is module-loading control flow with no auth, crypto, or untrusted-data parsing. The new graph walk operates on JSC-owned module records held by the loader registry; no user-controlled sizes drive allocation, and the DFS is bounded by the visited set.
Level of scrutiny
High. functionEsmLoadSync is on the critical path for every require() that resolves to an ES module, and the change alters when registry entries are retained versus removed on the throw path (kept on ERR_REQUIRE_ASYNC_MODULE so a later import() reuses the record; removed only for genuinely-async fetches this call created). The removal of !cyclic->hasTLA() from the post-evaluate Pending fallback is sound because TLA is now caught before evaluation, but the interaction between the two SynchronousModuleQueue scopes (the new load-only one and the existing loadModuleSync one), async plugin onLoad, and concurrent outer import() is subtle enough that a maintainer who owns this loader glue should sign off.
Other factors
Test coverage is strong: multiple await shapes, transitive TLA, retry consistency, evaluation-order proof that no TLA body runs before rejection, subsequent import() evaluating once, rejection after import() already evaluated, and exact multi-line message. Tests follow harness conventions (test.concurrent, Promise.all pipe drain, tempDir, stderr-before-exitCode). The two existing-test edits (24387 snapshot, transitive-TLA assertion) are legitimate updates to the intentionally changed error contract, not weakenings. The PR description is unusually detailed with a Node-vs-before-vs-after behavior matrix and named out-of-scope items. No prior reviewer objections in the timeline.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
… is still loading Since the WebKit bump in #43022, require() of a module whose fetch an import already started loads it synchronously, so this shape reaches the same check.
…nd through a wrapped Module._extensions handler The entry's own imports decide where the cycle is entered. A require() that starts to evaluate the graph enters it from the required module, and a module of the cycle then reads an export before it exists.
…o end Main's tests pin that require() hands over an ES module graph once an import() has run its top-level awaits to their end (module-graph.test.ts, "require(esm) by module state"), and test/aot/generate.ts relies on it. Keep that: the check before evaluation now looks for a module with top-level await that has not started or is still waiting, the rule of ReadyForSyncExecution. Also follow the edges of a prelinked graph, which are not in [[LoadedModules]], and read the requiring file's name from the module object that main now passes to $esmLoadSync. A module object without a parent is handled. Three assertions in main's tests restated the old TypeError text, and one listed the refused module in require.cache, where it no longer appears because it never runs.
…ire(), and cannot be required itself
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
src/jsc/bindings/ZigGlobalObject.cpp— Users of abun build --compile --aotexecutable still get the oldTypeError: require() async module "..." is unsupportedwith nocodewhen they require() a compiled ES module with top-level await, while plain Bun now throwsERR_REQUIRE_ASYNC_MODULE. The compiled-module path at ZigGlobalObject.cpp:1150 returns before the new check, and the AOT host table still hands JSCcreateAsyncModuleRequiredError(BunCompiledModules.cpp:620). Fix: make every synchronous require-of-TLA site throw the sameERR_REQUIRE_ASYNC_MODULEerror, including the AOT host entry, and update test/aot/programs/modules/a-require-comes-into-a-cycle-of-modules-that-awaits-while-an-import-is-loading-it.expected.txt, which pins the old message.Why this was flagged
A user builds an executable with
bun build --compile --aotwhose CommonJS code calls require() on an embedded ES module that has top-level await. functionEsmLoadSync at src/jsc/bindings/ZigGlobalObject.cpp:1149-1153 calls Bun::loadCompiledModuleNamespace and returns before the new findPendingTopLevelAwait/throwRequireAsyncModule path at :1185-1186 runs. The AOT host table at src/jsc/bindings/BunCompiledModules.cpp:620 still supplies &createAsyncModuleRequiredError, which builds the old TypeError at src/jsc/bindings/ModuleLoader.cpp:56-58 with nocodeproperty. The unchanged test/aot/programs/modules/a-require-comes-into-a-cycle-of-modules-that-awaits-while-an-import-is-loading-it.expected.txt confirms --aot output isTypeError require() async module slow-b.mjs" is unsupported. use "await import()" instead.After merging, code written against theERR_REQUIRE_ASYNC_MODULEcontract works under plain Bun and--compilebut misses the error inside an--aotexecutable. On the base branch both paths agreed on the TypeError, so there was no inconsistency.Verification: src/jsc/bindings/ZigGlobalObject.cpp:1149-1153 returns before the
findPendingTopLevelAwait/throwRequireAsyncModulecall at :1185-1186. The AOT host table still supplies&createAsyncModuleRequiredErrorat BunCompiledModules.cpp:620, which builds a TypeError with nocode. The pinned test/aot/programs/modules/a-require-comes-into-a-cycle-of-modules-that-awaits-while-an-import-is-loading-it.expected.txt records the old message.
|
The
If the engine gets the same check, the host error and that file change with it. If the current behaviour stays, this PR closes. That decision is a maintainer's. |
Problem
require()of an ES module graph with top-level await runs the graph up to the first await, then throwsTypeError: require() async module "<path>" is unsupported. use "await import()" instead.Node throwsERR_REQUIRE_ASYNC_MODULEand runs nothing.functionEsmLoadSync(src/jsc/bindings/ZigGlobalObject.cpp) evaluates while it loads (loadModuleSync()). Only a pending promise afterwards shows the await.Fix
loadModulewithoutModuleLoadFlag::Evaluate). Throw iffindPendingTopLevelAwaitfinds a module whose await has not ended. Otherwise link and evaluate.Errorwith codeERR_REQUIRE_ASYNC_MODULE. The message names the requiring file and the awaiting module.require-esm-transitive-tla.test.ts(6 fail on main),24387.test.ts. See notes.Background
functionEsmLoadSyncruns JSC's loader under aVM::SynchronousModuleQueue: its promise reactions run inline.[[HasTLA]]is set per record at parse time. The check followsReadyForSyncExecution(import defer proposal), but a module in mid-evaluation stays a cycle.v8::Module::IsGraphAsync) also refuses a finished graph. Not taken:module-graph.test.tspins the opposite.Downsides
--aotprogram keeps the old behaviour (itsrequire()is in WebKit), andtest/aotcompares both.require()returned a module whose awaits did not run or waited for settled promises, and now throws.test/aot/generate.tsrequires a file that awaits underif (import.meta.main): 29test/aotfiles fail in CI.require()of an ES module runs the load chain twice (9 more cells, counted).Notes
A program that ends with exit 1 on main and runs on Node and on this branch.
c.cjsrequiresm2.mjs, whose graph holds a top-level await. Main starts the graph fromm2.mjs, so the cyclem2 → m3 → m4 → m1 → m2is entered atm2andm1.mjsreadsv2before it exists. The error stays on the records, and the entry's ownimport "./m3.mjs"then fails.Node 26.3 and this branch:
c caught an error,m1 reads v2,m0 ends. Main at 11c3671:c caught an error, then exit 1. The test "and leaves an import cycle to be entered where the importing graph enters it" holds this program.Behaviour against Node 26.3 (
x= a module body ran before the throw):require(tla.mjs),await 0await Promise.resolve()import()started it)await import(tla)has ended, thenrequire(tla)require(tla), thenimport(tla)require(esm)whose body throws, 3 times, thenimport()The merge with main on 2026-10-28 changed three things in this PR:
$esmLoadSync(Bun.ModuleGraph: a context per graph for timers and I/O, per-graph CommonJS #42590). The error reads the file name from that object, so the PR no longer touchesCommonJS.tsorbuiltins.d.ts. AModule._extensionshandler passes the module that is being loaded. Its parent is then the requiring file, and a module object without a parent gets noFromline.prelinkedRequestedModule), which are not in[[LoadedModules]].import()has finished. Main added tests for that behaviour ("require(esm) by module state" intest/js/bun/module-graph/module-graph.test.ts), andtest/aot/generate.tsrequires a module whose graph holds the finished module with the await.Expectations of main's tests that this PR changes, all for a module whose await has not run:
module-graph.test.ts: one regular expression and onemessage.includes("await import")read the old message. One list expected the refusedtla.mjsinrequire.cache. It ran after the throw on main and does not run now.require-extensions.test.ts: the row fortop-level-await.jsexpected the old TypeError.The break that CI shows (build 134500).
test/aot/generate.tsloads its generators withrequire(). One of them,test/aot/generate-built-ins-alone.ts, is also a script: its only top-levelawaitis insideif (import.meta.main) { ... }. When the file is required, that branch does not run, so main evaluates the module synchronously and returns it.[[HasTLA]]is a property of the source text, so this branch refuses the file, as Node does. 29 test files oftest/aot(built-ins-alone-*,every-program-*,every-program-alone-*,calls-beyond-their-reach-*) fail when they load, all with this one error. I did not change the harness: whetherrequire()may refuse such a file is the decision this PR waits for.The same holds for a module whose awaits all wait for settled promises (
export const x = await Promise.resolve(1)): main returns it, this branch and Node refuse it.The
--aotdifference in detail.TEST_PART=aot/modules AOT_PROGRAMS=require-of-a-module-with-top-level-await bun bd test test/aot/parts.test.tsruns 2 programs and both fail on this branch. Underbun runeveryrequire()of a module with a pending await now throwsError. The compiled program still hands over a module whose awaits are settled and throwsTypeErrorfor the others. About 30 more programs oftest/aot/programs-in-every-run.txtrequire a module with top-level await (by name:require-of-a-module-with-top-level-await*,a-module-resumed-inside-require-*,require-settles-the-promise-*,each-way-to-require-*). I did not run those.BunCompiledModules.cppcallsAOT::requireModule, and compiled code calls the engine directly, so the host has no place for the check.docs/compiler/compatibility.mdx(the row onrequire()of modules where one awaits, and the sentence "require()can't wait") describes the oldbun runbehaviour and is not changed here. It depends on what the engine does.Differences from Node that remain:
if (false) await 0is accepted, because the transpiler drops the dead branch before the record exists.ERR_REQUIRE_ASYNC_MODULE. Node links first and reports theSyntaxError.After a refusal the graph stays loaded and not evaluated.
delete require.cache[file]does not drop it, because main does not evict a module that has not evaluated. On main the module finished later and could then be dropped and read again. Node keeps it too.The cost for a graph without top-level await: the link-and-evaluate step is main's
loadModuleSync(), which starts the top-level load chain again over the loaded graph. That is 6 promises, 2ModuleLoadingContextand 1ModuleGraphLoadingStateper firstrequire()of an ES module (JSModuleLoader::loadModuleandmoduleLoadTopSettledat WebKit fcc1ea0973a4), plus one walk of the graph. A laterrequire()of the same module returns before any of this.JSModuleLoader::linkAndEvaluateModulein place ofloadModuleSync()removes the second chain. It also replaces main's error path, so I left it out of this diff.Loads that cannot finish synchronously for another reason (a plugin with an asynchronous
onLoad) keep the old TypeError.Instead of Node's hint at
--experimental-print-required-tla(a flag Bun does not have) the message names the module with the await.Supersedes require(esm): throw ERR_REQUIRE_ASYNC_MODULE instead of a bare TypeError #33438 and require(esm): reject any top-level-await graph with ERR_REQUIRE_ASYNC_MODULE #33891. node: require(esm) v26 compat — TLA + cycle error contract (+72 tests) #35693 (draft, on another base) holds a similar two-step load inside a larger change.
Suites run on Linux x64 (ASAN debug) at 51adf36:
require-esm-transitive-tla,24387,require-extensions,module-graph,require-esm-evaluating-cycle,require-esm-microtask-order,require-esm-gc-roots,require.test.ts,import-meta,esModule,builtin-esm-lazy-exports,dynamic-import-tla-cycle,isolation-one-module-fetched-twice,require-extensions-override,esm-registry-concurrent-gc,mock-module,plugins,node-module-module,runtime-transpiler,module-graph-compile,module-graph-gc, andcompile/splitting/SplitRequireLoadsChunkSynchronously. These time out at the default 5 s on my ASAN debug build with and without the change:12910.test.ts, the fourdynamicImporttests ofmodule-graph.test.ts(and one or two more of that file from run to run), and the fiveFoldingNeverMakesABundleWorsegraphs (all five returnokwhencheckGraphruns outside the test). With the per-test timeout that CI gives an ASAN build (bun bd test --timeout=270000), all 322 tests of the four changed test files pass on that build. They also pass on a release build with the default timeout. Windows was not run after the merge.Self-reviewed after the merge: 7 concerns raised. 4 led to changes (a module object without a parent, prelinked edges, the finished graph, the CommonJS wording in the docs). 3 are listed above (
--aot,require.cache, the second load chain).[human-review] gate passed · iteration 4 · 8 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 4
evidence per changed file