From ab4ac78cd532de1d3592d5a7a82b5af4b8e94532 Mon Sep 17 00:00:00 2001 From: cjnoname Date: Fri, 18 Sep 2026 16:20:02 +1000 Subject: [PATCH 1/3] fix: register module hooks synchronously on Node.js >= 26.2.0 (#764) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem `packages/core/register.mjs` calls `module.register()`. Node.js runtime-deprecated that API in **v25.9.0** ([DEP0205](https://nodejs.org/api/deprecations.html#DEP0205)), so every `node --import @oxc-node/core/register` now prints: ``` (node:94305) [DEP0205] DeprecationWarning: `module.register()` is deprecated. Use `module.registerHooks()` instead. ``` This is not only cosmetic — **`pnpm test` already fails on Node.js 26** on `main`, because four tests assert an empty stderr and the warning lands there. CI runs Node 22 and 24 only, so it has stayed invisible: ``` # main, unmodified, Node v26.8.1 Test Files 2 failed | 7 passed | 1 skipped (10) Tests 4 failed | 70 passed | 7 skipped (81) FAIL __tests__/stdin-tty.spec.ts > CLI properly handles stdin piping FAIL __tests__/tsconfig-discovery.spec.ts > a broken project reference in an ancestor that does not own the file breaks nothing FAIL __tests__/tsconfig-discovery.spec.ts > tsconfig paths apply to JavaScript importers with allowJs unset FAIL __tests__/tsconfig-discovery.spec.ts > a project directory with a space and non-ASCII characters still resolves ``` All four are the same diff: `+ (node:1427) [DEP0205] DeprecationWarning: ...` ## Change Use the synchronous, in-thread `module.registerHooks()` where it is complete, and keep `module.register()` everywhere else. The ESM path is otherwise unchanged — `resolve` still hands everything to `createResolve` and `load` to `oxcLoad`, exactly as `esm.mjs` does. Three things make this more than a one-line swap. Each was measured against release binaries rather than reasoned about, and a naive replacement fails on all three. ### 1. `registerHooks()` shows `require()` to the hooks; `register()` never did The CommonJS resolve/load context carries **no `importAttributes`**, and both `createResolve` and `load` reject a context without it: ``` Error: Missing field `importAttributes` at resolve (.../register.mjs:11:12) at resolveForCJSWithHooks (node:internal/modules/cjs/loader:1217:25) ``` So a request whose `conditions` contain `require` goes to `nextResolve`/`nextLoad`. Node.js resolves those correctly by itself, because the `pirates` hook has put the TypeScript extensions into `Module._extensions` — that is what lets its CommonJS resolver complete `require('./dep')` to `./dep.ts` and `require('./sub')` to `./sub/index.ts`. ### 2. A file that oxc-node settles on as CommonJS must go back to `nextLoad` Otherwise the `pirates` hook no longer produces the inline source map, and stack trace precision regresses. `cli.spec.ts` covers exactly this: | | reported location | |---|---| | `module.register()` | `stacktrace-cjs.cts:6:12` ✅ | | naive `registerHooks()` | `stacktrace-cjs.cts:5:10` ❌ | Calling `oxcLoad` **first** and only then deferring is what keeps #759 intact — a CommonJS-reported file that actually contains ESM syntax still runs as an ES module. ### 3. `registerHooks` needs a version floor, not a `typeof` check It exists from v22.15.0/v23.5.0, but two defects had to be fixed first. Bisected over every available minor: | Node.js | ESM importing a CJS package | `require()` in an imported CJS entry | |---|---|---| | 22.14.0 (no `registerHooks`) | — fallback | — fallback | | 22.15.0 – 22.18.0 | ❌ | ❌ | | 22.19.0 – 22.22.1 | ✅ | ❌ | | 23.5.0 – 23.11.1 (EOL) | ❌ | ❌ | | 24.0.0 – 24.4.1 | ❌ | ❌ | | 24.5.0 – 25.9.0 | ✅ | ❌ | | 26.0.0 – 26.1.0 | ✅ | ❌ | | **26.2.0+** | ✅ | ✅ | Those boundaries are not arbitrary — they match the Node.js fixes exactly: - **[nodejs/node#59011](https://github.com/nodejs/node/pull/59011)** *"module: fix conditions override in synchronous resolve hooks"* — v24.5.0 (`fe0195fdcc`), backported to v22.19.0 (`0eec5cc492`), never backported to the end-of-life 23.x line. Without it, `import { jsx } from 'react/jsx-runtime'` fails with *does not provide an export named 'jsx'*. - **[nodejs/node#62920](https://github.com/nodejs/node/pull/62920)** *"module: fix sync hook short-circuit in require() in imported CJS"* — v26.2.0 (`96f19a16d0`). Without it, a `.ts` entry in a CommonJS package cannot `require()` its own files. Hence the floor is **v26.2.0**. Below it the fallback runs and nothing changes for those runtimes; v26.0/v26.1 keep the warning, which is the honest outcome given the defect. ## Verification **34 release binaries**, v22.14.0 → v26.8.2 (every available 22.x/23.x/24.x/25.x/26.x minor), against three fixtures: a CommonJS `require()` tree (extensionless `.ts`, directory `index.ts`, explicit `.cts`), an ESM tree (extensionless, `.mts`, tsconfig `paths`, `.tsx` + `react/jsx-runtime`), and a `createRequire` tree. ``` versions tested: 34 failures: 0 still warning: 26.0.0 26.1.0 ``` Upstream suite, same machine, Node v26.8.1: | | result | |---|---| | `main` | `4 failed | 70 passed` | | this PR | **`75 passed`** | `vp fmt` and `vp lint` are clean. ## Test added `require()` completing an extensionless TypeScript specifier had no coverage. The new case in `cjs-esm-syntax.spec.ts` fails with `Cannot find module './dep'` if the `pirates` hook stops registering those extensions — verified by removing it. --------- Co-authored-by: LongYinan --- .github/workflows/CI.yml | 3 + packages/core/register.mjs | 114 ++++++++++++- .../__tests__/cjs-esm-syntax.spec.ts | 21 +++ .../__tests__/register-hooks.spec.ts | 159 ++++++++++++++++++ src/lib.rs | 19 +++ 5 files changed, 312 insertions(+), 4 deletions(-) create mode 100644 packages/integrate-vitest/__tests__/register-hooks.spec.ts diff --git a/.github/workflows/CI.yml b/.github/workflows/CI.yml index cab1b3da..cca25910 100644 --- a/.github/workflows/CI.yml +++ b/.github/workflows/CI.yml @@ -262,6 +262,9 @@ jobs: node: - "22" - "24" + # The synchronous `module.registerHooks()` path only runs from v26.2.0 up, so + # without a 26 job nothing here exercises it. + - "26" transform_all: - "true" - "false" diff --git a/packages/core/register.mjs b/packages/core/register.mjs index 96585315..6292c4ac 100644 --- a/packages/core/register.mjs +++ b/packages/core/register.mjs @@ -2,10 +2,10 @@ import * as NodeModule from "node:module"; import { addHook } from "pirates"; -import { OxcTransformer } from "./index.js"; +import { OxcTransformer, createResolve, initTracing, load as oxcLoad } from "./index.js"; // Destructure from NodeModule namespace to support older Node.js versions -const { register, setSourceMapsSupport } = NodeModule; +const { register, registerHooks, setSourceMapsSupport } = NodeModule; const DEFAULT_EXTENSIONS = new Set([ ".js", @@ -20,8 +20,6 @@ const DEFAULT_EXTENSIONS = new Set([ ".es", ]); -register("@oxc-node/core/esm", import.meta.url); - if (typeof setSourceMapsSupport === "function") { setSourceMapsSupport(true, { nodeModules: true, generatedCode: true }); } else if (typeof process.setSourceMapsEnabled === "function") { @@ -49,3 +47,111 @@ addHook( ext: Array.from(DEFAULT_EXTENSIONS), }, ); + +/** + * Whether this request comes from `require()`. + * + * `module.register()` never showed `require()` to the hooks; `module.registerHooks()` + * does. Those requests stay on Node.js' own CommonJS resolution and the `pirates` hook + * above — exactly where they were before. Node.js resolves them correctly on its own: + * the `pirates` hook registers the TypeScript extensions in `Module._extensions`, which + * is what lets its CommonJS resolver complete `require('./foo')` to `./foo.ts`. + * + * The discriminator is `importAttributes`, the field that decides whether the native + * hooks can run at all: `createResolve` and `load` both take it as a required property + * and reject a context without it. A CommonJS context never carries it, an ESM context + * always does — even when empty. `conditions` cannot be used for this: `--conditions` + * appends its values to *every* request, so `--conditions=require` would send imports + * down the CommonJS path (`ERR_MODULE_NOT_FOUND` for an extensionless specifier) and + * `--conditions=import` would send `require()` down the ESM one. + * + * @param {{ importAttributes?: Record } | undefined} context + * @returns {boolean} + */ +function isCommonJsRequire(context) { + return context?.importAttributes === undefined; +} + +/** + * @type {import('node:module').ResolveHook} + */ +function resolve(specifier, context, nextResolve) { + if (isCommonJsRequire(context)) { + return nextResolve(specifier, context); + } + return createResolve( + { + getCurrentDirectory: () => process.cwd(), + }, + specifier, + context, + nextResolve, + ); +} + +/** + * @type {import('node:module').LoadHook} + */ +function load(url, context, nextLoad) { + if (isCommonJsRequire(context)) { + return nextLoad(url, context); + } + const result = oxcLoad(url, context, nextLoad); + // Anything oxc-node itself settles on as CommonJS is left to the CommonJS machinery, + // which compiles it through the `pirates` hook above and its accurate inline source + // map. Returning source from here instead costs stack trace precision: a throw in a + // `.cts` entry gets reported at the transformed position rather than the original one. + // Asking `oxcLoad` first is what keeps a CommonJS-reported file that actually contains + // ESM syntax running as an ES module. `commonjs-typescript` — Node.js' own format for a + // `.ts` file it strips types from — belongs on that same path, hence the prefix test. + if (result.format.startsWith("commonjs")) { + // A null source is what `module.register()`'s asynchronous default load returned for + // every CommonJS module, and it is the one shape that keeps `require()` inside such a + // module working on every runtime: a source-bearing result made Node.js short-circuit + // it incorrectly until https://github.com/nodejs/node/pull/62920. + return { format: result.format, source: null, responseURL: result.responseURL ?? url }; + } + return result; +} + +/** + * Whether `module.registerHooks()` can be relied on for everything this loader does. + * + * `registerHooks` itself landed in v22.15.0 and v23.5.0, but two defects kept it from + * being a drop-in replacement for far longer, both verified against release binaries: + * + * - Until https://github.com/nodejs/node/pull/59011 a synchronous resolve hook had its + * `conditions` overridden, which breaks CommonJS named-export detection for a package + * imported from ESM: `import { jsx } from 'react/jsx-runtime'` fails with "does not + * provide an export named 'jsx'". Fixed in v24.5.0, backported to v22.19.0, and never + * backported to the end-of-life 23.x line. + * - Until https://github.com/nodejs/node/pull/62920 `require()` inside an imported + * CommonJS module short-circuited incorrectly whenever a synchronous load hook handed + * back source for it, so a `.ts` entry point in a CommonJS package could not + * `require()` its own files. Fixed in v26.2.0. The `load` hook above never returns + * source for CommonJS, so this defect does not reach it — v26.2.0 is kept as the floor + * anyway, because it is the first release where the synchronous hooks are complete + * regardless of what a hook returns, and every runtime below it keeps exactly the + * behaviour it has today. + * + * Below v26.2.0 `module.register()` therefore stays in use, deprecation warning included. + * + * @returns {boolean} + */ +function canRegisterSyncHooks() { + if (typeof registerHooks !== "function") { + return false; + } + const [major, minor] = process.versions.node.split(".", 2).map(Number); + return major > 26 || (major === 26 && minor >= 2); +} + +// `module.register()` is deprecated — DEP0205, runtime-deprecated since v25.9.0 — and +// runs the hooks on a separate thread. Prefer the synchronous, in-thread +// `module.registerHooks()` on every runtime that implements it completely. +if (canRegisterSyncHooks()) { + initTracing(); + registerHooks({ load, resolve }); +} else { + register("@oxc-node/core/esm", import.meta.url); +} diff --git a/packages/integrate-vitest/__tests__/cjs-esm-syntax.spec.ts b/packages/integrate-vitest/__tests__/cjs-esm-syntax.spec.ts index 86c8ee26..3af7f8a8 100644 --- a/packages/integrate-vitest/__tests__/cjs-esm-syntax.spec.ts +++ b/packages/integrate-vitest/__tests__/cjs-esm-syntax.spec.ts @@ -237,6 +237,27 @@ describe("a CommonJS package", () => { expect(run(root, "./entry.ts")).toContain("type-only: true true"); }); + test("`require()` completes an extensionless TypeScript specifier", () => { + // `module.registerHooks()` routes `require()` through the resolve hook, where + // `nextResolve` is Node.js' CommonJS resolver — and that resolver only completes + // `./dep` to `./dep.ts`, or `./sub` to `./sub/index.ts`, for extensions present in + // `Module._extensions`. The `pirates` hook is what puts them there, so dropping it + // makes both requires below fail with MODULE_NOT_FOUND, which nothing else here + // would catch. + const root = fixture({ + "package.json": COMMONJS, + // Type annotations, so the files cannot run at all unless they were transformed. + "dep.ts": 'const value: string = "dep-ok";\nexports.dep = value;\n', + "sub/index.ts": 'const value: string = "sub-ok";\nexports.sub = value;\n', + "entry.ts": [ + 'const { dep } = require("./dep");', + 'const { sub } = require("./sub");', + 'console.log("require:", dep, sub);', + ].join("\n"), + }); + expect(run(root, "./entry.ts")).toContain("require: dep-ok sub-ok"); + }); + test("a .cts file is CommonJS by contract and never flips", () => { const root = fixture({ "package.json": COMMONJS, diff --git a/packages/integrate-vitest/__tests__/register-hooks.spec.ts b/packages/integrate-vitest/__tests__/register-hooks.spec.ts new file mode 100644 index 00000000..63627867 --- /dev/null +++ b/packages/integrate-vitest/__tests__/register-hooks.spec.ts @@ -0,0 +1,159 @@ +import { spawnSync } from "node:child_process"; +import { + mkdirSync, + mkdtempSync, + readFileSync, + readdirSync, + rmSync, + symlinkSync, + writeFileSync, +} from "node:fs"; +import { tmpdir } from "node:os"; +import { dirname, join } from "node:path"; +import { fileURLToPath } from "node:url"; +import { afterAll, describe, expect, test } from "vitest"; + +/** + * `module.registerHooks()` runs the hooks in-thread and shows them requests that + * `module.register()` never did, so two things have to hold that the asynchronous hooks + * got for free: + * + * - its default load is synchronous and always reads the file, so a `commonjs` result + * arrives with its bytes attached — even for a `.node` addon or a latin-1 file, where + * the asynchronous default load returned no source and left the read to the CommonJS + * machinery. Bytes that are not UTF-8 are not source code to transform, so they are + * handed back the same way instead of failing to decode. + * - `require()` reaches the hooks, and it is the absent `importAttributes` that tells + * such a request apart — the field the native hooks require anyway. The `require` + * export condition cannot: `--conditions` appends its values to *every* request, so it + * appears on imports too. + */ + +const CORE = fileURLToPath(new URL("../../core", import.meta.url)); +const COMMONJS = JSON.stringify({ name: "fx", private: true, type: "commonjs" }); + +const roots: string[] = []; + +afterAll(() => { + for (const root of roots) { + rmSync(root, { force: true, recursive: true }); + } +}); + +function fixture(files: Record): string { + const root = mkdtempSync(join(tmpdir(), "oxc-node-hooks-")); + roots.push(root); + for (const [name, contents] of Object.entries(files)) { + const path = join(root, name); + mkdirSync(dirname(path), { recursive: true }); + writeFileSync(path, contents); + } + mkdirSync(join(root, "node_modules", "@oxc-node"), { recursive: true }); + symlinkSync( + CORE, + join(root, "node_modules", "@oxc-node", "core"), + // `junction` is the only link type Windows allows without elevated privileges. + process.platform === "win32" ? "junction" : "dir", + ); + return root; +} + +function run(root: string, args: string[]): string { + const result = spawnSync( + process.execPath, + // A bare specifier resolves through the symlinked node_modules on every platform, + // Windows included — an absolute path would not parse as a URL. + ["--import", "@oxc-node/core/register", ...args], + { + cwd: root, + encoding: "utf8", + env: { + ...process.env, + NODE_OPTIONS: undefined, + OXC_LOG: undefined, + TS_NODE_PROJECT: undefined, + OXC_TSCONFIG_PATH: undefined, + }, + timeout: 30_000, + }, + ); + const output = `${result.stdout}${result.stderr}`; + expect(result.error, result.error?.message).toBeFalsy(); + expect(result.status, output).toBe(0); + return output; +} + +describe("a source that is not UTF-8", () => { + // The loader's own addon is the one binary that is guaranteed to be loadable by the + // Node.js running these tests; a WASI build of the package has none. + const addon = readdirSync(CORE).find((name) => name.endsWith(".node")); + + test.skipIf(addon === undefined)("an imported `.node` addon still loads", () => { + // `.node` resolves as `commonjs`, so the addon arrives at the load hook as a binary + // blob. Transforming it is not possible and not needed: `process.dlopen` reads the + // file itself once the CommonJS machinery takes over. + const root = fixture({ + "package.json": COMMONJS, + "addon.node": readFileSync(join(CORE, addon!)), + "entry.mts": [ + 'import addon from "./addon.node";', + 'console.log("addon:", typeof addon.transform);', + ].join("\n"), + }); + expect(run(root, ["./entry.mts"])).toContain("addon: function"); + }); + + test("an imported latin-1 CommonJS file still loads", () => { + const root = fixture({ + "package.json": COMMONJS, + // `café` in latin-1: the trailing `0xe9` is not valid UTF-8, and Node.js decodes it + // to a single replacement character — a four character string either way. + "legacy.js": Uint8Array.from([ + ...Buffer.from('module.exports = "caf', "utf8"), + 0xe9, + ...Buffer.from('";\n', "utf8"), + ]), + "entry.mts": [ + 'import legacy from "./legacy.js";', + 'console.log("latin1:", legacy.length);', + ].join("\n"), + }); + expect(run(root, ["./entry.mts"])).toContain("latin1: 4"); + }); +}); + +// `--conditions` adds to the condition set of every request, so neither `require` nor +// `import` says anything about which loader asked. Both directions have to keep working +// with either one of them injected. +describe.each(["require", "import"])("with --conditions=%s", (condition) => { + const flag = `--conditions=${condition}`; + + test("an import is still resolved by oxc-node", () => { + // Only oxc-node's resolver completes an extensionless specifier for an `import`; + // Node.js' ESM resolver reports ERR_MODULE_NOT_FOUND. + const root = fixture({ + "package.json": COMMONJS, + "dep.ts": 'const value: string = "dep-ok";\nexport default value;\n', + "entry.mts": ['import dep from "./dep";', 'console.log("import:", dep);'].join("\n"), + }); + expect(run(root, [flag, "./entry.mts"])).toContain("import: dep-ok"); + }); + + test("an imported CommonJS-reported file with ESM syntax still flips to module", () => { + const root = fixture({ + "package.json": COMMONJS, + "flip.ts": 'export const flip = "flip-ok";\n', + "entry.mts": ['import { flip } from "./flip.ts";', 'console.log("flip:", flip);'].join("\n"), + }); + expect(run(root, [flag, "./entry.mts"])).toContain("flip: flip-ok"); + }); + + test("`require()` is still resolved by Node.js", () => { + const root = fixture({ + "package.json": COMMONJS, + "dep.ts": 'const value: string = "dep-ok";\nexports.dep = value;\n', + "entry.ts": ['const { dep } = require("./dep");', 'console.log("require:", dep);'].join("\n"), + }); + expect(run(root, [flag, "./entry.ts"])).toContain("require: dep-ok"); + }); +}); diff --git a/src/lib.rs b/src/lib.rs index 68df941f..3b2c1eb2 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1203,6 +1203,25 @@ fn transform_output( let ext = src_path.extension().and_then(|ext| ext.to_str()); let is_json = ext.is_some_and(|ext| ext.eq_ignore_ascii_case("json")); + // A CommonJS result whose source is not valid UTF-8 is not source code this + // loader can transform: a `.node` addon resolves as `commonjs` and arrives as + // a binary, and a legacy CommonJS file may be latin-1. Node.js' asynchronous + // default load never handed those over — it returns no source for `commonjs` + // and lets the CommonJS machinery read the file — but the synchronous + // `defaultLoadSync` behind `module.registerHooks()` always reads it. Drop the + // bytes and defer the same way instead of failing to decode them. + if !is_json + && output.format.starts_with("commonjs") + && output.source.as_ref().unwrap().try_as_str().is_err() + { + tracing::debug!("Not UTF-8, deferring to the CommonJS loader {}", url); + return Ok(LoadFnOutput { + format: output.format, + source: None, + response_url: Some(url), + }); + } + // Turning JSON into a module is not a code transform, so it happens for // dependencies too — `OXC_TRANSFORM_ALL` decides whether their *source* is // transpiled, and skipping this would hand Node.js raw JSON to run as an ES From 6a4844875ce91f6e2690c8dc034c1ba6ea09aa28 Mon Sep 17 00:00:00 2001 From: LongYinan Date: Fri, 18 Sep 2026 14:39:27 +0800 Subject: [PATCH 2/3] fix: hand Node.js' null source back as null and keep commonjs-typescript off the deferral Node.js' `addon` translator asserts `source === null` for a `.node` file under node_modules, but `transform_output` dropped the `Option` and the result reached JavaScript as `undefined`, on both the asynchronous and the synchronous hooks. The `startsWith("commonjs")` deferral also caught `commonjs-typescript`, whose translator needs the source, turning Node's own type-stripping error into one blaming the hook. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- packages/core/register.mjs | 7 ++- .../__tests__/register-hooks.spec.ts | 56 ++++++++++++++++++- src/lib.rs | 14 ++++- 3 files changed, 70 insertions(+), 7 deletions(-) diff --git a/packages/core/register.mjs b/packages/core/register.mjs index 6292c4ac..6c7f6931 100644 --- a/packages/core/register.mjs +++ b/packages/core/register.mjs @@ -103,8 +103,11 @@ function load(url, context, nextLoad) { // `.cts` entry gets reported at the transformed position rather than the original one. // Asking `oxcLoad` first is what keeps a CommonJS-reported file that actually contains // ESM syntax running as an ES module. `commonjs-typescript` — Node.js' own format for a - // `.ts` file it strips types from — belongs on that same path, hence the prefix test. - if (result.format.startsWith("commonjs")) { + // `.ts` file it strips types from — is not deferred, because that translator needs the + // source and rejects `null`; it is passed through untouched so Node.js reports its own + // error (`ERR_UNSUPPORTED_NODE_MODULES_TYPE_STRIPPING`) for a `.ts` dependency instead + // of one blaming the hook. + if (result.format === "commonjs") { // A null source is what `module.register()`'s asynchronous default load returned for // every CommonJS module, and it is the one shape that keeps `require()` inside such a // module working on every runtime: a source-bearing result made Node.js short-circuit diff --git a/packages/integrate-vitest/__tests__/register-hooks.spec.ts b/packages/integrate-vitest/__tests__/register-hooks.spec.ts index 63627867..1ecd5810 100644 --- a/packages/integrate-vitest/__tests__/register-hooks.spec.ts +++ b/packages/integrate-vitest/__tests__/register-hooks.spec.ts @@ -58,7 +58,7 @@ function fixture(files: Record): string { return root; } -function run(root: string, args: string[]): string { +function spawn(root: string, args: string[]): { status: number | null; output: string } { const result = spawnSync( process.execPath, // A bare specifier resolves through the symlinked node_modules on every platform, @@ -79,7 +79,12 @@ function run(root: string, args: string[]): string { ); const output = `${result.stdout}${result.stderr}`; expect(result.error, result.error?.message).toBeFalsy(); - expect(result.status, output).toBe(0); + return { status: result.status, output }; +} + +function run(root: string, args: string[]): string { + const { status, output } = spawn(root, args); + expect(status, output).toBe(0); return output; } @@ -122,6 +127,53 @@ describe("a source that is not UTF-8", () => { }); }); +describe("a source Node.js hands over without bytes", () => { + // Not specific to the synchronous hooks — the asynchronous default load returned the + // same `null` — but it lives here for the fixture helpers. A `.node` addon under + // node_modules resolves without a format, so Node.js decides `addon` and hands the load + // hook `source: null`. Its `addon` translator asserts exactly `null` on the way back: + // an `undefined`, which is what a dropped `Option` serialised to, fails with + // ERR_INVALID_RETURN_PROPERTY_VALUE. + const addon = readdirSync(CORE).find((name) => name.endsWith(".node")); + + test.skipIf(addon === undefined)("an addon imported from a dependency still loads", () => { + const root = fixture({ + "package.json": COMMONJS, + "node_modules/addon-dep/package.json": JSON.stringify({ + name: "addon-dep", + main: "addon.node", + }), + "node_modules/addon-dep/addon.node": readFileSync(join(CORE, addon!)), + "entry.mts": [ + 'import addon from "addon-dep";', + 'console.log("addon:", typeof addon.transform);', + ].join("\n"), + }); + // Unflagged on v26; v22 and v24 need the flag, and v26 still accepts it. + expect(run(root, ["--experimental-addon-modules", "./entry.mts"])).toContain("addon: function"); + }); + + test("a TypeScript dependency gets Node.js' own error, not one blaming the hook", () => { + // `.cts` under node_modules is `commonjs-typescript` to Node.js, a format whose + // translator needs the source: deferring it with `source: null` like plain `commonjs` + // is an invalid return shape. Node.js refuses type stripping in node_modules on every + // path, so what has to hold is that *its* error is the one reported. + const root = fixture({ + "package.json": COMMONJS, + "node_modules/ts-dep/package.json": JSON.stringify({ + name: "ts-dep", + exports: "./index.cts", + }), + "node_modules/ts-dep/index.cts": "const c: number = 3;\nexport { c };\n", + "entry.mts": ['import { c } from "ts-dep";', 'console.log("cts:", c);'].join("\n"), + }); + const { status, output } = spawn(root, ["./entry.mts"]); + expect(status, output).not.toBe(0); + expect(output).toContain("ERR_UNSUPPORTED_NODE_MODULES_TYPE_STRIPPING"); + expect(output).not.toContain("ERR_INVALID_RETURN_PROPERTY_VALUE"); + }); +}); + // `--conditions` adds to the condition set of every request, so neither `require` nor // `import` says anything about which loader asked. Both directions have to keep working // with either one of them injected. diff --git a/src/lib.rs b/src/lib.rs index 3b2c1eb2..16acc6cb 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1194,7 +1194,13 @@ fn transform_output( return Ok(loaded); } tracing::debug!("No source code to transform {}", url); - Ok(LoadFnOutput { format: output.format, source: None, response_url: Some(url) }) + // The `null` Node.js handed over is handed back as-is: its `addon` + // translator asserts `source === null`, and an `undefined` fails it. + Ok(LoadFnOutput { + format: output.format, + source: Some(Either4::D(Null)), + response_url: Some(url), + }) } Some(Either4::A(_) | Either4::B(_) | Either4::C(_)) => { // `url` is a URL, so a `?query` or `#fragment` has to be stripped before it can @@ -1210,14 +1216,16 @@ fn transform_output( // and lets the CommonJS machinery read the file — but the synchronous // `defaultLoadSync` behind `module.registerHooks()` always reads it. Drop the // bytes and defer the same way instead of failing to decode them. + // `commonjs-typescript` is not deferred: Node.js' type-stripping translator + // needs the source, and handing it `null` is an invalid return shape. if !is_json - && output.format.starts_with("commonjs") + && output.format == "commonjs" && output.source.as_ref().unwrap().try_as_str().is_err() { tracing::debug!("Not UTF-8, deferring to the CommonJS loader {}", url); return Ok(LoadFnOutput { format: output.format, - source: None, + source: Some(Either4::D(Null)), response_url: Some(url), }); } From f8c9b60a28d455fcea0d4266c4ee026064a50ad2 Mon Sep 17 00:00:00 2001 From: LongYinan Date: Fri, 18 Sep 2026 14:58:03 +0800 Subject: [PATCH 3/3] test: cover the dependency and addon edge cases of the load hook on every matrix runtime MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Node.js hands the load hook `format: undefined` when the resolve chain reported none — a `.node` under node_modules without its flag — and the required `LoadContext.format` rejected the whole context with "Missing field `format`" where Node.js would have raised ERR_UNKNOWN_FILE_EXTENSION. Make it optional so Node.js' own error is what shows. The dependency cases (addon with and without the flag, latin-1, `.cts`, `.mts`) now run under both OXC_TRANSFORM_ALL values in-spec, with the addon default keyed on the runtime (on from v24.19.0 / v26.5.0), and the Linux matrix gains Node.js 26 so the synchronous hooks are exercised there as well as on macOS and Windows. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- .github/workflows/CI.yml | 6 + packages/core/index.d.ts | 10 +- .../__tests__/register-hooks.spec.ts | 180 ++++++++++++++---- src/lib.rs | 12 +- 4 files changed, 166 insertions(+), 42 deletions(-) diff --git a/.github/workflows/CI.yml b/.github/workflows/CI.yml index cca25910..d86c31cd 100644 --- a/.github/workflows/CI.yml +++ b/.github/workflows/CI.yml @@ -320,9 +320,15 @@ jobs: node: - "22" - "24" + # The synchronous `module.registerHooks()` path only runs from v26.2.0 up, so + # without a 26 job no Linux target exercises it. + - "26" exclude: - target: armv7-unknown-linux-gnueabihf node: "24" + # `node:26-slim` ships no arm/v7 image either. + - target: armv7-unknown-linux-gnueabihf + node: "26" # Node.js on qemu segfaults on s390x and arm64v8 when using 24.04 # See also https://github.com/actions/runner-images/issues/11471 runs-on: ${{ contains(matrix.target, 'aarch64') && 'ubuntu-24.04-arm' || 'ubuntu-24.04' }} diff --git a/packages/core/index.d.ts b/packages/core/index.d.ts index 4db84e57..41c27219 100644 --- a/packages/core/index.d.ts +++ b/packages/core/index.d.ts @@ -29,8 +29,14 @@ export declare function load(url: string, context: LoadContext, nextLoad: (arg0: export interface LoadContext { /** Export conditions of the relevant `package.json` */ conditions?: Array - /** The format optionally supplied by the `resolve` hook chain */ - format: string | null + /** + * The format optionally supplied by the `resolve` hook chain. Node.js passes it as + * `undefined`, not `null`, when the chain reported none — a `.node` or `.wasm` file + * resolved without its flag, any extension Node.js does not know — and a required + * field would reject the whole context with "Missing field `format`" instead of + * letting Node.js raise its own `ERR_UNKNOWN_FILE_EXTENSION`. + */ + format?: string | null /** An object whose key-value pairs represent the assertions for the module to import */ importAttributes: Record } diff --git a/packages/integrate-vitest/__tests__/register-hooks.spec.ts b/packages/integrate-vitest/__tests__/register-hooks.spec.ts index 1ecd5810..53e64196 100644 --- a/packages/integrate-vitest/__tests__/register-hooks.spec.ts +++ b/packages/integrate-vitest/__tests__/register-hooks.spec.ts @@ -32,6 +32,32 @@ import { afterAll, describe, expect, test } from "vitest"; const CORE = fileURLToPath(new URL("../../core", import.meta.url)); const COMMONJS = JSON.stringify({ name: "fx", private: true, type: "commonjs" }); +// The loader's own addon is the one binary that is guaranteed to be loadable by the +// Node.js running these tests; a WASI build of the package has none. +const addon = readdirSync(CORE).find((name) => name.endsWith(".node")); +const ADDON_ENTRY = [ + 'import addon from "./addon.node";', + 'console.log("addon:", typeof addon.transform);', +].join("\n"); +const ADDON_DEP_ENTRY = [ + 'import addon from "addon-dep";', + 'console.log("addon:", typeof addon.transform);', +].join("\n"); +// `café` in latin-1: the trailing `0xe9` is not valid UTF-8, and Node.js decodes it to a +// single replacement character — a four character string either way. +const LATIN1_SOURCE = Uint8Array.from([ + ...Buffer.from('module.exports = "caf', "utf8"), + 0xe9, + ...Buffer.from('";\n', "utf8"), +]); + +const [nodeMajor, nodeMinor] = process.versions.node.split(".", 2).map(Number); +// `import` of a `.node` addon is on by default from v24.19.0 and v26.5.0; on the 22 line +// the flag exists (v22.20.0) but stays opt-in. 23 and 25 are end-of-life and not in the +// CI matrix, so they are not modelled. +const addonImportsByDefault = + (nodeMajor === 24 && nodeMinor >= 19) || nodeMajor > 26 || (nodeMajor === 26 && nodeMinor >= 5); + const roots: string[] = []; afterAll(() => { @@ -58,7 +84,11 @@ function fixture(files: Record): string { return root; } -function spawn(root: string, args: string[]): { status: number | null; output: string } { +function spawn( + root: string, + args: string[], + env: NodeJS.ProcessEnv = {}, +): { status: number | null; output: string } { const result = spawnSync( process.execPath, // A bare specifier resolves through the symlinked node_modules on every platform, @@ -73,6 +103,7 @@ function spawn(root: string, args: string[]): { status: number | null; output: s OXC_LOG: undefined, TS_NODE_PROJECT: undefined, OXC_TSCONFIG_PATH: undefined, + ...env, }, timeout: 30_000, }, @@ -82,17 +113,23 @@ function spawn(root: string, args: string[]): { status: number | null; output: s return { status: result.status, output }; } -function run(root: string, args: string[]): string { - const { status, output } = spawn(root, args); +function run(root: string, args: string[], env: NodeJS.ProcessEnv = {}): string { + const { status, output } = spawn(root, args, env); expect(status, output).toBe(0); return output; } -describe("a source that is not UTF-8", () => { - // The loader's own addon is the one binary that is guaranteed to be loadable by the - // Node.js running these tests; a WASI build of the package has none. - const addon = readdirSync(CORE).find((name) => name.endsWith(".node")); +/** Runs a fixture that must fail, and must fail with Node.js' own error rather than one + * blaming the load hook's return shape. */ +function runFailing(root: string, args: string[], code: string, env: NodeJS.ProcessEnv = {}) { + const { status, output } = spawn(root, args, env); + expect(status, output).not.toBe(0); + expect(output).toContain(code); + expect(output).not.toContain("ERR_INVALID_RETURN_PROPERTY_VALUE"); + expect(output).not.toContain("Missing field"); +} +describe("a local source that is not UTF-8", () => { test.skipIf(addon === undefined)("an imported `.node` addon still loads", () => { // `.node` resolves as `commonjs`, so the addon arrives at the load hook as a binary // blob. Transforming it is not possible and not needed: `process.dlopen` reads the @@ -100,24 +137,46 @@ describe("a source that is not UTF-8", () => { const root = fixture({ "package.json": COMMONJS, "addon.node": readFileSync(join(CORE, addon!)), - "entry.mts": [ - 'import addon from "./addon.node";', + "entry.mts": ADDON_ENTRY, + }); + expect(run(root, ["./entry.mts"])).toContain("addon: function"); + }); + + test.skipIf(addon === undefined)( + "an imported `.node` addon still loads with --experimental-addon-modules", + () => { + // The flag decides the format Node.js reports for a `.node` file, not the one + // oxc-node does: a local addon is still `commonjs` and still reaches `dlopen` + // through the CommonJS machinery rather than Node.js' `addon` translator. + const root = fixture({ + "package.json": COMMONJS, + "addon.node": readFileSync(join(CORE, addon!)), + "entry.mts": ADDON_ENTRY, + }); + expect(run(root, ["--experimental-addon-modules", "./entry.mts"])).toContain( + "addon: function", + ); + }, + ); + + test.skipIf(addon === undefined)("a `require()`d `.node` addon still loads", () => { + // `require()` reaches the synchronous hooks and is handed straight back to Node.js; + // `module.register()` never showed it to the hooks at all. + const root = fixture({ + "package.json": COMMONJS, + "addon.node": readFileSync(join(CORE, addon!)), + "entry.cts": [ + 'const addon = require("./addon.node");', 'console.log("addon:", typeof addon.transform);', ].join("\n"), }); - expect(run(root, ["./entry.mts"])).toContain("addon: function"); + expect(run(root, ["./entry.cts"])).toContain("addon: function"); }); test("an imported latin-1 CommonJS file still loads", () => { const root = fixture({ "package.json": COMMONJS, - // `café` in latin-1: the trailing `0xe9` is not valid UTF-8, and Node.js decodes it - // to a single replacement character — a four character string either way. - "legacy.js": Uint8Array.from([ - ...Buffer.from('module.exports = "caf', "utf8"), - 0xe9, - ...Buffer.from('";\n', "utf8"), - ]), + "legacy.js": LATIN1_SOURCE, "entry.mts": [ 'import legacy from "./legacy.js";', 'console.log("latin1:", legacy.length);', @@ -127,33 +186,68 @@ describe("a source that is not UTF-8", () => { }); }); -describe("a source Node.js hands over without bytes", () => { - // Not specific to the synchronous hooks — the asynchronous default load returned the - // same `null` — but it lives here for the fixture helpers. A `.node` addon under - // node_modules resolves without a format, so Node.js decides `addon` and hands the load - // hook `source: null`. Its `addon` translator asserts exactly `null` on the way back: - // an `undefined`, which is what a dropped `Option` serialised to, fails with - // ERR_INVALID_RETURN_PROPERTY_VALUE. - const addon = readdirSync(CORE).find((name) => name.endsWith(".node")); - - test.skipIf(addon === undefined)("an addon imported from a dependency still loads", () => { - const root = fixture({ +// A dependency is resolved by oxc-node but its format is left to Node.js, so these are +// the cases where Node.js' own formats — `addon`, `commonjs-typescript`, +// `module-typescript`, or none at all — reach the load hook. The CI matrix runs the suite +// under both `OXC_TRANSFORM_ALL` values; for a dependency that setting decides whether +// `transform_output` transforms it or hands it back untouched, so these run under both +// regardless of what the job set. +describe.each(["false", "true"])("a dependency, OXC_TRANSFORM_ALL=%s", (transformAll) => { + const env = { OXC_TRANSFORM_ALL: transformAll }; + const addonDep = () => + fixture({ "package.json": COMMONJS, "node_modules/addon-dep/package.json": JSON.stringify({ name: "addon-dep", main: "addon.node", }), "node_modules/addon-dep/addon.node": readFileSync(join(CORE, addon!)), + "entry.mts": ADDON_DEP_ENTRY, + }); + + test.skipIf(addon === undefined)("an addon still loads with --experimental-addon-modules", () => { + // Not specific to the synchronous hooks — the asynchronous default load returned the + // same `null`. Node.js decides `addon` and hands the load hook `source: null`; its + // `addon` translator asserts exactly `null` on the way back, and an `undefined`, which + // is what a dropped `Option` serialised to, fails with ERR_INVALID_RETURN_PROPERTY_VALUE. + expect(run(addonDep(), ["--experimental-addon-modules", "./entry.mts"], env)).toContain( + "addon: function", + ); + }); + + test.skipIf(addon === undefined || nodeMajor === 23 || nodeMajor === 25)( + "an addon follows Node.js' own default for the flag", + () => { + // With the flag off Node.js reports no format for the file — `format: undefined`, + // which the load context has to accept — so the load hook short-circuits to + // `nextLoad` and Node.js' own error is what shows, exactly as without oxc-node. + if (addonImportsByDefault) { + expect(run(addonDep(), ["./entry.mts"], env)).toContain("addon: function"); + } else { + runFailing(addonDep(), ["./entry.mts"], "ERR_UNKNOWN_FILE_EXTENSION", env); + } + }, + ); + + test("a latin-1 CommonJS file still loads", () => { + // The UTF-8 check in `transform_output` runs before the node_modules skip, so a + // dependency is deferred to the CommonJS machinery the same way a local file is. + const root = fixture({ + "package.json": COMMONJS, + "node_modules/latin-dep/package.json": JSON.stringify({ + name: "latin-dep", + main: "index.js", + }), + "node_modules/latin-dep/index.js": LATIN1_SOURCE, "entry.mts": [ - 'import addon from "addon-dep";', - 'console.log("addon:", typeof addon.transform);', + 'import legacy from "latin-dep";', + 'console.log("latin1:", legacy.length);', ].join("\n"), }); - // Unflagged on v26; v22 and v24 need the flag, and v26 still accepts it. - expect(run(root, ["--experimental-addon-modules", "./entry.mts"])).toContain("addon: function"); + expect(run(root, ["./entry.mts"], env)).toContain("latin1: 4"); }); - test("a TypeScript dependency gets Node.js' own error, not one blaming the hook", () => { + test("a `.cts` file gets Node.js' own type-stripping error, not one blaming the hook", () => { // `.cts` under node_modules is `commonjs-typescript` to Node.js, a format whose // translator needs the source: deferring it with `source: null` like plain `commonjs` // is an invalid return shape. Node.js refuses type stripping in node_modules on every @@ -167,10 +261,22 @@ describe("a source Node.js hands over without bytes", () => { "node_modules/ts-dep/index.cts": "const c: number = 3;\nexport { c };\n", "entry.mts": ['import { c } from "ts-dep";', 'console.log("cts:", c);'].join("\n"), }); - const { status, output } = spawn(root, ["./entry.mts"]); - expect(status, output).not.toBe(0); - expect(output).toContain("ERR_UNSUPPORTED_NODE_MODULES_TYPE_STRIPPING"); - expect(output).not.toContain("ERR_INVALID_RETURN_PROPERTY_VALUE"); + runFailing(root, ["./entry.mts"], "ERR_UNSUPPORTED_NODE_MODULES_TYPE_STRIPPING", env); + }); + + test("an `.mts` file gets Node.js' own type-stripping error, not one blaming the hook", () => { + // `module-typescript` is the sibling of `commonjs-typescript`; it is never deferred + // and has to stay passed through with its source. + const root = fixture({ + "package.json": COMMONJS, + "node_modules/mts-dep/package.json": JSON.stringify({ + name: "mts-dep", + exports: "./index.mts", + }), + "node_modules/mts-dep/index.mts": "export const c: number = 3;\n", + "entry.mts": ['import { c } from "mts-dep";', 'console.log("mts:", c);'].join("\n"), + }); + runFailing(root, ["./entry.mts"], "ERR_UNSUPPORTED_NODE_MODULES_TYPE_STRIPPING", env); }); }); diff --git a/src/lib.rs b/src/lib.rs index 16acc6cb..ca7595a0 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1027,8 +1027,12 @@ fn json_format(context: &ResolveContext) -> &'static str { pub struct LoadContext { /// Export conditions of the relevant `package.json` pub conditions: Option>, - /// The format optionally supplied by the `resolve` hook chain - pub format: Either, + /// The format optionally supplied by the `resolve` hook chain. Node.js passes it as + /// `undefined`, not `null`, when the chain reported none — a `.node` or `.wasm` file + /// resolved without its flag, any extension Node.js does not know — and a required + /// field would reject the whole context with "Missing field `format`" instead of + /// letting Node.js raise its own `ERR_UNKNOWN_FILE_EXTENSION`. + pub format: Option>, /// An object whose key-value pairs represent the assertions for the module to import pub import_attributes: HashMap, } @@ -1055,7 +1059,9 @@ pub fn load<'env>( tracing::debug!(url = ?url, context = ?context, "load"); if url.starts_with("data:") || { match context.format { - Either::A(ref format) => format == "builtin" || format == "json" || format == "wasm", + Some(Either::A(ref format)) => { + format == "builtin" || format == "json" || format == "wasm" + } _ => true, } } {