Skip to content

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

Open
joyeecheung wants to merge 7 commits intonodejs:v22.x-stagingfrom
joyeecheung:backport-module-fixes-22
Open

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

Conversation

@joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Feb 27, 2026 •
edited
Loading

Fixes: #61801

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 () => {

joyeecheung and others added 7 commits February 25, 2026 20:27
@joyeecheung
esm: populate separate cache for require(esm) in imported CJS
7e19cd0 
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>
@joyeecheung
module: only put directly require-d ESM into require.cache
041e977 
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>
@marco-ippolito @joyeecheung
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>
@joyeecheung
module: handle null source from async loader hooks in sync hooks
63468af 
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>
@joyeecheung
module: fix sync resolve hooks for require with node: prefixes
cf1e37d 
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>
@joyeecheung
module: do not wrap module._load when tracing is not enabled
6df0cdf 
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>
@joyeecheung
module: do not invoke resolve hooks twice for imported cjs
7d1b217 
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>
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 27, 2026

Review requested:

  • @nodejs/loaders
  • @nodejs/vm

@nodejs-github-bot 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 joyeecheung changed the title Backport module fixes 22 [v22.x] backport module loader & loader hook fixes Feb 27, 2026
@joyeecheung joyeecheung mentioned this pull request Feb 27, 2026
Open
@JakobJingleheimer
Copy link
Member

JakobJingleheimer commented Feb 27, 2026

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@joyeecheung @nodejs-github-bot @JakobJingleheimer @marco-ippolito