fix: migrate to module.registerHooks() and fix CommonJS interop regressions#633
Open
cjnoname wants to merge 7 commits into
Open
fix: migrate to module.registerHooks() and fix CommonJS interop regressions#633cjnoname wants to merge 7 commits into
cjnoname wants to merge 7 commits into
Conversation
…ssions module.register() has been runtime-deprecated as DEP0205 since Node.js v25.9.0, emitting a deprecation warning on every invocation. Switch to the synchronous, in-thread module.registerHooks() (Node.js >= 23.5.0 / 22.15.0) when available, falling back to module.register() on older runtimes. The synchronous loader routes CommonJS require() and named-export detection through the customization hooks, which surfaced several interop differences that the old worker-thread loader hid. Fix them so behaviour is unchanged: - Defer modules oxc-node does not transform (plain .js/.cjs/.json, native addons, …) and anything Node classifies as commonjs back to Node.js, so its built-in CommonJS named-export detection (incl. transitive __export(require()) re-exports) and pirates-based transpilation/source maps keep working. - Complete missing TypeScript/JSX extensions for CommonJS require() specifiers, which the ESM-style resolve hook's nextResolve does not resolve via Module._extensions where pirates installs its handler. - Make ResolveContext/LoadContext conditions and importAttributes optional, as the CommonJS require() path does not always provide them. - Move the loader-thread keep-alive MessageChannel out of the shared hook module so it only runs on the dedicated register() loader thread, instead of pinning unrelated worker threads (e.g. test runners) and preventing process exit. Verified on Node.js 24 and 26 with the integrate-module, integrate-module-bundler and integrate-ava suites all green and no DEP0205 warning.
Replace the filesystem-probing (existsSync) extension completion for CommonJS require() of TypeScript files with a dedicated native resolver entry, resolveCjsSpecifier(). It reuses oxc-node's resolver so tsconfig `paths`, package `exports`, conditions and symlinks are all honoured (the previous best-effort probing only handled relative/absolute specifiers), and removes the synchronous filesystem stat loop from the hot resolve path.
- Replace the per-resolve/per-load `new URL(url).pathname` transformability check
with a substring regex on the raw string (~9.6x faster on the hot path; extension
characters are never percent-encoded so this is safe), and check the cheap
`format === 'commonjs'` branch before it in the load hook.
- Load index.js via createRequire instead of a named ESM import. The native binding's
exports are only statically detectable once oxc-node's hooks are active, so a named
`import { … } from './index.js'` could fail to find exports under some bootstrap
entrypoints (e.g. --eval). This also avoids ESM named-export detection overhead.
A specifier that already carries a transformable extension (e.g. `./foo.ts`) is always oxc-node's to resolve, so the speculative Node `nextResolve` — whose result would only be discarded — can be skipped, resolving relative TypeScript imports once instead of twice. ~18% faster on a TypeScript-heavy module graph (200 modules: 36.5ms -> 29.9ms).
For modules oxc-node has fully resolved outside node_modules (URL + concrete format already known), build the resolve hook output directly rather than calling nextResolve with the resolved URL. The latter made Node redundantly re-read package.json and stat the file for a path oxc-node had already resolved. node_modules modules still defer to Node so CommonJS named-export detection is unaffected. ~20% faster on a TypeScript-heavy module graph (500 modules: 70.5ms -> 56.2ms).
The synchronous-loader fast paths in hooks.mjs rely on a synchronous nextResolve (only true under module.registerHooks()). Under the module.register() fallback, hooks run on a worker thread where nextResolve is asynchronous, so the try/catch guarding the speculative resolve cannot catch its rejection and tsconfig paths aliases threw ERR_MODULE_NOT_FOUND. The async worker loader also does not have the CommonJS interop limitations hooks.mjs works around, so the register() entry (esm.mjs) now uses the plain resolver directly, while registerHooks keeps using the optimized hooks.mjs.
Author
|
@Boshen Please review this PR. Thanks. |
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
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.
Summary
module.register()has been runtime-deprecated as DEP0205 since Node.js v25.9.0 (see nodejs/node#62401), emitting a warning on every invocation:This migrates
register.mjsto the synchronous, in-threadmodule.registerHooks()(Node.js ≥ 23.5.0 / 22.15.0) when available, falling back tomodule.register()on older runtimes. It supersedes #585, which was closed because switching toregisterHookssurfaced CommonJS interop regressions that the asynchronous worker-thread loader had hidden. This PR fixes the root causes so behaviour is unchanged — and ends up faster than the previous worker-thread loader.Correctness: CommonJS interop under the synchronous loader
The synchronous loader routes CommonJS
require()and named-export detection through the customization hooks (the old loader ran them on a worker thread and Node handled CJS itself). That exposed four issues, each fixed inhooks.mjs/src/lib.rs:import { foo } from "cjs-pkg"failed with "does not provide an export named 'foo'" for packages using__export(require("./src")). Fixed by deferring modules oxc-node does not transform (plain.js/.cjs/.json, native addons, …) and anything Node classifies ascommonjsback to Node.js, so its built-in CJS named-export detection keeps working.require()of TypeScript files — extensionlessrequire("./foo")of a.tsfailed because the resolve hook'snextResolvedoes not consultModule._extensions(wherepiratesinstalls its handler). Fixed by completing the extension via oxc-node's own resolver (resolveCjsSpecifier, honouring tsconfigpaths/exports/ conditions / symlinks) so the CJS loader +piratescan transpile it..cts— stack traces pointed at the wrong location because CJS sources were transpiled to ESM output. Fixed by deferringcommonjs-format loads to Node.js (and thuspirates), which emits CJS output with an accurate inline source map.Missing field 'importAttributes'— the CJSrequire()path does not always provideconditions/importAttributes. Fixed by making those fields optional inResolveContext/LoadContext.Two further robustness fixes:
MessageChannellives only in theregister()worker entry (esm.mjs), not in the shared hooks, so it no longer pins unrelated worker threads (e.g. test runners) and prevents process exit underregisterHooks.register()fallback uses the plain resolver directly. The synchronous-loader fast paths rely on a synchronousnextResolve; under the async worker loadernextResolveis asynchronous (so the guardingtry/catchcannot catch its rejection), and the worker loader does not have the CJS interop limitations the fast paths work around.Performance
Migrating to the in-thread synchronous loader removes the per-module worker-thread IPC of
module.register(). On top of that, three optimizations were applied to the hot resolve/load path:new URL(url).pathname(~9.6× faster on that check; extension characters are never percent-encoded so it is safe), and the cheapformat === "commonjs"branch is tested first in the load hook.nextResolvefor specifiers that already carry a transformable extension (e.g../foo.ts) — these are unconditionally oxc-node's to resolve, so relative TypeScript imports are resolved once instead of twice.node_modules(URL + format already known), instead of callingnextResolvewith the resolved URL — which made Node redundantly re-readpackage.jsonandstatthe file.Benchmark — importing a 500-module TypeScript graph, median of 15 runs, Node v26.3.0:
registerHooks+ optimizations)registerworker thread)Verification
Rebuilt binding;
integrate-module(21/21),integrate-module-bundler(22/22) andintegrate-ava(9/9) all pass on Node.js 24 and 26, via both theregisterHookspath and the forcedregister()fallback. NoDEP0205warning;cargo clippyandoxlintclean.