Repository navigation
Ask a runtime plugin's onResolve once: fix require() of a path a plugin moves into a namespace - #44474
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I also checked the Bun__onDidAppendPlugin ABI change for stale callers: the C thunk is generated from the Rust on_did_append_plugin signature (src/codegen/generate-host-exports.ts), and both callback(ctx) sites in src/jsc/bindings/BunPlugin.cpp match, so no call site is left passing the old second argument. The three plugin_runner.is_none()/is_some() to has_plugins swaps (hardcoded-alias fast path, could_be_plugin gate, concurrent-transpile gate) keep the original polarity, and the per-file reset in VirtualMachine.rs still clears the flag.
Extended reasoning...
The PR removes the transpile-time onResolve call path (Linker::link, PluginRunner.rs, PRINT_NAMESPACE_IN_PATH) and the resolve-hook namespace shortcut in ZigGlobalObject.cpp, collapsing VirtualMachine.plugin_runner to a bool; it touches no auth, crypto or injection surface but is a behavior change in module resolution for runtime plugins. Several confirmed inline findings (behavior regressions for onLoad-only namespaces, relative-path onResolve answers, stale transpiler cache entries, bare-name specifiers) already signal a human needs to review, so this body only records what else was checked and ruled out.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/bundler/transpiler.rs— Windows users with a catch-all onResolve now have static imports and literal require() of files on drive A:, Z:, a: or z: bypass their plugin, because the runtime path treats the drive letter as a namespace. extract_namespace at src/bundler/transpiler.rs:57-58 uses strict>and<, soA:\x.jsyields namespace "A" and run_on_resolve_plugins is asked with namespace "A" instead of the file namespace. Fix: use inclusive letter ranges (oris_ascii_alphabetic) so every drive letter is excluded, matching theisASCIIAlphatest the deleted C++ shortcut used.Why this was flagged
Trigger: on Windows, a
Bun.plugin()onResolve with afile-namespace filter, and a specifier whose first byte isA,Z,aorzfollowed by:and a separator, for exampleimport x from "A:\\proj\\x.js". All such specifiers now reach src/jsc/VirtualMachine.rs:5284-5291 because the link-time path was removed; could_be_plugin returns true (extensionjs), then extract_namespace at src/bundler/transpiler.rs:53-62 only returnsb""whenspecifier[0] > b'a' && specifier[0] < b'z'or the uppercase twin, which excludes the endpoints. The call at VirtualMachine.rs:5291 is then made with namespace "A" and specifier\x.js, so a plugin registered without a namespace never sees it and the import silently falls through to the filesystem resolver. Pre-existing for import() and require() with non-literal arguments, but every static import and literal require() is now routed through it. Remedy:specifier[0].is_ascii_alphabetic().Verification: Pre-existing. src/bundler/transpiler.rs:57-58 uses strict comparisons, so for
A:\x.jsextract_namespacereturnsb"A". At src/jsc/VirtualMachine.rs:5284-5297 namespace"A"is passed toplugin_runner_on_resolve_jsc, so afile-namespace onResolve is never invoked. The base's runtime block at base VirtualMachine.rs:5280-5288 made the identical misclassification; merging this PR neither introduces nor widens it.
There was a problem hiding this comment.
Code review found no new issues
No new issues were found in this update; 5 findings from earlier reviews are still open above.
Still open from earlier reviews (5):
- 🔴
src/bundler/linker.rs:431—Users whose runtime plugin's onResolve returns a relative or extensionless file path (the documented example at docs/ru… - 🔴
src/js_printer/lib.rs:6499—Users upgrading with an existing on-disk transpiler cache get Cannot find package 'virtual:from x.virtual' from static… - 🔴
src/jsc/bindings/ZigGlobalObject.cpp:3564—Users whose plugin registers only an onLoad for a namespace and whose code has a static import "ns:thing" now get Canno… - Also unresolved: 2 minor or pre-existing.
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
…o continues that skip nothing
There was a problem hiding this comment.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Still open from earlier reviews (1):
- 🔴
src/jsc/bindings/ZigGlobalObject.cpp:3564—Users whose plugin registers only an onLoad for a namespace and whose code has a static import "ns:thing" now get Canno…
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
4d04cdb
into
claude/module-key-resolved-once
|
This shows as merged because its commits are now in the branch of #44473, which was its base. Nothing has landed on |
2 participants
Stacked on #44473.
What does this PR do?
A runtime plugin's
onResolvewas asked twice about every specifier the transpiler could read:importandexport ... fromstatements, andrequire("..."),module.require("...")andrequire.resolve("...")with a literal. The second time it was asked about its own answer.What that broke, with a plugin from a
--preload:Cause
Linker::linkput every import record that is not a dynamic import throughonResolvewhile the file was transpiled, and printed the answer into the code. When the code ran, the module loader orrequire()resolved what was printed, which asksonResolveagain. Nothing else is resolved while transpiling at run time:linkonly maps the names of builtins.Printing the namespace into the path (
PRINT_NAMESPACE_IN_PATH, "used to prevent running resolve plugins multiple times for the same path") hid that for one case: an answer with a namespace, read back by the module loader's resolve hook, which returned any key in a namespace that has anonLoadhandler as it was.require()has no such shortcut. An answer without a namespace was always asked about again.Fix
onResolveis asked when the code runs, like for a specifier the transpiler cannot read, and not before. Removed:Linker::link, thePluginResolvertrait andLinker::plugin_runnerit went through, andPluginRunner::on_resolve, which was a second copy ofplugin_runner_on_resolve_jscand leaked a buffer for each answerPRINT_NAMESPACE_IN_PATHand what the printer did for itFIXME: with the linker's call gone it would keeponResolvefrom an import written as"ns:...", whichisolation.test.tshas a test ofVirtualMachine::plugin_runnerwas only ever asked whether it was there, so it ishas_plugins: bool.import()had the same fault from the other side:moduleLoaderImportModuleresolved the specifier and handed the loader the answer, andrequestImportModuleresolves what it is handed. It hands over the specifier and the referrer now, as WebCore's does, so the loader's call is the only one. (With the shortcut still there, that would have keptonResolvefromimport("ns:...").)What is printed for such an import changes, so the version of the transpiler cache goes from 33 to 34.
Every way to load or resolve
13 ways, to an ES module, a CommonJS module and a path that
onResolvemoves into a namespace, with a literal and with a specifier the parser cannot know, with the plugin from a--preloadand from the file itself: 138 cases.onResolvesendsatob,btoc,ctod. Right isb, with one call. Linux x64, canary7fe13e1b9.This PR changes 24. Eight are
import(), to ESM and to CommonJS:cwith 2 calls before,bwith 1 now. The other 16 are all a literal specifier with the plugin from a preload:import ... from,export ... from, to ESM and to CommonJSc, 2 callsb, 1 callrequire()in CommonJS and in ESM,module.require(), to ESM and to CommonJSc, 2 callsb, 1 callCannot find package 'ns:from-x.virtual'require.resolve(), to ESM and to CommonJSc, 2 callsb, 1 callrequire.resolve(), into a namespacens:from-from-x.virtual, 2 callsns:from-x.virtual, 1 callNo other case changes. The 12 that are left are
import.meta.resolve(), which does not askonResolveat all. Not changed here. Nor isBun.ModuleGraph'simport(), which asks twice: it needs the key before the load starts, and the loader has no way to import by key.import()of 24 kinds (relative, absolute,file:URL, query, hash, builtins, missing, JSON, text, CommonJS, fromeval,new Function, a timer,node:vm, adata:URL): the value or the name, message,code,referrerandspecifierof the error are what canary gives.With answers that are not a whole absolute path (relative, without an extension, a directory, a package's name; 72 cases, in #44473) nothing differs between that PR and this one, and an import statement,
require("...")andrequire.resolve("...")give what canary gives. The resolver's pass over the answer, which the second call used to be, is in #44473.What else changes
require("...")in a branch or a function that does not runonResolveis asked when the file loadsonResolvethrows, and therequire()is in atryonResolvereturns nothing, or apathornamespacethat is not validimport "ns:thing"where the plugin has anonLoadfornsand noonResolveCannot find package 'ns:thing'The last row is the one thing that stops working. It worked through the shortcut only:
import("ns:thing")andrequire("ns:thing")give that error on canary too. The documentation pairs anonResolvewith theonLoadof a namespace.How did you verify your code works?
18 tests in
plugins.test.ts: one call for each of eight ways to load and for three things Bun loads itself (a later preload,Module.runMain(), a test file), five ways into a namespace, arequire()that does not run, and arequire()in atry.The two that pass without the fix are
importandimport()into a namespace. They are there for the comparison withrequire(), and because no test had a static import thatonResolvemoves into a namespace: taking out the shortcut alone broke that and every test passed.preload-test.test.js,test/js/bun/plugin,test/js/node/module,test/js/bun/resolve,isolation.test.ts,worker.test.ts,hot.test.ts,bundler_compile.test.ts,mock-module.test.ts,transpiler-cache.test.ts, the dev server'splugins.test.tsandesm.test.ts: 877 pass, 0 fail.test/internal, before the merges of the base: 677 pass, 0 fail.