[v22.x] backport module loader & loader hook fixes by joyeecheung · Pull Request #62029 · nodejs/node · GitHub

This backports the following bug fixes/refactors:

As discussed in #61801 (comment), #59929 is backported with an additional diff to check how the module is loaded differently (by observing NODE_DEBUG output, instead of checking that require.cache does not exist) for the --experimental-default-type=module test, since this feature was only in early development and had been removed since v23.4.0, backporting a small behavior change to v22 (which arguably is the less surprising behavior) should be fine.

see diff

diff --git a/test/es-module/test-esm-type-flag-errors.mjs b/test/es-module/test-esm-type-flag-errors.mjs
index 2f7a1db35d2..fbef806b907 100644
--- a/test/es-module/test-esm-type-flag-errors.mjs
+++ b/test/es-module/test-esm-type-flag-errors.mjs
@@ -36,14 +36,16 @@ describe('--experimental-default-type=module', { concurrency: !process.env.TEST_
     const result = await spawnPromisified(process.execPath, [
       '--experimental-default-type=module',
       fixtures.path('es-modules/package-type-commonjs/echo-require-cache.js'),
-    ]);
-
-    deepStrictEqual(result, {
-      code: 0,
-      stderr: '',
-      stdout: 'undefined\n',
-      signal: null,
+    ], {
+      env: {
+        ...process.env,
+        NODE_DEBUG: 'esm',
+      }
     });
+    match(result.stderr, /Translating CJSModule file.+echo-require-cache\.js/);
+    match(result.stdout, /Object: null prototype/);
+    strictEqual(result.code, 0);
+    strictEqual(result.signal, null);
   });
 
   it('should affect .cjs files that are imported', async () => {
@@ -51,25 +53,34 @@ describe('--experimental-default-type=module', { concurrency: !process.env.TEST_
       '--experimental-default-type=module',
       '-e',
       `import ${JSON.stringify(fixtures.fileURL('es-module-require-cache/echo.cjs'))}`,
-    ]);
-
-    deepStrictEqual(result, {
-      code: 0,
-      stderr: '',
-      stdout: 'undefined\n',
-      signal: null,
+    ], {
+      env: {
+        ...process.env,
+        NODE_DEBUG: 'esm',
+      }
     });
+
+    match(result.stderr, /Translating CJSModule file.+echo\.cjs/);
+    match(result.stdout, /Object: null prototype/);
+    strictEqual(result.code, 0);
+    strictEqual(result.signal, null);
   });
 
   it('should affect entry point .cjs files (with no hooks)', async () => {
-    const { stderr, stdout, code } = await spawnPromisified(process.execPath, [
+    const result = await spawnPromisified(process.execPath, [
       '--experimental-default-type=module',
       fixtures.path('es-module-require-cache/echo.cjs'),
-    ]);
+    ], {
+      env: {
+        ...process.env,
+        NODE_DEBUG: 'esm',
+      }
+    });
 
-    strictEqual(stderr, '');
-    match(stdout, /^undefined\n$/);
-    strictEqual(code, 0);
+    match(result.stderr, /Translating CJSModule file.+echo\.cjs/);
+    match(result.stdout, /Object: null prototype/);
+    strictEqual(result.code, 0);
+    strictEqual(result.signal, null);
   });
 
   it('should affect entry point .cjs files (when any hooks is registered)', async () => {

Otherwise if the ESM happens to be cached separately by the ESM loader before it gets loaded with `require(esm)` from within an imported CJS file (which uses a re-invented require() with a couple of quirks, including a separate cache), it won't be able to load the esm properly from the cache. PR-URL: nodejs#59679 Refs: nodejs#59666 Refs: nodejs#52697 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

This reduces the impact of https://redirect.github.com/nodejs/node/pull/59679 by delaying the require.cache population of ESM until they are directly required. After that, it's necessary for them to be in the cache to maintain correctness. PR-URL: nodejs#59874 Refs: nodejs#59868 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

PR-URL: nodejs#60072 Fixes: nodejs#59963 Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.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>

Previously, when require()-ing builtins with the node: prefix, the sync resolve hooks were not properly invoked, and load hooks could not override the builtin's format. This fixes the handling and enables redirecting prefixed built-ins to on-disk files and overriding them with other module types via hooks. PR-URL: nodejs#61088 Fixes: nodejs#60005 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>

This prevents clobbering the stack traces with another internal frame and removes the unnecessary hoops from step-debugging. PR-URL: nodejs#61479 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>

Previously the resolve hook can be invoked twice from the synthetic module evaluation step of imported CJS in the extra module._load() call that's invoked on the resolved full path. Add an option to avoid it, since the resolution and loading has already been done before. PR-URL: nodejs#61529 Fixes: nodejs#57125 Refs: nodejs#55808 Refs: nodejs#56241 Reviewed-By: Jacob Smith <jacob@frende.me>

Review requested:

@nodejs/loaders
@nodejs/vm

I'm in favour of the change in principle (haven't had a chance to review the code yet).

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

cc @nodejs/releasers Should this be merged?

Otherwise if the ESM happens to be cached separately by the ESM loader before it gets loaded with `require(esm)` from within an imported CJS file (which uses a re-invented require() with a couple of quirks, including a separate cache), it won't be able to load the esm properly from the cache. PR-URL: #59679 Backport-PR-URL: #62029 Refs: #59666 Refs: #52697 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Fixes: #61801

This reduces the impact of https://redirect.github.com/nodejs/node/pull/59679 by delaying the require.cache population of ESM until they are directly required. After that, it's necessary for them to be in the cache to maintain correctness. PR-URL: #59874 Backport-PR-URL: #62029 Refs: #59868 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Fixes: #61801

PR-URL: #60072 Backport-PR-URL: #62029 Fixes: #59963 Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Fixes: #61801

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

Previously, when require()-ing builtins with the node: prefix, the sync resolve hooks were not properly invoked, and load hooks could not override the builtin's format. This fixes the handling and enables redirecting prefixed built-ins to on-disk files and overriding them with other module types via hooks. PR-URL: #61088 Backport-PR-URL: #62029 Fixes: #60005 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Fixes: #61801

This prevents clobbering the stack traces with another internal frame and removes the unnecessary hoops from step-debugging. PR-URL: #61479 Backport-PR-URL: #62029 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Fixes: #61801

Previously the resolve hook can be invoked twice from the synthetic module evaluation step of imported CJS in the extra module._load() call that's invoked on the resolved full path. Add an option to avoid it, since the resolution and loading has already been done before. PR-URL: #61529 Backport-PR-URL: #62029 Fixes: #57125 Refs: #55808 Refs: #56241 Reviewed-By: Jacob Smith <jacob@frende.me> Fixes: #61801

Landed in dbabdf7...5065a0a

joyeecheung and others added 7 commits February 25, 2026 20:27

module: use sync cjs when importing cts

4aff83c

PR-URL: nodejs#60072 Fixes: nodejs#59963 Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

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. v22.x Issues that can be reproduced on v22.x or PRs targeting the v22.x-staging branch. labels Feb 27, 2026

joyeecheung changed the title Backport module fixes 22 [v22.x] backport module loader & loader hook fixes Feb 27, 2026

joyeecheung mentioned this pull request Feb 27, 2026

Request: Backport loader fix to v22.x #61801

Closed

Renegade334 linked an issue Mar 3, 2026 that may be closed by this pull request

Request: Backport loader fix to v22.x #61801

Closed

gengjiawen approved these changes Mar 5, 2026

View reviewed changes

gengjiawen added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 5, 2026

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 5, 2026

JakobJingleheimer approved these changes Mar 8, 2026

View reviewed changes

joyeecheung added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. labels Mar 9, 2026

gengjiawen added the fast-track PRs that do not need to wait for 48 hours to land. label Mar 12, 2026

This comment was marked as off-topic.

Sign in to view

aduh95 added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. and removed fast-track PRs that do not need to wait for 48 hours to land. commit-queue Add this label to land a pull request using GitHub Actions. labels Mar 12, 2026

marco-ippolito closed this Apr 27, 2026

[v22.x] backport module loader & loader hook fixes by joyeecheung · Pull Request #62029 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

[v22.x] backport module loader & loader hook fixes#62029
joyeecheung wants to merge 7 commits into
nodejs:v22.x-stagingfrom
joyeecheung:backport-module-fixes-22

joyeecheung commented Feb 27, 2026 •
edited
Loading

Uh oh!

Uh oh!

nodejs-github-bot commented Feb 27, 2026

Uh oh!

JakobJingleheimer commented Feb 27, 2026

Uh oh!

nodejs-github-bot commented Mar 5, 2026

Uh oh!

joyeecheung commented Mar 6, 2026

Uh oh!

This comment was marked as off-topic.

marco-ippolito commented Apr 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

[v22.x] backport module loader & loader hook fixes by joyeecheung · Pull Request #62029 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

[v22.x] backport module loader & loader hook fixes#62029joyeecheung wants to merge 7 commits intonodejs:v22.x-stagingnodejs/node:v22.x-stagingfrom joyeecheung:backport-module-fixes-22joyeecheung/node:backport-module-fixes-22Copy head branch name to clipboard

Conversation

joyeecheung commented Feb 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Feb 27, 2026

Uh oh!

JakobJingleheimer commented Feb 27, 2026

Uh oh!

nodejs-github-bot commented Mar 5, 2026

Uh oh!

joyeecheung commented Mar 6, 2026

Uh oh!

This comment was marked as off-topic.

marco-ippolito commented Apr 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

[v22.x] backport module loader & loader hook fixes#62029
joyeecheung wants to merge 7 commits into
nodejs:v22.x-stagingfrom
joyeecheung:backport-module-fixes-22

joyeecheung commented Feb 27, 2026 •
edited
Loading