Repository navigation
Conversation
|
Warning Review limit reached
Next review available in: 19 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
Comment |
|
Updated 10:30 AM PT - Jul 6th, 2026
❌ @robobun, your commit 0961d1a has 1 failures in
🧪 To try this PR locally: bunx bun-pr 33438That installs a local version of the PR into your bun-33438 --bun |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Not adding The actual fix for #33180 is #33184, which corrects the pending-promise check in Worth noting the two are independent and compose: once #33184 stops the false positive, the remaining (correct) throws for real top-level-await graphs carry |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — routes an existing error through the ErrorCode machinery so require(esm) on TLA throws with code: 'ERR_REQUIRE_ASYNC_MODULE', matching Node.
Extended reasoning...
Overview
This PR replaces three throwTypeError/throwVMTypeError call sites (two in fetchCommonJSModule in ModuleLoader.cpp, one in functionEsmLoadSync in ZigGlobalObject.cpp) with a single new helper Bun::throwRequireAsyncModuleError that routes through Bun::throwError(..., ErrorCode::ERR_REQUIRE_ASYNC_MODULE, ...). The error code was already declared in ErrorCode.ts but unused. The message text is unchanged; only the error class (TypeError → Error) and the presence of .code change, aligning with Node's documented behavior. Tests are added in require.test.ts (direct and transitive TLA) and strengthened in plugins.test.ts (async virtual module), plus one existing test updated to assert .code instead of instanceof TypeError.
Security risks
None. This only changes which error constructor and .code property are attached to an already-thrown error on an existing failure path. No new inputs are parsed, no validation is loosened, no auth/crypto/permissions code is touched.
Level of scrutiny
Low. The native change is a mechanical 4-line helper wrapping the existing centralized Bun::throwError / ErrorCode machinery — exactly what the repo's guidance recommends over inline error construction. Each call-site replacement preserves the surrounding RELEASE_AND_RETURN / return control flow. ERR_REQUIRE_ASYNC_MODULE was already registered in ErrorCode.ts as an Error, so the generated code path is well-trodden.
Other factors
The PR includes new tests that assert the exact error class, .name, .code, and message contents for both direct and transitive TLA, plus the async-plugin path, and the description confirms they fail on main and pass on the branch. No CODEOWNERS cover the touched files, there are no outstanding human review comments, and the intentional TypeError → Error change is called out explicitly in the description as a Node-compat correction.
require() of an ES module whose graph cannot finish loading synchronously threw a TypeError with no .code. Node throws an Error carrying ERR_REQUIRE_ASYNC_MODULE, which loaders catch to fall back to import(), so that detection branch never fired on Bun. Route the three throw sites that share this message through the existing (previously unused) ERR_REQUIRE_ASYNC_MODULE entry in ErrorCode.ts.
09d388c to
be19d26
Compare
There was a problem hiding this comment.
LGTM — clean Node compat fix routing three existing throw sites through the already-declared ERR_REQUIRE_ASYNC_MODULE code.
Extended reasoning...
Overview
This PR swaps three throwTypeError/throwVMTypeError calls for a new Bun::throwRequireAsyncModuleError helper that routes through Bun::throwError(..., ErrorCode::ERR_REQUIRE_ASYNC_MODULE, ...). The error code was already declared in ErrorCode.ts (as Error, matching Node) but unused. The message text is unchanged; only the error class (Error instead of TypeError) and the .code property differ. Three test files are updated: two new test cases in require.test.ts for direct and transitive TLA, a strengthened assertion in plugins.test.ts for the async-plugin path, and one existing instanceof TypeError check in require-esm-transitive-tla.test.ts migrated to check .code instead.
Security risks
None. This is purely error-metadata plumbing for Node compatibility — no input parsing, no auth, no resource handling changes.
Level of scrutiny
Low-to-medium. The touched files (ModuleLoader.cpp, ZigGlobalObject.cpp) are core, but the change is mechanical: same message, same control flow (each site still throws then RELEASE_AND_RETURN), just a different error constructor. I verified ERR_REQUIRE_ASYNC_MODULE exists in ErrorCode.ts:220, that Bun::throwError's signature (ErrorCode.h:63) returns EncodedJSValue matching the ZigGlobalObject.cpp call site's return, and that ZigGlobalObject.cpp already includes ModuleLoader.h for the declaration. The two ModuleLoader.cpp sites discard the return value, which is fine since the exception lives on the scope.
Other factors
The PR description is thorough, correctly identifies all three sites, and explains why the message text isn't copied from Node. The one intentional behavior change (instanceof TypeError no longer matches) is called out and moves Bun closer to Node. Test coverage hits all three modified code paths. The bug-hunting system found nothing. No outstanding reviewer comments.
There was a problem hiding this comment.
LGTM — clean Node compat fix routing three existing throw sites through the already-declared ERR_REQUIRE_ASYNC_MODULE code.
Extended reasoning...
Overview
This PR changes three throw sites in the require(esm) path to throw an Error with code === "ERR_REQUIRE_ASYNC_MODULE" instead of a bare TypeError with no .code, matching Node.js. It adds a small shared helper Bun::throwRequireAsyncModuleError in ModuleLoader.cpp/.h and calls it from the two fetchCommonJSModule pending-promise branches and the functionEsmLoadSync pending branch in ZigGlobalObject.cpp. The error code was already declared in ErrorCode.ts but unused. The message text is unchanged.
Test changes: two new tests in require.test.ts (direct and transitive TLA), a strengthened assertion in plugins.test.ts (async virtual module), and one existing test in require-esm-transitive-tla.test.ts updated from instanceof TypeError to check the error code — the only in-tree consumer of the old class.
I confirmed the three remaining throw new TypeError(...async module...) matches in src/js/builtins/CommonJS.ts are inside a block-commented-out legacy implementation, so all live sites are covered.
Security risks
None. This only changes the error prototype and attaches a .code property on an existing error path. No input handling, auth, or resource management is touched.
Level of scrutiny
Low. This is a mechanical Node-compat refactor using the established Bun::throwError(globalObject, scope, ErrorCode::X, ...) pattern already used throughout src/jsc/bindings/. The helper is a one-liner; each call site is a 1:1 swap with identical control flow (RELEASE_AND_RETURN after the throw is preserved). None of the touched files are covered by CODEOWNERS.
Other factors
The intentional behavior change (TypeError → Error) is called out in the PR description and is the point of the fix — ecosystem loaders branch on e.code === "ERR_REQUIRE_ASYNC_MODULE" to fall back to import(). Test coverage exercises all three modified sites and asserts class, name, code, and message content. No prior reviews or outstanding comments on the PR.
Status: ready to merge, CI red only on infra lanesThe diff is unchanged since the LGTM and passes locally. Build 68960 finished 272 passed / 2 failed / 12 expired, and neither failure executes any code from this change. 1. 2. Plus 12 jobs expired waiting for an agent. The Verified locally against the current sha: The new tests also ran green on Windows shards in CI, which covers the I have used my one CI re-roll and won't push further retriggers. Handing off for a maintainer to merge or re-run the two darwin lanes. |
|
Closing in favour of #41788, which throws |
require()of an ES module whose graph contains top-level await throws a bareTypeErrorwith no.code:Node documents
ERR_REQUIRE_ASYNC_MODULEas the signal for "this module needsimport()", and the ecosystem'srequire(esm)migration path istry { require(f) } catch (e) { if (e.code === "ERR_REQUIRE_ASYNC_MODULE") return import(f) }. On Bun that branch never matched, so loaders and shims rethrow instead of falling back.Cause
Three sites throw the same
require() async module "..." is unsupportedmessage, and all three usedthrowTypeError/throwVMTypeErrordirectly, which attaches nocode:functionEsmLoadSyncinZigGlobalObject.cpp— therequire(esm)path, reached whenever the synchronous module-loader drain leaves the load promise pending (top-level await anywhere in the graph).fetchCommonJSModuleinModuleLoader.cpp—require()of a virtual module whose pluginonLoadis async.ERR_REQUIRE_ASYNC_MODULEwas already declared insrc/jsc/bindings/ErrorCode.ts(as anError, matching Node) but nothing ever used it.Fix
A single
Bun::throwRequireAsyncModuleErrorhelper routes all three sites throughBun::throwError(..., ErrorCode::ERR_REQUIRE_ASYNC_MODULE, ...). The thrown value is now anErrorwithcode === "ERR_REQUIRE_ASYNC_MODULE", matching Node's class and code.The message text is unchanged. Node's wording points at
--experimental-print-required-tla, a flag Bun does not implement, and Bun's message already names the offending module, so copying it verbatim would be worse. Only the error class and.codechange.Note this is a behavior change for anyone matching
e instanceof TypeErroron this throw: it is nowError, as in Node. The one in-tree test doing that is updated to assert the code instead.This is independent of #33249, which makes some TLA graphs load successfully under
require(); graphs that genuinely need the event loop still throw, and now they throw the right thing.Verification
New tests in
test/js/bun/resolve/require.test.ts(direct and transitive top-level await) and a strengthened assertion intest/js/bun/plugin/plugins.test.ts(async plugin module) fail onmainwithExpected: "ERR_REQUIRE_ASYNC_MODULE" / Received: undefined.