Uh oh!
There was an error while loading. Please reload this page.
module: handle null source from async loader hooks in sync hooks - #59929
Merged
Conversation
nodejs-github-bot
commented
Sep 18, 2025
Collaborator
Review requested:
|
joyeecheungforce-pushed
the
commonjs-source
branch
2 times, most recently
from
September 18, 2025 17:50
db791a0 to
64e35a5CompareCodecov Report❌ Patch coverage is
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
🚀 New features to boost your workflow:
|
joyeecheungforce-pushed
the
commonjs-source
branch
from
September 22, 2025 11:42
64e35a5 to
ea58659Comparenodejs-github-bot
commented
Sep 22, 2025
Collaborator
joyeecheung
commented
Sep 22, 2025
MemberAuthor
This is necessary to fix known issues with sync/async loader hook interop. Can I get some reviews please? @nodejs/loaders |
GeoffreyBooth
approved these changes
Sep 22, 2025
GeoffreyBooth
left a comment
Member
There was a problem hiding this comment.
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
commented
Sep 23, 2025
Collaborator
JakobJingleheimer
self-requested a review
September 23, 2025 20:12
nodejs-github-bot
commented
Sep 24, 2025
Collaborator
JakobJingleheimer
approved these changes
Sep 24, 2025
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
joyeecheungforce-pushed
the
commonjs-source
branch
from
September 29, 2025 14:44
ea58659 to
6ea04e7Comparenodejs-github-bot
commented
Sep 29, 2025
Collaborator
nodejs-github-bot
commented
Sep 29, 2025
Collaborator
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.
joyeecheungforce-pushed
the
commonjs-source
branch
from
October 8, 2025 13:38
6ea04e7 to
b5663afComparejoyeecheungforce-pushed
the
commonjs-source
branch
from
October 8, 2025 14:54
b5663af to
790b5f4Comparenodejs-github-bot
commented
Oct 8, 2025
Collaborator
joyeecheung
commented
Oct 9, 2025
MemberAuthor
Addressed the comments and CI is finally green. Can you take a look again? Thanks! @GeoffreyBooth@JakobJingleheimer |
JakobJingleheimer
approved these changes
Oct 13, 2025
11 tasks
jkleinsc added a commit
to electron/electron
that referenced
this pull request
Nov 14, 2025
jkleinsc added a commit
to electron/electron
that referenced
this pull request
Nov 18, 2025
* 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>
jkleinsc added a commit
to electron/electron
that referenced
this pull request
Nov 18, 2025
* 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>
jkleinsc added a commit
to electron/electron
that referenced
this pull request
Nov 18, 2025
* 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>
jkleinsc added a commit
to electron/electron
that referenced
this pull request
Nov 18, 2025
* 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>
joyeecheung added a commit
to joyeecheung/node
that referenced
this pull request
Feb 27, 2026
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#59929Fixes: nodejs#59384Fixes: 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>
marco-ippolito pushed a commit
that referenced
this pull request
Apr 27, 2026
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: #62029Fixes: #59384Fixes: #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
BridgeAR added a commit
to BridgeAR/import-in-the-middle
that referenced
this pull request
Jun 17, 2026
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
BridgeAR added a commit
to DataDog/dd-trace-js
that referenced
this pull request
Jun 17, 2026
…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
BridgeAR added a commit
to BridgeAR/import-in-the-middle
that referenced
this pull request
Jun 29, 2026
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
BridgeAR added a commit
to BridgeAR/import-in-the-middle
that referenced
this pull request
Jun 29, 2026
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
privatenumber added a commit
to privatenumber/tsx
that referenced
this pull request
Jul 13, 2026
## 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 ```
22 tasks
filipkunc pushed a commit
to filipkunc/wrap-esm-lambda
that referenced
this pull request
Jul 22, 2026
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
filipkunc pushed a commit
to filipkunc/wrap-esm-lambda
that referenced
this pull request
Jul 22, 2026
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
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
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