From 6a4844875ce91f6e2690c8dc034c1ba6ea09aa28 Mon Sep 17 00:00:00 2001 From: LongYinan Date: Fri, 18 Sep 2026 14:39:27 +0800 Subject: [PATCH 1/2] 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 2/2] 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, } } {