Repository navigation
bake: make import() reject instead of throwing when a module fails to evaluate in the dev server - #37734
bake: make import() reject instead of throwing when a module fails to evaluate in the dev server#37734robobun wants to merge 5 commits into
Conversation
… evaluate in the dev server The dev server lowers import() to HMRModule.dynamicImport, which called loadModuleAsync without a try/catch. The loader throws synchronously when the target module (or one of its synchronously loaded dependencies) throws while evaluating, when the module already failed earlier, or when converting CommonJS exports into a namespace fails, so import() threw into the caller instead of returning a rejected promise like a native import() or bun build output does. Mark dynamicImport async so every failure surfaces as a rejection of the returned promise. The loader is still invoked synchronously, so importer tracking and evaluation order are unchanged.
|
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
ChangesDynamic import promise handling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — adding async to dynamicImport is the minimal correct fix for matching native import() rejection semantics.
What was reviewed:
- Confirmed the only callers are the printer's
hmr.dynamicImport(...)lowering (js_printer/lib.rs:2648, :3305), which already expects a promise; the.then(toESM)chaining it may append still composes. - Checked evaluation order is preserved: the body has no
await, soloadModuleAsyncand importer tracking still run synchronously at theimport()call site. - Verified the removed
Promise.resolve()wrapper is redundant underasync, and that the not-bundled fallback and.then(getEsmExports)branches are correctly adopted by the async function's promise. - Test covers each throw path enumerated in the description plus the success/cached and builtin-fallback cases with exact error messages.
Extended reasoning...
Overview
The PR changes HMRModule.dynamicImport in src/runtime/bake/hmr-module.ts from a plain method to an async method, so that synchronous throws from loadModuleAsync / getEsmExports become promise rejections. It also drops the now-redundant Promise.resolve() wrapper on the already-loaded branch and adds a short comment explaining why the method is async. A new dev-server test in test/bake/dev/esm.test.ts exercises every failure shape (ESM body throw, CJS body throw, cached failure, dependency throw, TLA-dependency rejection, CJS-namespace conversion throw) plus the success and builtin-fallback paths, asserting each returns a promise and settles with the expected value.
Security risks
None. This is dev-server-only module-loading glue; no auth, crypto, or untrusted-input parsing is involved. The change only affects whether an already-thrown error surfaces synchronously or as a rejection.
Level of scrutiny
Low-to-medium. The source change is a one-keyword addition plus a mechanical Promise.resolve unwrap. The semantics of async (synchronous body execution up to the first await, throws → rejections, returned thenables adopted) directly match the desired behavior, so the fix is correct by construction. I checked the two printer emit sites in src/js_printer/lib.rs — both substitute hmr.dynamicImport(...) for import(...) and may append .then(...), all of which compose unchanged with an async method. No other callers exist.
Other factors
The test is thorough and follows the existing devTest/minimalFramework pattern in the file, asserts exact error messages via a single toEqual on the full result object, and includes positive controls (ok/okAgain/builtin) so regressions in the happy path would also fail. The PR description documents the USE_SYSTEM_BUN=1 failure diff and passing suites. The unrelated pending-state loader bookkeeping issue is called out and intentionally scoped out. No CODEOWNERS entry covers src/runtime/bake/. No prior human review comments to address.
|
Updated 12:10 PM PT - Aug 12th, 2026
❌ @robobun, your commit 9732554 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 37734That installs a local version of the PR into your bun-37734 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — adding async to dynamicImport is the minimal correct fix at the right layer, and the test covers every synchronous-throw path in the loader.
What was reviewed:
- Traced each throw site in
loadModuleAsync/finishLoadModuleAsync/getEsmExportsagainst the test's probes; all are covered and assert the exact error message. - Confirmed the only callers are the two
.dynamicImport(lowerings injs_printer/lib.rs, which already expect a promise; no caller depends on a synchronous throw. - Checked that evaluation order and importer tracking are preserved (no
awaitbeforeloadModuleAsync, so it still runs synchronously); the droppedPromise.resolveis redundant underasync. - The comment-cop feedback was addressed in 6318f78 and the thread is resolved.
Extended reasoning...
Overview
The PR changes two lines in src/runtime/bake/hmr-module.ts: it marks HMRModule.dynamicImport as async and drops the now-redundant Promise.resolve() wrapper on the already-loaded branch. It adds a ~90-line dev-server test in test/bake/dev/esm.test.ts that probes eleven import() shapes (ESM/CJS body throws, cached failure, dependency throw, TLA rejection and its cached failure, CJS-namespace-conversion throw, plus successful and builtin cases) and asserts each returns a promise that settles with the module's specific error.
Security risks
None. This is dev-server-only bundled runtime code that changes how a load failure surfaces (rejection vs. synchronous throw); no new inputs are accepted and no security-sensitive path is touched.
Level of scrutiny
Low-to-medium. The source change is a two-line semantic adjustment whose effect is fully described by JS async semantics: any throw inside the body becomes a rejection, and returned promises are adopted. The function body has no await, so loadModuleAsync still executes synchronously at the import() call site — importer-set registration and module evaluation order are unchanged. I grepped for all callers of dynamicImport: the only ones on this method are the two InternalBakeDev lowerings in src/js_printer/lib.rs, which emit it in place of import() and therefore already treat the result as a promise. loadExports (the other loadModuleAsync consumer in this file) is already async, so it was never affected.
Other factors
The test is thorough and follows the harness conventions in the file (uses minimalFramework, devTest, exact-object toEqual with specific error messages, and includes negative controls that must keep working). The PR description documents that it fails on the released binary and passes on the debug build, and that the client-side import() suites (bundle.test.ts, hot.test.ts, react-spa.test.ts) still pass. The one piece of review feedback (comment-cop on doc-comment length) was addressed in 6318f78 and resolved. The pre-existing missing return in the with-options fallback and the pending-state bookkeeping issue are correctly identified as separate and out of scope.
There was a problem hiding this comment.
LGTM — adding async to dynamicImport is the minimal correct fix for making lowered import() reject instead of throw.
What was reviewed:
- Confirmed the only callers are the printer's lowered
import()emits (src/js_printer/lib.rs:2648,:3305), which already expect a promise; no site relied on the synchronous throw. - Checked that
loadModuleAsyncstill runs synchronously (noawaitbefore it), so importer tracking and evaluation order are unchanged; the removedPromise.resolvewrapper is redundant underasync. - Test covers each enumerated throw path plus the already-working success/builtin cases, asserts exact rejection messages, and was verified to fail on the released binary.
Extended reasoning...
Overview
The PR changes two files: src/runtime/bake/hmr-module.ts adds the async keyword to HMRModule.dynamicImport (and drops a now-redundant Promise.resolve wrapper), and test/bake/dev/esm.test.ts adds a dev-server test exercising every failure shape of a dynamically imported module. The production code change is effectively one keyword.
Security risks
None. This is dev-server-only runtime glue (hmr-module.ts, shared client/server HMR loader). No auth, crypto, filesystem, or network parsing is touched; the change only alters whether a thrown error surfaces as a synchronous exception or a rejected promise from the lowered import() call.
Level of scrutiny
Low-to-moderate. The mechanism is well understood: an async function with no await executes its body synchronously and converts any throw into a rejection of its returned promise. I grepped for all consumers of dynamicImport — the only ones are the two InternalBakeDev printer sites in src/js_printer/lib.rs that emit hmr.dynamicImport(...) in place of import(...), and callers of import() already treat the result as a promise. The already-loaded branch previously returned Promise.resolve(getEsmExports(...)) and now returns getEsmExports(...) directly, which async re-wraps identically; the .then(getEsmExports) and native-import() fallback branches are adopted by the async promise unchanged.
Other factors
The test is comprehensive (ESM throw, cached failure, CJS throw, dependency throw, TLA rejection + cached, CJS-namespace-conversion throw, plus positive controls for a good module and a builtin fallthrough), asserts exact messages via toEqual on the full result object, and the PR description shows the USE_SYSTEM_BUN=1 failure diff. The comment-cop feedback about doc-comment length was addressed in 6318f78 and the thread is resolved. The unrelated pre-existing issues (missing return in the with-options fallback, pending-state bookkeeping) are correctly noted as out of scope and tracked separately. First CI build was green on all test lanes.
There was a problem hiding this comment.
LGTM — adding async to dynamicImport is the minimal correct fix for matching native import() rejection semantics.
What was reviewed:
- Confirmed
loadModuleAsyncstill runs synchronously (noawaitbefore it), so importer tracking and evaluation order are unchanged. - Checked the printer call sites in
src/js_printer/lib.rs— the lowered.dynamicImport(...)is only ever used as a promise expression, so no caller depends on synchronous throw. - The dropped
Promise.resolve()wrapper is equivalent underasync; ifgetEsmExportsthrows (thecjsProxycase) it now correctly rejects. - Test covers each throw path enumerated in the description plus the success and builtin-fallthrough paths, with exact error messages.
Extended reasoning...
Overview
This PR changes HMRModule.dynamicImport in src/runtime/bake/hmr-module.ts from a plain method to an async method, and drops the now-redundant Promise.resolve() wrapper on the already-loaded branch. It adds a dev-server test in test/bake/dev/esm.test.ts that probes eleven import() shapes and asserts each returns a promise that settles with the expected value or error.
Security risks
None. This is a dev-server-only module loader (src/runtime/bake/), and the change only alters whether an existing throw surfaces synchronously or as a promise rejection. No new inputs are parsed, no auth/crypto/permissions code is touched.
Level of scrutiny
Low-to-moderate. The source change is effectively one keyword. The subtlety — whether making the function async shifts evaluation timing — is addressed: async functions execute synchronously to the first await, and there is none, so loadModuleAsync(id, true, this) still runs at call time and importers.add(importer) still fires before the caller sees a promise. I checked the two printer call sites (src/js_printer/lib.rs:2648, :3305) that emit .dynamicImport( — both replace an import() expression, whose consumers already expect a promise, so nothing depends on the old synchronous-throw behavior. The .then(getEsmExports) and native-import(id) fallthrough branches already returned promises and are adopted unchanged by the async wrapper.
Other factors
The comment-cop bot's feedback (long doc comment) was addressed in 6318f78 and the thread is resolved; there are no outstanding human review comments. The test is thorough: it distinguishes "threw synchronously" from "rejected" from "returned a non-promise", asserts exact error messages, and covers the cached-failure re-import paths for both ESM (recorded) and CJS (re-evaluated). The PR description documents that the test fails on the released binary and passes on this branch, and that adjacent dev-server suites (bundle.test.ts, react-spa.test.ts, hot.test.ts) still pass. No CODEOWNERS entry covers src/runtime/bake/ or test/bake/. The separately-noted pending-state bookkeeping issue and the missing return in the with-options fallback are pre-existing and explicitly out of scope.
Problem
bun ./index.html, or a Bake app in development),import()of a module that fails to evaluate throws synchronously into the caller instead of returning a rejected promise. Soimport(x).catch(...),Promise.all([...])andPromise.allSettled([...])work in production and break in development.import()into a call on its own module loader, and that loader reports load failures by throwing. Nothing between the loader and the caller turned the throw into a rejection.import()never throws synchronously, and neither doesbun buildoutput, which the dev runtime is documented to match.Fix
import()is rewritten to becomesasync, so anything it throws becomes a rejection of the promise it returns. The source is shared by the client and server dev runtimes, so both are fixed.await, so the loader still runs synchronously at the call site, and importer tracking and evaluation order are as before. The fallback to the runtime's ownimport()for unbundled modules is untouched (its missingreturnis a separate bug, bake: return the promise from import() with options in the dev server runtime #37727).import()on each failure shape above and checks that each call returns a promise that rejects with the module's error. Seven shapes throw synchronously without the fix; all pass with it. It runs in a route module, so it covers the server runtime; the client runtime is covered only by existing success-path tests.import()of it resolves tonull. Tracked separately.Background
bun ./index.html, or a framework app in development). It bundles modules on the fly and evaluates them through its own hot-reloading module registry rather than the engine's native module loader.import(x)in user code into a method call on the dev runtime, soimport()in development behaves exactly as that method does. The engine's ownimport()is reached only for modules the dev server did not bundle, such as builtins.asyncfunction runs synchronously up to its firstawait; with noawaitit runs entirely synchronously and only changes how the outcome is delivered: a return value resolves the returned promise, a throw rejects it, a returned promise is adopted. That property is the whole fix.import()returns a promise in every case, including link and evaluation failures;bun buildoutput keeps that by running the module initialiser insidePromise.resolve().then(...).[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
In the Bake dev server every dynamic import() is lowered to HMRModule.dynamicImport, which invoked the module loader directly and let its synchronous throws (a module body or CommonJS module throwing during evaluation, a missing or failing dependency, a previously recorded failure, or a namespace conversion error) escape into the caller, whereas native import() and bun build output always return a rejected promise. The fix makes dynamicImport an async method, so any failure raised on these paths becomes a rejection of the returned promise and patterns such as import().catch() and Promise.al…
Original description
What does this PR do?
In the dev server (
bun ./index.html, or a Bake app in development), a dynamicimport()of a module that fails to evaluate throws synchronously into the caller instead of returning a rejected promise:The dev server lowers every
import()tohmr.dynamicImport(id)(InternalBakeDevbranches insrc/js_printer/lib.rs).HMRModule.dynamicImportinsrc/runtime/bake/hmr-module.tscallsloadModuleAsyncdirectly, and the loader throws synchronously in several cases:finishLoadModuleAsyncrethrows),parseEsmDependencies),throw mod.failure), which also applies to modules whose top-level-await dependency rejected on a previous import,getEsmExportsin the already-loaded branch).A native
import()never throws synchronously, and neither doesbun buildoutput (Promise.resolve().then(() => (init_x(), exports_x)), which rejects), which is the behavior the dev runtime is documented to follow (see the header comment ofhmr-module.ts). Soimport(x).catch(...),Promise.all([...]),Promise.allSettled([...])and similar patterns that work in production break in development.The fix marks
dynamicImportasasync, so any of the throws above becomes a rejection of the returned promise. Nothing else moves: the loader is still invoked synchronously whenimport()is called (an async function runs synchronously up to its firstawait, and this one has none), so importer tracking and module evaluation order are unchanged; the already-loaded branch returns the namespace directly instead of wrapping it inPromise.resolve; the promise-returning branches are adopted by the async function's promise. The fallback to the runtime's ownimport()for modules the dev server did not bundle is also unchanged (the missingreturnin its with-options variant is a separate bug, fixed in #37727; the two changes touch different lines and compose).hmr-module.tsis shared by the client and server runtimes, so both are fixed.How did you verify your code works?
Added a dev server test to
test/bake/dev/esm.test.tsthat callsimport()on each failure shape listed above (ESM body throws, the same module again, CJS body throws, the same module again, a dependency throws, a top-level-await dependency rejects and the same module again, and a CJS module whose exports cannot be converted), plus two that already worked and must keep working (a module that loads fine, imported twice, and a builtin that falls through to the runtime'simport()). Each call is checked to return a promise and to settle with the module's error. The imports run in a route module, so the test exercises the server runtime; the client runtime is built from the samehmr-module.tssource. Note that "the same module again" means different things for the two module kinds: an ESM failure is recorded and rethrown, while a failed CJS module is marked stale and evaluated again (which throws again); the test comments say which is which.Without the fix (
USE_SYSTEM_BUN=1 bun test test/bake/dev/esm.test.ts, and likewise withsrc/stashed):With the fix,
bun bd test test/bake/dev/esm.test.tspasses (18 tests).test/bake/dev/bundle.test.tsandtest/bake/dev/react-spa.test.ts, whichimport()bundled modules on the client side (the success path of the changed method in the client runtime), andtest/bake/dev/hot.test.ts, as a general hot-reload regression check, also pass.While writing the test I noticed a separate loader bookkeeping issue: a module whose dependency throws synchronously (or whose own top-level await rejects) is left in the pending state, so a second
import()of it resolves tonull. That is independent of this change and is tracked separately; the new test imports those two shapes only once.