Repository navigation
Resolve static imports of unknown node: names at runtime so mock.module / build.module / plugins apply to require()d files and the entry point - #39314
Conversation
…ailing them in the linker Linker::link, which runs over files transpiled on the JS thread (the entry point, require()d files, plugin-provided sources, everything when the concurrent transpiler is disabled), rejected any static import of a node: name missing from the builtin table while the file was still being transpiled. The runtime resolve hook, which is what files transpiled by RuntimeTranspilerStore go through, checks the virtual module map (mock.module / build.module), onResolve plugins and the resolver before giving up, so the same import worked in an import()ed file and failed in a require()d one or in the entry point. Drop the linker arm so these imports reach the runtime resolve like every other specifier. Unknown names still fail, now with the same "No such built-in module" ResolveMessage on both paths. when_module_not_found and bun_bundler::Error::ResolveMessage had no other users and are removed.
|
Warning Review limit reached
Next review available in: 1 minute 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 (6)
Comment |
|
Reproduced on the 1.4.0 release binary and on main: with Fix: this PR (#39314). The new tests in |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes how the runtime linker handles unknown node: specifiers (deferring to runtime resolve, which also changes the user-visible error text/position on the JS-thread path), a maintainer look would still be worthwhile.
Checked: when_module_not_found and Error::ResolveMessage have no remaining references; the deleted arm was inside if IS_BUN so the <false, false> caller in transpiler.rs was never affected; inline_require_and_import_errors: false at transpiler.rs:2507 confirms the is_disabled write for HANDLES_IMPORT_ERRORS was inert on the runtime printer; could_be_plugin("node:foo") returns true so the fall-through now correctly reaches the plugin runner.
Extended reasoning...
Overview
Deletes the node: arm from Linker::link (src/bundler/linker.rs) that failed static imports of unknown node: names at transpile time on the JS-thread path only, along with its now-dead callees when_module_not_found and bun_bundler::Error::ResolveMessage. Adds tests covering mock.module, build.module, onResolve({namespace: "node"}), and error-message consistency across the concurrent/JS-thread transpile paths and entry/require/import shapes.
Security risks
None. This is a resolve-timing change that defers an early rejection to the existing runtime resolver; no new inputs are trusted and no security checks are removed.
Level of scrutiny
Module resolution is a hot, user-visible core path. The mechanism (JS-thread Linker::link short-circuiting before the virtual-module map / plugin registry / tsconfig paths are consulted, while the concurrent transpiler has no such check) is well-traced in the description and I verified the key claims: only three link::<_, true> callers exist (jsc_hooks.rs:3131, AsyncModule.rs:1218, transpiler.rs:2916), the deleted block was inside if IS_BUN, and the "bun install" text still exists at all its bundle_v2/bake call sites so bun build is unaffected. The is_disabled inertness claim checks out for the runtime printer.
That said, this is a behavior change: the JS-thread path now surfaces No such built-in module: node:x from the runtime resolver instead of the transpile-time Could not resolve ... bun install? with a source position, and Bun.Transpiler with target: "bun" will now pass unknown node: names through instead of erroring. The description argues both are improvements (unifies with the concurrent path and with Node), which is reasonable, but a maintainer should confirm the error-message/position tradeoff is acceptable.
Other factors
The tests are thorough — matrix over concurrent-transpiler on/off, entry point vs require()d vs import()ed, and they assert exact {name, code, specifier, message} on the error object rather than just toThrow(). All follow harness conventions (tempDir, bunEnv spread, concurrent pipe drain, combined-object assertions). No prior human reviews on the timeline.
|
One correction to the review's extended reasoning: The error-text/position trade-off on the JS-thread path is deliberate and is called out in the Fix section; the position was only ever printed for files transpiled on that path, |
Problem
node:name that is not a builtin (mock.module("node:x", ...),build.module("node:x", ...)) is seen by a staticimportin animport()ed file, but the sameimportin arequire()d file or in the entry point fails the whole file withCould not resolve: "node:x". Maybe you need to "bun install"?. WithBUN_FEATURE_FLAG_DISABLE_ASYNC_TRANSPILER=1theimport()ed file fails the same way.node:name that anonResolve({ namespace: "node" })plugin or a tsconfigpathsentry maps.import("node:x")andrequire("node:x")of these names work everywhere; only the static form is affected.Linker::link(src/bundler/linker.rs:449on main), which only runs over files transpiled on the JS thread, fails every static import of anode:name missing from the builtin table while the file is still being transpiled (when_module_not_found). That is before the runtime resolve, which is where virtual modules, plugins and tsconfig paths are consulted. The concurrent transpiler's own pass over the import records (src/jsc/RuntimeTranspilerStore.rs:1068) has no such check, so its files reach the runtime resolve and work.Fix
node:arm. A static import of an unknown-to-the-tablenode:name is now left in the import record like every other specifier and resolved by the runtime resolve when the module is linked, on both transpile paths.import(),require()and every concurrently transpiled static import. The arm was a transpile-time shortcut from when the linker still resolved all imports (throw error if node module does not exist #3913), and fix(linker): defer dynamic import() of unknown node: modules to runtime #26981 already had to carveimport()out of it for the same reason.ResolveMessagewithcodeERR_UNKNOWN_BUILTIN_MODULE(unchanged) and Node's textNo such built-in module: node:x, which the concurrent path already produced; the JS-thread path used to print thebun installtext with the import's source position. This makes Report unknown node: builtins in static imports with "No such built-in module" on every transpile path #39246 (message text only) unnecessary.try { require("node:x") }it setpath.is_disabled, which the printer only reads underinline_require_and_import_errors, and the runtime printer (Format::EsmAscii,src/bundler/transpiler.rs:2507) sets that to false.try { require("node:x") }threwERR_UNKNOWN_BUILTIN_MODULEat runtime before and still does.when_module_not_found(this arm was its only caller;node:names are always package paths and the runtime target is never the browser, so only itsbun installbranch was reachable) andbun_bundler::Error::ResolveMessage(only produced there).bun build,bun build --no-bundleandBun.Transpilerare unaffected: the bundler reports unresolved imports frombundle_v2.rs, and the other two never ranLinker::link(verified: all three produce byte-identical output before and after for a file importing an unknownnode:name,Bun.Transpilerwith each of the three targets).require("node:x")of such a virtual module throwsFailed to fetch builtin moduleon every path. That is the CommonJSnode:fast path (src/js/builtins/CommonJS.tstojsFunctionRequireNativeModule), the area mock.module: apply builtin mocks to require() under bun test #34984 is about.Could not resolvetext and passing with this change:test/js/bun/test/mock/mock-module.test.ts:mock.module()of anode:name, seen by a static import in arequire()d fixture, and (via--preload) by the entry point, arequire()d file and animport()edexport ... from, with and without the concurrent transpiler.test/js/bun/plugin/plugins.test.ts: the same matrix forbuild.module(), plus anonResolve({ namespace: "node" })plugin redirecting the static import in all three files (registering a plugin puts every file on the JS thread, so one run covers them).test/js/bun/resolve/resolve-error.test.ts: an unknownnode:name rejects with the samename/code/specifier/messagefromrequire()andimport(), with and without the concurrent transpiler, and the entry point reports the same message.test/js/bun/resolve/,test/js/bun/plugin/,test/js/bun/test/mock/,test/regression/issue/25707.test.ts,test/js/node/missing-module.test.js,test/js/node/test/parallel/test-require-node-prefix.jsandtest-stream-iter-disabled.js. The one failure,load-same-js-file-a-lot.test.ts, is its second test hitting the default 5s timeout under the debug build on this machine (the first test of the file takes 17s here against a 20s budget); it imports a file with no import records, which this change does not touch.Background
transpile_source_code_innerinsrc/runtime/jsc_hooks.rs, which callsLinker::linkat line 3131) or by the concurrent transpiler, a thread pool inRuntimeTranspilerStore. The JS thread handles the entry point, everyrequire(), plugin-provided sources, and everything once anonLoad/onResolveplugin is registered orBUN_FEATURE_FLAG_DISABLE_ASYNC_TRANSPILERis set; ordinaryimports after startup use the pool. Only the JS-thread path runsLinker::link.Linker::linkis a pass over a transpiled file's import records. Today it rewrites builtin aliases (fstonode:fs), stripsbun:prefixes and offers namespaced specifiers to onResolve plugins; it does not call the resolver.moduleLoaderResolve(src/jsc/bindings/ZigGlobalObject.cpp:3650), which checks the virtual module map and then the Rust resolve (resolve_maybe_needs_trailing_slash,src/jsc/VirtualMachine.rs:4528): onResolve plugins for namespaced specifiers, the builtin alias table, then the package resolver (where tsconfigpathsapply). Anode:name that fails all of these is formatted byResolveMessage::fmt(src/jsc/ResolveMessage.rs:171) asNo such built-in module: <specifier>.BunPlugin.cppthat bothmock.module()andbuild.module()write into; it is only consulted by the runtime resolve and fetch, never by the transpiler.ResolveMessage: the error object for a failed resolution. Its.codeand.specifierare derived from the specifier, which is why those already matched on both paths and only.messagediffered.