fix: hand Node.js' null source back as null; keep commonjs-typescript off the deferral - #1
Closed
Brooooooklyn wants to merge 3 commits into
Conversation
## 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](nodejs/node#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](nodejs/node#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 <lynweklm@gmail.com>
Author
|
Pushed a second commit with edge-case coverage and one more fix it flushed out:
Verified: 95/95 full suite on 22.23.2 / 24.11.1 / 26.9.0; spec alone additionally on 24.21.0 (the default-on branch on the 24 line) and with an outer |
…ipt 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>
…very matrix runtime 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>
Brooooooklyn
force-pushed
the
fix/register-hooks-sync-followup
branch
from
September 18, 2026 07:01
ba923c2 to
f8c9b60
Compare
Author
|
Superseded: oxc-project#764 landed on main and the follow-ups are now oxc-project#767 rebased on top of it. |
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.
Follow-up for oxc-project#764 — the two remaining notes from the review, as a PR into your branch since "allow edits by maintainers" is off.
Summary
transform_output: thenullsource Node.js hands over (addonfor a.nodeunder node_modules) now goes back asnull, not a droppedOption→undefined, whichtranslateAddonrejects. Pre-existing on both hook paths.register.mjs+ the non-UTF-8 guard: exactcommonjsmatch instead ofstartsWith, socommonjs-typescriptkeeps its source and Node.js reportsERR_UNSUPPORTED_NODE_MODULES_TYPE_STRIPPINGitself instead ofERR_INVALID_RETURN_PROPERTY_VALUE.register-hooks.spec.ts(spawnhelper split out ofrunfor the failing-exit case).Test plan
register-hooks.spec.tsalone on 22.23.2 / 24.11.1 / 26.9.0, and 26 withOXC_TRANSFORM_ALL=truecargo clippy -D warnings,cargo fmt --check,vp checkGenerated with Devin