Repository navigation
Conversation
…lt-in module" text A static import of a node: specifier that is not a builtin is rejected in Linker::link when the importing file is transpiled on the JS thread (require(), plugin onLoad sources, the concurrent transpiler disabled), and by the runtime resolve hook otherwise. The linker logged the generic 'Could not resolve: "node:x". Maybe you need to "bun install"?' while the hook reports Node's 'No such built-in module: node:x', so the message depended on which path transpiled the importer. when_module_not_found is only ever reached for node: specifiers, so it becomes when_builtin_not_found and logs the same text as the hook. The browser and bare package branches it carried were unreachable from its single caller.
|
Warning Review limit reached
Next review available in: 2 minutes Limit details: You’ve used all 5 included reviews currently available under your plan. 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 (2)
Comment |
|
Status: ready for review. CI is green on f8dedea and the review threads are resolved. Reproduced on the released 1.4.0 binary with Two other differences between the same two transpile paths turned up while checking this and are filed separately, since they are behavior rather than message text: virtual modules ( |
There was a problem hiding this comment.
LGTM — the linker's node: not-found path now emits the same No such built-in module: <specifier> text as ResolveMessage::fmt, and the deleted browser/non-package branches were provably unreachable from the sole node: call site. One inline nit about the now-always-true IS_BUN const generic; not blocking.
Extended reasoning...
Overview
The PR renames Linker::when_module_not_found → when_builtin_not_found and replaces its three-branch Could not resolve: "…" message with the single No such built-in module: {specifier} string that ResolveMessage::fmt (src/jsc/ResolveMessage.rs:175) already produces for the concurrent-transpiler path. The target: BundleTarget parameter and Result<bool> wrapper are dropped since the browser/is_package_path arms could never be taken from the only call site (which is inside if strings::starts_with(path, b"node:") — always a package path, never target == Browser under the enclosing if IS_BUN). New tests in test/js/bun/resolve/resolve-error.test.ts cover require() vs import(), the DISABLE_ASYNC_TRANSPILER env, export * from, plugin onLoad sources, and the entry-point stderr snapshot.
Security risks
None. This is an error-message text change on a resolve-failure path; no new inputs are parsed, no allocation sizing changes, and the specifier is still formatted through bstr::BStr exactly as before.
Level of scrutiny
Low-to-medium. The functional surface is one format_args! string plus dead-branch removal in a private helper with a single caller. I confirmed:
when_builtin_not_foundhas exactly one call site (grep), inside thenode:prefix guard, so the removedis_package_path/BundleTarget::Browserarms were dead.add_resolve_errorlocates.specifierviaBabyString::in(&text, specifier_arg)(src/ast/lib.rs:1650); the new message containsimport_record.path.textverbatim, so.specifierand the.codederivation (which checksspecifier.starts_with(b"node:")at ResolveMessage.rs:109/118) continue to work.BundleTargetis still used byhardcoded_module::get, so no unused-import warning.- The
handles_import_errorsearly-return and the defer-to-runtime branch forRequire/RequireResolve/Dynamicare preserved unchanged.
Other factors
Tests follow harness conventions (tempDir, bunEnv spread, describe.concurrent, drained pipes via Promise.all, normalizeBunSnapshot), assert exact { code, specifier, importKind, message } shapes rather than substrings, and use one fixture file per load to avoid module-registry caching. The PR description lists the existing tests that were re-run (missing-module.test.js, bundler_browser.test.ts, etc.) and confirms bun build keeps its own message via bundle_v2.rs. The one inline nit — the now-vestigial <const IS_BUN: bool> generic — is a cleanup consistency point, not a correctness issue.
There was a problem hiding this comment.
LGTM — the earlier nit (dead IS_BUN const generic) and the comment-cop flags are all addressed, and the final diff is a clean message-text unification with thorough coverage.
What was reviewed:
when_builtin_not_foundis private with a single call site insidelink()'sif IS_BUN+node:branch, so the removed browser/non-package/Resultarms were provably dead.- New format string matches
ResolveMessage::fmt(src/jsc/ResolveMessage.rs:175) byte-for-byte;BabyString::r#ininadd_resolve_errorstill locates the specifier, so.specifier/.codekeep working (tests assert both). bun buildis unaffected —bundle_v2.rskeeps its own "bun install" text andbundler_browser.test.ts:465still pins it; no imports inlinker.rswent dead.
Extended reasoning...
Overview
This PR unifies the error message for a static import "node:<unknown>" so it reads No such built-in module: <specifier> regardless of whether the file was transpiled on the JS thread (via Linker::link) or on the concurrent transpiler (via the runtime resolve hook). It renames the private when_module_not_found → when_builtin_not_found in src/bundler/linker.rs, drops its unreachable branches (target param, browser-target hint, non-package-path arm, IS_BUN const generic, Result return), and swaps the format string to match ResolveMessage::fmt. A new describe.concurrent block in test/js/bun/resolve/resolve-error.test.ts covers require() vs import(), the concurrent-transpiler-disabled path, export * from, plugin onLoad sources, and the entry-point stderr snapshot.
Security risks
None. This changes only the human-readable text of a resolve error for an already-failing import; no parsing, validation, allocation sizing, or trust boundary is touched. The specifier is printed via bstr::BStr exactly as before.
Level of scrutiny
Low-to-medium. The Rust change is a net simplification of a private helper with one caller. I verified: (1) the sole call site is nested inside if IS_BUN after a starts_with(b"node:") check, so every removed branch was dead; (2) the new text is byte-identical to src/jsc/ResolveMessage.rs:175; (3) add_resolve_error derives .specifier via BabyString::r#in(&text, specifier_arg) and the specifier appears verbatim in the new text, so .specifier/.code (which keys on the node: prefix at ResolveMessage.rs:118) are preserved — the tests assert both; (4) bun build uses bundle_v2.rs, which retains its own "Maybe you need to bun install" copies, so bundler_browser.test.ts is unaffected; (5) no use statements went dead (resolver::, options::, BundleTarget are still referenced).
Other factors
All prior review threads are resolved: my earlier nit about the dead <const IS_BUN: bool> generic was applied in a143f8a, and the three comment-cop flags about a paragraph-long doc comment were addressed by removing the comment in f8dedea (the parity requirement is now enforced by the test instead). The new tests follow harness conventions — tempDir with using, {...bunEnv, ...}, concurrent pipe draining via Promise.all, exact-object toEqual assertions, normalizeBunSnapshot for the stderr snapshot, and one fixture file per load to avoid module-registry caching. The PR description states the new tests fail on the released binary and pass with the change, satisfying the "fails for the right reason" requirement.
Problem
import "node:<unknown>"rejects with a different.messagedepending on which thread transpiled it:require("./dep.ts")(and any other file transpiled on the JS thread: pluginonLoadsources,import()withBUN_FEATURE_FLAG_DISABLE_ASYNC_TRANSPILER=1, the entry point):Could not resolve: "node:x". Maybe you need to "bun install"?import("./dep.ts")(concurrent transpiler):No such built-in module: node:x.codeisERR_UNKNOWN_BUILTIN_MODULEin both cases; only the text differs, andbun installcannot fix anode:specifier (Bun has nonode:test/reportersmodule #24444 quotes this hint for a builtin Bun does not ship; that module is still missing after this PR, the hint is what changes).Linker::link(src/bundler/linker.rs) routes everynode:specifier that is not in the builtin table towhen_module_not_found, which logged the generic bare-package text. The concurrent transpiler's copy of this pass has no such branch, so there the import reaches the runtime resolve hook, whose message (ResolveMessage::fmtinsrc/jsc/ResolveMessage.rs) already uses Node's text.Fix
when_module_not_foundhas one caller, thenode:branch insidelink'sif IS_BUNblock (true since before the Rust port as well), so it becomeswhen_builtin_not_foundand logsNo such built-in module: <specifier>, the same text as the resolve hook. The parts only a different caller could reach are removed: the browser (--target="node"hint) and non-package message branches, thetargetparameter, theIS_BUNconst generic (always true there) and the never-failingResultreturn. Thehandles_import_errorsearly-out and the deferral ofrequire/require.resolve/import()to runtime are unchanged.ERR_UNKNOWN_BUILTIN_MODULEtext (which.codealready claimed) and no longer depends on the transpile path. The linker path still attaches the import's source position, which the hook path does not have..specifier/.codestill work because they locate the specifier inside the message text, which contains it.mock.module/build.moduleregistered for anode:name is only honored on the concurrent path because this branch fails first;require.resolve("fs")returns"node:fs"on the concurrent path and"fs"on this one); those are filed separately, and whether this branch should eventually be removed in favor of the hook is decided there. This change is compatible with either outcome.bun buildis untouched: the bundler has its own copy of thebun installmessage inbundle_v2.rs, andtest/bundler/bundler_browser.test.tsstill pins it.test/js/bun/resolve/resolve-error.test.ts(newstatic import of an unknown node: builtinblock:require()vsimport(), both again with the concurrent transpiler disabled, anexport * fromre-export, pluginonLoadsource, and the entry point's stderr). All four fail on the released binary with exactly the old text and pass with this change; the rest of the file,test/js/node/missing-module.test.js,test/regression/issue/25707.test.ts,test/js/bun/resolve/import-meta.test.js,test/js/node/test/parallel/test-stream-iter-disabled.js,test-require-node-prefix.jsandtest/bundler/bundler_browser.test.tsstill pass.Background
require(), the entry point, plugin-provided sources, or everything when the concurrent transpiler is disabled) or on the concurrent transpiler (import()and static imports of files,src/jsc/RuntimeTranspilerStore.rs). Each path runs its own copy of a small pre-pass over the file's import records: both rewrite known builtins through the hardcoded alias table and stripbun:; only the JS-thread copy (Linker::link) additionally fails the module up front fornode:names the table does not know. That early failure is what produced the text this PR changes.node:import in a concurrently transpiled file fails, and it formats its errors withResolveMessage::fmt.ResolveMessageis the error object thrown on both paths;.codeand.specifierare derived from the specifier's position inside the message text (BabyString), which is why the text must contain the specifier.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/resolve/resolve-error.test.ts