Repository navigation
Conversation
…stead of rewriting the caught one HMRModule.require() produced its "Cannot require ..." message by assigning e.message on the AsyncImportError it caught and rethrowing the same object. That object is shared: a module whose body failed on such a require() records it as its failure and rethrows it on every later load, so every require() the error later passed through renamed it to whatever module that call was requiring, and the failing require() inside a required module's body was renamed once per enclosing require() as well. require() is the only place an AsyncImportError leaves the loader, so it now throws a new plain Error with the message once, at the require() call that failed, and enclosing require() calls pass it through unchanged like any other module evaluation error.
|
Updated 5:05 PM PT - Aug 12th, 2026
❌ @robobun, your commit b954799 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 37779That installs a local version of the PR into your bun-37779 --bun |
|
Status: ready to merge from my side (code unchanged since cee958f; b954799 is an empty commit that re-ran CI). Reproduced on main with the new test in CI build 93514 finished with 189 lanes passed; |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Walkthrough
ChangesTop-level-await require errors
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the fix is correct and the test covers every propagation path (direct, static-import chain, nested require, recorded-failure rethrow). The inline note about the now-dead declare global { interface Error { asyncId } } block is a cleanup nit with zero runtime effect and doesn't block.
Extended reasoning...
Overview
This PR changes one code path in HMRModule.require() (src/runtime/bake/hmr-module.ts): instead of mutating e.message on a caught AsyncImportError and rethrowing that same object, it throws a fresh new Error(...) with the same message text. A new test in test/bake/dev/esm.test.ts exercises every path the error can propagate through.
Correctness
The bug is well-characterized: the caught AsyncImportError object is shared — it gets recorded as a module's failure in finishLoadModuleAsync and rethrown on every later load, so each subsequent require() that catches it via instanceof AsyncImportError renames it in place. Throwing a fresh plain Error means the conversion happens exactly once (at the innermost failing require()), and from there it propagates unchanged like any other module-body error — outer require() catches see a plain Error, fail the instanceof check, and rethrow untouched. I verified AsyncImportError is only thrown by loadModuleSync and only escapes the loader via require() (loadModuleAsync awaits async deps rather than throwing), so no other conversion site is needed. The message text and name are unchanged for the direct case, so the existing "cannot require a module with top level await" test is unaffected.
Security risks
None. This is dev-server (bake/HMR) error-message construction; no user input reaches new sinks.
Level of scrutiny
Low-to-medium. Dev-server-only TypeScript, ~6 lines of production change, well-isolated, thoroughly tested with exact-message assertions on a 7-case matrix. The PR description demonstrates the author traced every producer and consumer of AsyncImportError and cross-referenced the equivalent bun build diagnostic location.
Other factors
The one inline finding is a nit: the declare global { interface Error { asyncId?: string } } block near the bottom of the file documented a runtime shape (user-visible errors carrying .asyncId) that this PR removes. It's a type-only ambient declaration with no runtime footprint, and the actual .asyncId read at line 136 typechecks via the AsyncImportError class field regardless. Worth cleaning up per REVIEW.md's dead-code rule, but it doesn't affect correctness and shouldn't block the merge.
Nothing reads asyncId off an Error anymore: the only error that carried it no longer leaves the loader.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
The instance no longer leaves the loader, so nothing observes its name.
There was a problem hiding this comment.
LGTM — the two dead-code cleanups from earlier review rounds (the declare global augmentation and the name override in AsyncImportError) are now applied.
What was reviewed:
- Traced
AsyncImportError's only throw site (loadModuleSync) and only catch site (require()) — the instance can no longer reach user code or a recordedmod.failure, so both deletions are safe. - Checked the nested-require and recorded-failure propagation paths against the new test's expectations; the plain
Errorfails theinstanceof AsyncImportErrorguard in every enclosingrequire(), so the message stays fixed. - Confirmed nothing else in
src/runtime/bake/reads.asyncIdor importsloadModuleSync.
Extended reasoning...
Overview
This PR changes one catch block in src/runtime/bake/hmr-module.ts's HMRModule.require(): instead of mutating e.message on the caught AsyncImportError and rethrowing the same object, it throws a fresh new Error(...) with the same message text. Two pieces of code that only existed to shape the escaping AsyncImportError instance are deleted as now-dead: the Object.defineProperty(this, "name", { value: "Error" }) in its constructor and the declare global { interface Error { asyncId?: string } } type augmentation. A new devTest in test/bake/dev/esm.test.ts covers direct require, static-import-chain require, nested require chains, and the recorded-failure rethrow path (via both require() and import()).
Security risks
None. This is dev-server module-loader code that formats an error message; no auth, crypto, filesystem, or untrusted-input parsing is involved. The message interpolates bundler-resolved module ids, not user-controlled strings.
Level of scrutiny
Low-to-medium. The runtime change is a three-line semantic change (mutate-and-rethrow → throw-new) in development-only HMR code, plus two mechanical deletions of code the PR itself makes dead. I traced the control flow: AsyncImportError is thrown only inside loadModuleSync, which is reached only from HMRModule.require() (directly and via parseEsmDependencies recursion); no other file in src/runtime/bake/ imports loadModuleSync. So the conversion happens exactly once at the innermost failing require(), and from there the plain Error fails every enclosing instanceof AsyncImportError check and propagates unchanged — including through finishLoadModuleAsync's mod.failure = e recording. The existing direct-require test's message is byte-identical.
Other factors
Both prior review comments (from earlier runs of this reviewer) have been addressed in follow-up commits fdb315d and df79945, and the comment-cop nit about the long inline comment was cut down in 7694286. The new test asserts exact messages via toEqual on a JSON object rather than substring matches, follows the file's devTest/minimalFramework conventions, and the PR description confirms it fails under USE_SYSTEM_BUN=1 on the four renamed cases and passes with the fix. No outstanding reviewer comments remain.
There was a problem hiding this comment.
LGTM — the two dead-code cleanups from earlier review rounds are in, and the final catch (e) change lets instanceof narrow correctly.
What was reviewed:
- Traced every path an
AsyncImportErrorcan take: thrown only at hmr-module.ts:338 inloadModuleSync, reachable only viarequire()'s catch at :131, so the conversion to a plainErrorhappens exactly once and recorded failures are neverAsyncImportErrorinstances. - Walked each of the seven test cases through the loader (nested-require chain, recorded-failure rethrow via require/static-import/import) — all match the expected
cannotRequire("tla.ts", "tla.ts")under the new code and diverge under the old. - Confirmed nothing else in
src/runtime/bake/reads.asyncIdor referencesAsyncImportError, so the deletednameoverride anddeclare globalblock leave no dangling users.
Extended reasoning...
Overview
This PR fixes an error-message bug in the bake dev server's HMR module loader. HMRModule.require() previously converted the loader-internal AsyncImportError into the user-facing "Cannot require ... uses top-level await" message by assigning e.message on the caught object and rethrowing it. Because that same object could be recorded as a module's failure and rethrown on later loads, subsequent require() calls that propagated it would rename it in place, so the message ended up naming whichever module was required most recently rather than the call that actually failed. The fix throws a fresh new Error(...) instead of mutating the caught one, and deletes two pieces of code that only mattered when the AsyncImportError instance escaped to user code: the Object.defineProperty(this, "name", { value: "Error" }) override in its constructor, and the declare global { interface Error { asyncId?: string } } type augmentation. A new dev-server test in test/bake/dev/esm.test.ts covers all seven paths through the catch (direct require, require-through-static-import, nested require chain, recorded failure via import() then rethrown three ways).
Security risks
None. This is dev-server-only error-message formatting in the HMR runtime; it does not touch auth, crypto, permissions, or any production code path.
Level of scrutiny
Low-to-medium. The runtime change is three lines in a single catch block, plus two deletions of code this PR makes dead. The behavioral surface is narrow (one error message in one loader path) and the existing "cannot require a module with top level await" test still passes with identical output. I've reviewed this PR across three runs now — the first two flagged the dead declare global block and the dead name override, both of which were removed in fdb315d and df79945 respectively; the comment-cop paragraph-comment nit was addressed in 7694286; and the final commit cee958f is a trivial catch (e: any) → catch (e) cleanup that lets TypeScript's instanceof narrowing type e.asyncId without the global augmentation.
Other factors
I traced the control flow to confirm the PR description's claim that AsyncImportError never escapes the loader after this change: it is constructed only at hmr-module.ts:338 inside loadModuleSync, which is reached only from require() (directly or recursively via parseEsmDependencies), and require()'s catch now discards the instance. loadModuleAsync never throws it. The recorded mod.failure is therefore always a plain Error after this change, so the instanceof AsyncImportError check on a rethrown recorded failure is correctly false and the message is preserved. The new test asserts exact messages for every branch and the PR description confirms it fails on system Bun and passes on the debug build. The bug hunting system found nothing.
There was a problem hiding this comment.
LGTM — both dead-code cleanups from earlier review rounds are in, and the fix itself is a minimal, well-tested change.
Checked that AsyncImportError is thrown only from loadModuleSync and that require() is its only exit from the loader, so the new Error conversion happens exactly once at the innermost failing require() and the recorded-failure / nested-require paths now propagate an immutable message. Verified catch (e) + instanceof narrowing reaches e.asyncId via the class's own field declaration (no need for the removed global augmentation), and that dropping the name override is a no-op since subclasses inherit Error.prototype.name === "Error". The new test exercises direct, static-import-chain, nested-require, and recorded-failure-rethrow paths with exact message assertions.
Extended reasoning...
Overview
Two files touched. In src/runtime/bake/hmr-module.ts, HMRModule.require() now throws a fresh new Error(...) when it catches the loader-internal AsyncImportError, instead of mutating e.message on the caught object and rethrowing it. The catch (e: any) becomes catch (e) (narrowed by instanceof). Two pieces of code that only existed because the AsyncImportError instance used to escape to user code are deleted: the Object.defineProperty(this, "name", { value: "Error" }) in its constructor and the declare global { interface Error { asyncId?: string } } augmentation. In test/bake/dev/esm.test.ts, a new devTest covers seven load paths and asserts exact error messages for each.
Security risks
None. This is dev-server-only TypeScript (the bake HMR runtime, bundled into the dev client/server), and the change affects only the text and identity of an error object thrown for a build-time-detectable condition (require of a TLA module). No auth, crypto, filesystem, or network surface.
Level of scrutiny
Low-to-moderate. The runtime change is effectively one line of semantics (e.message = ...; throw e → throw new Error(...)), and the mechanism is clear: the old code shared a mutable error object across a module's recorded failure and every enclosing require() catch, so later propagations renamed it. Grep confirms AsyncImportError is constructed only at loadModuleSync:338, loadModuleSync is reached only from require() (directly and via the recursive parseEsmDependencies call), and nothing else in src/runtime/bake/ reads .asyncId or references the class — so the conversion in require()'s catch is exhaustive and the two deletions are genuinely dead. The message text is byte-identical to before, and name remains "Error" (inherited from Error.prototype), so the existing "cannot require a module with top level await" test's expectations still hold.
Other factors
All three prior review comments on this PR (my two dead-code notes and the comment-cop's long-comment flag) are resolved in the current diff — the declare global block, the name override, and the multi-line comment are all gone/shortened. The new test is placed alongside the existing TLA-require test in esm.test.ts, uses minimalFramework + dev.fetch().json() like neighboring tests, asserts exact strings via toEqual on the full response object, and the PR states it fails under USE_SYSTEM_BUN=1 on the four renamed cases and passes with the fix. The PR also notes its interaction with #37742/#37759/#37765, none of which conflict. No outstanding reviewer concerns; the bug hunting system found nothing this run.
Pull request was closed
Problem
require()of a module that uses top-level await (itself or through a static import) throwsCannot require "X" because "Y" uses top-level await, but 'require' is a synchronous operation.In two cases theXis the wrong module.outer.tsrequiringinner.ts, which requirestla.ts,require("./outer")saidCannot require "outer.ts", a module that can be required, while the stack pointed at therequire("./tla")ininner.ts.import()of a module has failed this way, an unrelatedrequire()that hits the recorded failure renames it, and a laterimport()of the same module then names a module it never involved.require()produced the message by overwritingmessageon the loader's own error and rethrowing it. That one object is shared with every enclosingrequire()and with the module's recorded failure, so eachrequire()it passed through renamed it.Fix
require()now throws a new plainErrorcarrying the message and leaves the caught loader error alone.require(), so the conversion happens exactly once, at therequire()that failed, and from there it propagates unchanged like any other evaluation error.bun buildattaches its equivalent error at the same place.nameare unchanged, so the direct and static-import cases report exactly what they did before. Two leftovers that only existed because the internal error escaped (a globalError.asyncIdtype augmentation and anameoverride) are deleted.Background
require(),import()and static imports of user modules all go through it, and this error message is produced there.require(). The loader reports this with an internal error naming the offending module, which may be a static dependency of the module that was required rather than that module itself.require(), so one failing call unwinds through severalrequire()frames, and every one of them sees the same error object.[review] gate passed · iteration 2 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 2
evidence per changed file
root cause · written by the author bot
HMRModule.require() handled an AsyncImportError by overwriting the caught object's message and rethrowing that same object, while the loader also stored that object by reference as the module's recorded failure and rethrew it on every later load, so each enclosing require() the shared object passed through renamed it after the module that was most recently required rather than the one that had actually failed. The fix has require() throw a fresh Error carrying the rewritten message instead of mutating the caught one, so a recorded failure keeps the message from the point where it was record…
Original description
What does this PR do?
In the dev server's module loader (
src/runtime/bake/hmr-module.ts),HMRModule.require()turns the loader's internalAsyncImportError(thrown when the required module, or one of its static imports, uses top-level await) into the user-facing message by assigninge.messageon the error it caught and rethrowing that same object.The object is shared. A module whose body fails on such a
require()records the error as itsfailureand rethrows the same object on every later load, so everyrequire()the recorded error later propagates through renames it again, and the message ends up naming whichever module wasrequire()d most recently:The in-place rewrite also renames the error once per enclosing
require()when the failingrequire()is inside a required module's body. Withouter.tsrequiringinner.ts, which requirestla.ts,require("./outer")reportedCannot require "outer.ts" because "tla.ts" ..., naming a module that can be required, while the error's stack pointed at therequire("./tla")ininner.ts.require()now throws a new, plainErrorwith the message instead of mutating the caught one.AsyncImportErroris only thrown byloadModuleSync, andrequire()is the only way out of the loader for it (import()and static imports go throughloadModuleAsync, which never throws it), so the conversion happens exactly once, at therequire()call that failed. From there the error propagates through enclosingrequire()calls, and through a module's recorded failure, unchanged, the same way every other error thrown while evaluating a module does. It is also wherebun buildreports the equivalent build error (the diagnostic inLinkerContext.rsis attached to therequire()call whose import graph contains the top-level await), and this runtime is documented as mirroringbun build. The message text and the error'sname("Error"in both cases:AsyncImportErrorinherited it) are unchanged, so the direct and static-import cases, including the existing "cannot require a module with top level await" test, report exactly what they did before; only the two cases above change. As a side effect the browser console now showsError: Cannot require ...with a stack starting at the user'srequire()call instead of the minified class name, the loader's internal frames and anasyncIdproperty.Also deleted, since the
AsyncImportErrorinstance no longer leaves the loader and these two lines were only there on account of it escaping: thedeclare global { interface Error { asyncId?: string } }augmentation at the bottom of the file (the onlyasyncIdread is on theAsyncImportErrorinstance itself, which declares the field; nothing needed the augmentation for type-checking), and the own-propertynameoverride inAsyncImportError's constructor (a subclass ofErroralready reportsname === "Error"through the prototype, so the override changed nothing observable either). Thecatch (e: any)inrequire()is now a barecatch (e)like the other catch clauses in the file;instanceofnarrows it.Related: #37742 records failures on more loader paths and matches the module name loosely in its
requireWithDepThatFailedToRequirecase because of this. With this change a recorded failure is never anAsyncImportError, so that match can be exact and itsstate !== State.Errorguard becomes redundant (it stays correct). TheCannot requireexpectations in #37759 and #37765 are direct requires and are unaffected.How did you verify your code works?
New test in
test/bake/dev/esm.test.ts, "the error for requiring a module with top level await names the require call that failed". One route handler goes through every path out ofrequire()'s catch: a directrequire()of a top-level-await module and one through a static import chain (both messages unchanged), the nestedrequire()chain, and a failure recorded byimport()that is then rethrown by a directrequire(), by arequire()of a static importer, and by a secondimport().With
USE_SYSTEM_BUN=1it fails on the four renamed messages (outer.ts,requires-tla.ts, andimports-requires-tla.tstwice). Withbun bd testit passes, as does the rest oftest/bake/dev/esm.test.ts.