node:module: implement synchronous module.registerHooks() (+49 tests) - #35690
Draft
cirospaciari wants to merge 12 commits into
Draft
cirospaciari wants to merge 12 commits into
cirospaciari wants to merge 12 commits into
Conversation
…on hooks [allow size] Adds Node's synchronous module customization hooks API (module.registerHooks, Node 23+, the supported successor to the deprecated off-thread module.register). Resolve and load hook chains run for require(), require.resolve(), static and dynamic import, and import.meta.resolve, with Node's contract: parentURL/conditions/importAttributes context, nextResolve/ nextLoad chaining, the shortCircuit requirement, format overrides (module/commonjs/json/typescript variants), builtin observation with node: URLs and null default source, deregister(), and ERR_INVALID_RETURN_PROPERTY_VALUE / ERR_UNKNOWN_MODULE_FORMAT validation. The chain and validation logic is a port of Node's lib/internal/modules/customization_hooks.js into a new internal module. The native side consults it from the resolver funnel (resolveMaybeNeedsTrailingSlash and its jsc_hooks twin, before the builtin alias fast path), from the module loader (transpileFile, before reading a module off disk), and from fetchBuiltinModule for builtins - all gated on hook counters mirrored onto VirtualMachine so the hook-free path only pays an integer compare. The chain's default resolve step re-enters Bun's native resolution under a reentrancy flag so it cannot recurse into the hooks. module.register() stays a no-op but now emits Node's DEP0205 deprecation warning (once per process, suppressed by --no-deprecation) pointing at registerHooks(); covered by tests in node-module-module.test.js.
Vendors byte-verbatim from Node v26.3.0: - test/js/node/test/module-hooks/: 47 of the 63 synchronous module.registerHooks() tests (plus their fixtures), all passing. The suite directory is added to the node-test runner's inclusion list; a deliberately failing canary run confirmed the runner executes the new directory. - test/js/node/test/es-module/: test-esm-import-meta-resolve-hooks.mjs and test-import-preload-require-cycle.js (with the import-require-cycle fixtures). The es-module runner line matches the pending es-module suite branch, so the two merge cleanly. Not vendored, with reasons: custom-conditions* (3) need per-resolution conditions plumbed into the native resolver; load-builtin-override-* (3) replace a builtin's implementation via load-hook format override, which is unsupported; *inline-typescript* and preload (5) depend on the 11 MB fixtures/snapshot/typescript.js fixture; load-async-and-sync and require-esm (2) need the off-thread module.register() protocol; builtin-require and load-builtin-require (2) require node:sea; create-require-with-url (1) needs URL-string specifiers preserved through createRequire(url); test-esm-import-attributes-identity.mjs needs the module map keyed by import attributes; test-esm-register-deprecation.mjs asserts Node's [DEP0205] stderr format and --throw-deprecation exit behavior, which Bun's warning printer does not produce.
Collaborator
Contributor
|
Found 4 issues this PR may fix:
🤖 Generated with Claude Code |
Contributor
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
…aude/node-esm-loader-hooks # Conflicts: # src/jsc/bindings/ErrorCode.ts
… fallout - Restore src/jsc/bindings/BunHeapProfiler.h (deleted by #36500 on main, but still needed by GeneratedJS2Native.h for the $newCppFunction calls added on this branch). - VirtualMachine.rs / jsc_hooks.rs: resolve_maybe_needs_trailing_slash now takes ResolveMode, not is_esm/is_user_require_resolve; derive the two bools from `mode` for Bun__runModuleResolveHooks. Use `&raw const` for the pointer args. - web_worker.rs: parent_ref binding was lost in the merge; read heap_profiler_config via `(*parent)` like the neighbouring fields. - permission.rs: switch to bun_threading::RwLock (no poisoning) and bun_core::env_var::NODE_OPTIONS per clippy disallowed-types/methods. - clap Diagnostic fields pub (read by bun_runtime::cli::Arguments). - path.rs resolve_posix_t pub(crate) (called from permission.rs). - Minor clippy: then_some, contains(), SAFETY comment placement, unreachable_pub on exec_check.
…aude/node-esm-loader-hooks # Conflicts: # src/jsc/bindings/BunHeapProfiler.h # src/runtime/cli/run_command.rs # src/runtime/permission.rs
…aude/node-esm-loader-hooks
Collaborator
|
Cross-reference: this resolves #27369 ("Bun does not support One small thing the stub had that this does not: the |
This was referenced Sep 2, 2026
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements the synchronous module customization hooks API —
module.registerHooks()(Node 23+, the supported successor tomodule.register()) — and vendors the upstreamtest/module-hooks/suite that exercises it.What this does
module.registerHooks({ resolve, load })now works in Bun for bothrequire()andimport:require,require.resolve, static/dynamicimport,import.meta.resolve), with Node's context contract (parentURL,conditions,importAttributes),nextResolvechaining, theshortCircuitrequirement, andERR_INVALID_RETURN_PROPERTY_VALUEvalidation.{url, context.format}contract and can replace the source; returnedformat(module,commonjs,json, the*-typescriptvariants) maps onto Bun's loader + module-type pipeline. Builtins are observed withnode:-prefixed URLs and anulldefault source (source overrides for builtins are ignored, like Node ignores them forbuiltinformat).hook.deregister(), hook chaining/merged contexts, and virtual specifiers (e.g.virtual:xids produced by hooks) work.module.register()stays a no-op but now emits Node'sDEP0205deprecation warning pointing atregisterHooks().How it's wired
src/js/internal/modules/customization_hooks.ts— port of Node'slib/internal/modules/customization_hooks.jschain/validation logic, plus the two Bun-facing entry points (runResolveHooksBun,runLoadHooksBun).VirtualMachine::resolve_maybe_needs_trailing_slash, and itsjsc_hooks.rstwin) consults the JS resolve chain before the builtin-alias fast path; the module loader (transpile_file) consults the load chain before reading a module off disk;fetch_builtin_moduleconsults it for builtins. All gated on plain integer counters mirrored ontoVirtualMachine, so the hook-free hot path only pays an integer compare.fs.readFileSync.ERR_INVALID_RETURN_PROPERTY_VALUEandERR_UNKNOWN_MODULE_FORMAT(message builders match Node's text).Tests
Vendored byte-verbatim from Node v26.3.0 (each verified failing on system Bun and passing on this build):
test/js/node/test/module-hooks/— 47 of the 63 sync-hooks tests (the suite is added to the CI runner's inclusion list; a canary run verified the runner executes the new directory).test/js/node/test/es-module/—test-esm-import-meta-resolve-hooks.mjsandtest-import-preload-require-cycle.js(the runner line matches the es-module suite PR; trivial merge overlap).Not covered (dropped, with reasons)
module.register()protocol — the 20test-esm-loader-*failures in the es-module suite all depend on it (--experimental-loader/--loaderchildren or directregister()calls);register()is deprecated upstream (DEP0205). Tests: gated on that feature.custom-conditions*(3) — per-resolutioncontext.conditionsoverrides need conditions plumbed through the native resolver per-call.load-builtin-override-{commonjs,json,module}(3) — replacing a builtin's implementation via load-hook format override is unsupported.*-inline-typescript*+preload(5) — depend on the 11 MBfixtures/snapshot/typescript.jsupstream fixture.load-async-and-sync,require-esm(2) — need off-threadregister().builtin-require,load-builtin-require(2) — requirenode:sea, which Bun does not implement.create-require-with-url(1) — needs URL-string specifiers preserved throughcreateRequire(url).test-esm-import-attributes-identity.mjs(1) — needs the ES module map keyed by import attributes.test-esm-register-deprecation.mjs(1) — the DEP0205 warning is implemented (with Bun-native tests intest/js/node/module/node-module-module.test.js), but the upstream test asserts Node's[DEP0205]stderr format and--throw-deprecationexit behavior, which Bun's warning printer does not produce.