module: handle null source from async loader hooks in sync hooks by joyeecheung · Pull Request #59929 · nodejs/node

joyeecheung · 2025-09-18T17:47:34Z

This relaxes the validation in sync hooks so that it accepts the quirky nullish source returned by the default step of the async loader when the module being loaded is CommonJS. When there are no customization hooks registered, a saner synchronous default load step is used to use a property instead of a reset nullish source to signify that the module should go through the CJS monkey patching routes and reduce excessive reloading from disk.

Fixes: #59384
Fixes: #57327
Refs: #59666
Refs: https://github.com/dygabo/load_module_test

nodejs-github-bot · 2025-09-18T17:47:39Z

Review requested:

@nodejs/loaders

codecov · 2025-09-18T18:52:40Z

Codecov Report

❌ Patch coverage is 99.34641% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 88.55%. Comparing base (f0aa073) to head (790b5f4).
⚠️ Report is 41 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/modules/esm/loader.js	97.14%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #59929   +/-   ##
=======================================
  Coverage   88.55%   88.55%           
=======================================
  Files         704      704           
  Lines      208081   208127   +46     
  Branches    40004    40022   +18     
=======================================
+ Hits       184265   184310   +45     
- Misses      15809    15827   +18     
+ Partials     8007     7990   -17

Files with missing lines	Coverage Δ
lib/internal/modules/cjs/loader.js	`97.97% <100.00%> (+0.04%)`	⬆️
lib/internal/modules/customization_hooks.js	`100.00% <100.00%> (ø)`
lib/internal/modules/esm/hooks.js	`90.68% <100.00%> (ø)`
lib/internal/modules/esm/load.js	`93.27% <100.00%> (-1.04%)`	⬇️
lib/internal/modules/esm/translators.js	`92.90% <100.00%> (+1.06%)`	⬆️
lib/internal/modules/esm/loader.js	`96.18% <97.14%> (+0.03%)`	⬆️

... and 35 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

nodejs-github-bot · 2025-09-22T11:42:44Z

CI: https://ci.nodejs.org/job/node-test-pull-request/69322/

joyeecheung · 2025-09-22T14:54:21Z

This is necessary to fix known issues with sync/async loader hook interop. Can I get some reviews please? @nodejs/loaders

GeoffreyBooth

Thank you, I've always disliked this “null means this“ pattern and at least this makes it clearer and more explicit. Maybe when we remove the async hooks we can get rid of this pattern.

nodejs-github-bot · 2025-09-23T18:29:11Z

CI: https://ci.nodejs.org/job/node-test-pull-request/69340/

nodejs-github-bot · 2025-09-24T11:51:24Z

CI: https://ci.nodejs.org/job/node-test-pull-request/69349/

nodejs-github-bot · 2025-09-29T14:45:32Z

CI: https://ci.nodejs.org/job/node-test-pull-request/69440/

nodejs-github-bot · 2025-09-29T18:42:53Z

CI: https://ci.nodejs.org/job/node-test-pull-request/69448/

This relaxes the validation in sync hooks so that it accepts the quirky nullish source returned by the default step of the async loader when the module being loaded is CommonJS. When there are no customization hooks registered, a saner synchronous default load step is used to use a property instead of a reset nullish source to signify that the module should go through the CJS monkey patching routes and reduce excessive reloading from disk.

nodejs-github-bot · 2025-10-08T14:55:22Z

CI: https://ci.nodejs.org/job/node-test-pull-request/69621/

joyeecheung · 2025-10-09T06:27:11Z

Addressed the comments and CI is finally green. Can you take a look again? Thanks! @GeoffreyBooth @JakobJingleheimer

This relaxes the validation in sync hooks so that it accepts the quirky nullish source returned by the default step of the async loader when the module being loaded is CommonJS. When there are no customization hooks registered, a saner synchronous default load step is used to use a property instead of a reset nullish source to signify that the module should go through the CJS monkey patching routes and reduce excessive reloading from disk. PR-URL: #59929 Fixes: #59384 Fixes: #57327 Refs: #59666 Refs: https://github.com/dygabo/load_module_test Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Jacob Smith <jacob@frende.me>

nodejs/node#59929

* chore: bump node in DEPS to v24.11.1 * src: add a default branch for module phase nodejs/node#60261 * src: conditionally disable source phase imports by default nodejs/node#60364 * chore: update patches * src: update locks to use DictionaryTemplate and other minor cleanups nodejs/node#60061 * deps: update simdjson to 4.0.7 nodejs/node#59883 * test: move sea tests into test/sea nodejs/node#60250 * fixup deps: update simdjson to 4.0.7a * src: conditionally disable source phase imports by default nodejs/node#60364 * module: handle null source from async loader hooks in sync hooks nodejs/node#59929 * Revert "src: conditionally disable source phase imports by default" This reverts commit 5f85b84. * src: allow disabling JS source phase imports nodejs/node#60364 --------- Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org> Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>

* chore: bump node in DEPS to v24.11.1 * chore: bump node to v24.11.1 (main) (#48917) * chore: bump node in DEPS to v24.11.1 * src: add a default branch for module phase nodejs/node#60261 * src: conditionally disable source phase imports by default nodejs/node#60364 * chore: update patches * src: update locks to use DictionaryTemplate and other minor cleanups nodejs/node#60061 * deps: update simdjson to 4.0.7 nodejs/node#59883 * test: move sea tests into test/sea nodejs/node#60250 * fixup deps: update simdjson to 4.0.7a * src: conditionally disable source phase imports by default nodejs/node#60364 * module: handle null source from async loader hooks in sync hooks nodejs/node#59929 * Revert "src: conditionally disable source phase imports by default" This reverts commit 5f85b84. * src: allow disabling JS source phase imports nodejs/node#60364 --------- Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org> Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com> --------- Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org> Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>

This relaxes the validation in sync hooks so that it accepts the quirky nullish source returned by the default step of the async loader when the module being loaded is CommonJS. When there are no customization hooks registered, a saner synchronous default load step is used to use a property instead of a reset nullish source to signify that the module should go through the CJS monkey patching routes and reduce excessive reloading from disk. PR-URL: nodejs#59929 Fixes: nodejs#59384 Fixes: nodejs#57327 Refs: nodejs#59666 Refs: https://github.com/dygabo/load_module_test Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Jacob Smith <jacob@frende.me>

This relaxes the validation in sync hooks so that it accepts the quirky nullish source returned by the default step of the async loader when the module being loaded is CommonJS. When there are no customization hooks registered, a saner synchronous default load step is used to use a property instead of a reset nullish source to signify that the module should go through the CJS monkey patching routes and reduce excessive reloading from disk. PR-URL: #59929 Backport-PR-URL: #62029 Fixes: #59384 Fixes: #57327 Refs: #59666 Refs: https://github.com/dygabo/load_module_test Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Jacob Smith <jacob@frende.me> Fixes: #61801

module.registerHooks exists since v22.15, but its synchronous load hook rejected the nullish CommonJS source the loader returns for require()s pulled into the ESM graph (ERR_INVALID_RETURN_PROPERTY_VALUE) until nodejs/node#59929, released in 22.22.3, 24.11.1, 25.1.0 and 26.0.0. register() now throws on versions that ship registerHooks but predate the fix (< 22.22.3, <= 24.11.0, 25.0.0) so embedders can fall back to the asynchronous loader instead of crashing mid-graph. supportsSyncHooks() exposes the same check for consumers that register the hooks themselves. Refs: nodejs/node#59929

…upport module.registerHooks rejects the nullish CommonJS source the synchronous loader returns for require()s pulled into the ESM graph (ERR_INVALID_RETURN_PROPERTY_VALUE) until nodejs/node#59929, released in 22.22.3, 24.11.1, 25.1.0 and 26.0.0; note 24.0.0-24.11.0 and 25.0.0 ship registerHooks but predate it. Consult iitm's supportsSyncHooks() (which owns this capability check) and bump the fork to pick it up; unsupported versions fall back to the asynchronous loader. Refs: nodejs/node#59929

Extracting `supportsSyncHooks` into its own module added a relative `import './supports-sync-hooks.mjs'` to `create-hook.mjs`. The `specifiers-map-cleanup` loader evaluates a copy of `create-hook.mjs` from a `data:` URL, which has no hierarchical base, and only rewrote `./lib/...` imports to absolute file URLs. The new sibling import stayed relative and threw `ERR_INVALID_URL`, failing CI on every Node >= 20. 1. Rewrite every relative `./...` import in the loader, not just `./lib/...`, so any sibling module `create-hook.mjs` imports resolves from the `data:` URL. 2. Read the version with `parseInt` + `indexOf` instead of a regex match mapped into a destructure. The major decides every release line except 22/24/25, so the minor and patch are parsed lazily and only when one of those is hit. 3. Document `supportsSyncHooks` and the real support matrix (22.22.3, 24.11.1, 25.1.0, 26.0.0) in the README and `register-hooks.d.ts`; the prior text pointed at the `module.registerHooks()` introduction versions, which ship the API but predate the nullish-CommonJS-source fix. Refs: nodejs/node#59929

## Problem tsx uses the sync `module.registerHooks()` path (no loader worker thread) only once Node can re-enter `Module._load` from sync hooks ([nodejs/node#59929](nodejs/node#59929)). That landed in v24.11.1 / v25.1.0 and was **also backported to v22.22.3**, but the gate had no 22.x entry — so v22.22.3+ kept taking the slower async `module.register()` path. ## Changes - Add `[22, 22, 3]` to the `moduleRegisterHooksCjsReload` gate. v22.22.2 and earlier stay on the async path; v22.22.3+ now uses sync hooks. - Bump the 22.x CI matrix entry `22.22.2` → `22.22.3` so CI exercises the newly-enabled sync path on the 22 line. Verified the backport is behaviorally complete: the full suite (incl. CJS-interop, `require.cache`, watch, version-sensitive) passes with Node 22.22.3 as the runner. Startup on Node 22.22.3, 1000 modules (min of 6 runs, M5 Pro): ``` async (before) sync (this PR) hooks-passthrough 437ms 190ms esm-ts 694ms 337ms ```

The Frida-removal rationale cited the pre-registerHooks breakage history but skipped the episode where the new API and Module._load broke each other: across Node 22.16-22.18 and the 23.x/24.x lines, registering sync hooks rerouted CJS off Module._load entirely (dygabo/load_module_test is the canonical reproducer), plain hooks died with ERR_INVALID_RETURN_PROPERTY_VALUE (nodejs/node#59384), mixing register() with registerHooks() fed CJS a null source (nodejs/node#57327), and nodejs/node#59666 catalogued the rest. Fixed by nodejs/node#59929 and nodejs/node#60380 - the same fix train (v22.22.3 / v24.11.1 / v25.1.0) already cited as iitm's sync-mode version floor elsewhere in the README. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016SaeYZzjXdWXGVMTDHRXQK

hooks/interplay-matrix runs self-contained scenarios (in the spirit of dygabo/load_module_test) against a ladder of official Node builds bracketing the nodejs/node#59929 fix train, plus this repo's real runtime hook via the ABI-stable napi addon. pnpm matrix rebuilds matrix.md. What the ladder shows: with a pure-passthrough sync load hook registered, a Module._load monkey-patch never sees import-ed CJS on 22.15.0-22.22.2 and 24.10.0-24.11.0, flipping to visible at exactly 22.22.3 and 24.11.1 - the blinding, empirically pinned to the fix boundary. The hook-fed synthetic require carries neither require.extensions nor require.cache until 24.18.0 and still carries neither anywhere on 22.x (22.23.1 included). Plain require() stayed on Module._load throughout, and the exports tap passes on every rung because it never depended on the patch point - the operative fact for Lambda runtimes that may sit on pre-fix minors (AWS applies Node minor updates on its own cadence; the base images rebuild daily without exposing the embedded minor, so process.version in a live function is the only ground truth). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016SaeYZzjXdWXGVMTDHRXQK

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Sep 18, 2025

joyeecheung force-pushed the commonjs-source branch 2 times, most recently from db791a0 to 64e35a5 Compare September 18, 2025 17:50

joyeecheung force-pushed the commonjs-source branch from 64e35a5 to ea58659 Compare September 22, 2025 11:42

joyeecheung mentioned this pull request Sep 22, 2025

ERR_INVALID_RETURN_PROPERTY_VALUE when using module.registerHooks #59384

Closed

joyeecheung added the review wanted PRs that need reviews. label Sep 22, 2025

GeoffreyBooth approved these changes Sep 22, 2025

View reviewed changes

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 23, 2025

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 23, 2025

JakobJingleheimer self-requested a review September 23, 2025 20:12

JakobJingleheimer approved these changes Sep 24, 2025

View reviewed changes

joyeecheung force-pushed the commonjs-source branch from ea58659 to 6ea04e7 Compare September 29, 2025 14:44

joyeecheung mentioned this pull request Sep 29, 2025

module: implement synchronous module evaluate hooks #57139

Closed

joyeecheung force-pushed the commonjs-source branch from 6ea04e7 to b5663af Compare October 8, 2025 13:38

fixup! module: handle null source from async loader hooks in sync hooks

790b5f4

joyeecheung force-pushed the commonjs-source branch from b5663af to 790b5f4 Compare October 8, 2025 14:54

JakobJingleheimer approved these changes Oct 13, 2025

View reviewed changes

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 13, 2025

github-actions Bot mentioned this pull request Nov 7, 2025

2025-11-11, Version 24.11.1 'Krypton' (LTS) #60622

Merged

joyeecheung mentioned this pull request Nov 9, 2025

module.registerHooks() tracking issue #56241

Open

11 tasks

jkleinsc added a commit to electron/electron that referenced this pull request Nov 14, 2025

module: handle null source from async loader hooks in sync hooks

ff50050

nodejs/node#59929

joyeecheung mentioned this pull request Dec 6, 2025

registerHooks: TypeError when json file is required in hook and in the imported file #57358

Closed

sxzz mentioned this pull request Feb 13, 2026

Request: Backport loader fix to v22.x #61801

Open

joyeecheung mentioned this pull request Feb 27, 2026

[v22.x] backport module loader & loader hook fixes #62029

Closed

songkeys mentioned this pull request Apr 23, 2026

TypeError: ...got undefined from load hook when using Vite plugin in a Windows + pnpm environment songkeys/tailwind-preset-mantine#43

Closed

marco-ippolito mentioned this pull request May 11, 2026

2026-05-13, Version 22.22.3 'Jod' (LTS), @marco-ippolito #63250

Merged

danieltroger mentioned this pull request May 18, 2026

[BUG] HAS_BROKEN_FSTAT_FOR_ZIP_FDS version gate should include Node 22.22+ yarnpkg/berry#7140

Closed

claude Bot mentioned this pull request Jul 14, 2026

fix(server): replace deprecated --experimental-loader with register() for Otel instrumentation medplum/medplum#9836

Open

This was referenced Jul 15, 2026

[Feature]: built-in TS transform for runtime-loaded modules outside the bundle graph web-infra-dev/rstest#1577

Open

feat(core): add runtimeTsTransform for runtime-loaded TypeScript web-infra-dev/rstest#1579

Draft

bareketsarusi mentioned this pull request Jul 16, 2026

Sync (registerHooks) + async (register) loaders co-residence: hook silently never fires nodejs/import-in-the-middle#274

Open

coderabbitai Bot mentioned this pull request Jul 21, 2026

feat(messaging): add Google Chat channel NVIDIA/NemoClaw#7317

Draft

21 tasks

Uh oh!

Uh oh!

Conversation

joyeecheung commented Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Sep 18, 2025

Uh oh!

codecov Bot commented Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nodejs-github-bot commented Sep 22, 2025

Uh oh!

joyeecheung commented Sep 22, 2025

Uh oh!

GeoffreyBooth left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Sep 23, 2025

Uh oh!

nodejs-github-bot commented Sep 24, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Sep 29, 2025

Uh oh!

nodejs-github-bot commented Sep 29, 2025

Uh oh!

nodejs-github-bot commented Oct 8, 2025

Uh oh!

joyeecheung commented Oct 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

joyeecheung commented Sep 18, 2025 •

edited

Loading

codecov Bot commented Sep 18, 2025 •

edited

Loading