Visitar URL original
module: do not invoke resolve hooks twice for imported cjs · nodejs/node@f4646a7 · GitHub
Skip to content

Commit f4646a7

Browse files
joyeecheungjoyeecheung
authored
module: do not invoke resolve hooks twice for imported cjs
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 Fixes: #57125 Refs: #55808 Refs: #56241 Reviewed-By: Jacob Smith <jacob@frende.me>
1 parent 61a4a30 commit f4646a7

5 files changed

Lines changed: 59 additions & 11 deletions

File tree

‎lib/internal/modules/cjs/loader.js‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -235,7 +235,7 @@ let statCache = null;
235235
* See more {@link Module._load}
236236
* @returns {object}
237237
*/
238-
function wrapModuleLoad(request, parent, isMain) {
238+
function wrapModuleLoad(request, parent, isMain, options) {
239239
const logLabel = `[${parent?.id || ''}] [${request}]`;
240240
const traceLabel = `require('${request}')`;
241241
const channel = onRequire();
@@ -248,11 +248,11 @@ function wrapModuleLoad(request, parent, isMain) {
248248
__proto__: null,
249249
parentFilename: parent?.filename,
250250
id: request,
251-
}, Module, request, parent, isMain);
251+
}, Module, request, parent, isMain, options);
252252
}
253253
// No subscribers, skip the wrapping to avoid clobbering stack traces
254254
// and debugging steps.
255-
return Module._load(request, parent, isMain);
255+
return Module._load(request, parent, isMain, options);
256256
} finally {
257257
endTimer(logLabel, traceLabel);
258258
}
@@ -1040,9 +1040,10 @@ function getExportsForCircularRequire(module) {
10401040
* @param {string} specifier
10411041
* @param {Module|undefined} parent
10421042
* @param {boolean} isMain
1043+
* @param {boolean} shouldSkipModuleHooks
10431044
* @returns {{url?: string, format?: string, parentURL?: string, filename: string}}
10441045
*/
1045-
function resolveForCJSWithHooks(specifier, parent, isMain) {
1046+
function resolveForCJSWithHooks(specifier, parent, isMain, shouldSkipModuleHooks) {
10461047
let defaultResolvedURL;
10471048
let defaultResolvedFilename;
10481049
let format;
@@ -1065,7 +1066,7 @@ function resolveForCJSWithHooks(specifier, parent, isMain) {
10651066
}
10661067

10671068
// Fast path: no hooks, just return simple results.
1068-
if (!resolveHooks.length) {
1069+
if (!resolveHooks.length || shouldSkipModuleHooks) {
10691070
const filename = defaultResolveImpl(specifier, parent, isMain);
10701071
return { __proto__: null, url: defaultResolvedURL, filename, format };
10711072
}
@@ -1118,7 +1119,7 @@ function resolveForCJSWithHooks(specifier, parent, isMain) {
11181119
}
11191120

11201121
const result = { __proto__: null, url, format, filename, parentURL };
1121-
debug('resolveForCJSWithHooks', specifier, parent?.id, '->', result);
1122+
debug('resolveForCJSWithHooks', specifier, parent?.id, isMain, shouldSkipModuleHooks, '->', result);
11221123
return result;
11231124
}
11241125

@@ -1211,9 +1212,10 @@ function loadBuiltinWithHooks(id, url, format) {
12111212
* @param {string} request Specifier of module to load via `require`
12121213
* @param {Module} parent Absolute path of the module importing the child
12131214
* @param {boolean} isMain Whether the module is the main entry point
1215+
* @param {object|undefined} options Additional options for loading the module
12141216
* @returns {object}
12151217
*/
1216-
Module._load = function(request, parent, isMain) {
1218+
Module._load = function(request, parent, isMain, options = kEmptyObject) {
12171219
let relResolveCacheIdentifier;
12181220
if (parent) {
12191221
debug('Module._load REQUEST %s parent: %s', request, parent.id);
@@ -1236,7 +1238,7 @@ Module._load = function(request, parent, isMain) {
12361238
}
12371239
}
12381240

1239-
const resolveResult = resolveForCJSWithHooks(request, parent, isMain);
1241+
const resolveResult = resolveForCJSWithHooks(request, parent, isMain, options.shouldSkipModuleHooks);
12401242
let { format } = resolveResult;
12411243
const { url, filename } = resolveResult;
12421244

‎lib/internal/modules/esm/translators.js‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,7 @@ translators.set('module', function moduleStrategy(url, translateContext, parentU
100100
});
101101

102102
const { requestTypes: { kRequireInImportedCJS } } = require('internal/modules/esm/utils');
103+
const kShouldSkipModuleHooks = { __proto__: null, shouldSkipModuleHooks: true };
103104
/**
104105
* Loads a CommonJS module via the ESM Loader sync CommonJS translator.
105106
* This translator creates its own version of the `require` function passed into CommonJS modules.
@@ -132,7 +133,9 @@ function loadCJSModule(module, source, url, filename, isMain) {
132133
importAttributes = { __proto__: null, type: 'json' };
133134
break;
134135
case '.node':
135-
return wrapModuleLoad(specifier, module);
136+
// If it gets here in the translators, the hooks must have already been invoked
137+
// in the loader. Skip them in the synthetic module evaluation step.
138+
return wrapModuleLoad(specifier, module, false, kShouldSkipModuleHooks);
136139
default:
137140
// fall through
138141
}
@@ -280,7 +283,9 @@ function createCJSNoSourceModuleWrap(url, parentURL) {
280283
debug(`Loading CJSModule ${url}`);
281284

282285
if (!module.loaded) {
283-
wrapModuleLoad(filename, null, isMain);
286+
// If it gets here in the translators, the hooks must have already been invoked
287+
// in the loader. Skip them in the synthetic module evaluation step.
288+
wrapModuleLoad(filename, null, isMain, kShouldSkipModuleHooks);
284289
}
285290

286291
/** @type {import('./loader').ModuleExports} */
@@ -317,7 +322,9 @@ translators.set('require-commonjs-typescript', (url, translateContext, parentURL
317322
// This goes through Module._load to accommodate monkey-patchers.
318323
function loadCJSModuleWithModuleLoad(module, source, url, filename, isMain) {
319324
assert(module === CJSModule._cache[filename]);
320-
wrapModuleLoad(filename, undefined, isMain);
325+
// If it gets here in the translators, the hooks must have already been invoked
326+
// in the loader. Skip them in the synthetic module evaluation step.
327+
wrapModuleLoad(filename, undefined, isMain, kShouldSkipModuleHooks);
321328
}
322329

323330
// Handle CommonJS modules referenced by `import` statements or expressions,

‎test/fixtures/value.cjs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
exports.value = 42;

‎test/module-hooks/test-module-hooks-load-import-cjs.js‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
'use strict';
2+
// Test that load hook in imported CJS only gets invoked once.
3+
4+
const common = require('../common');
5+
const assert = require('assert');
6+
const { registerHooks } = require('module');
7+
const path = require('path');
8+
const { pathToFileURL } = require('url');
9+
10+
const hook = registerHooks({
11+
load: common.mustCall(function(url, context, nextLoad) {
12+
assert.strictEqual(url, pathToFileURL(path.resolve(__dirname, '../fixtures/value.cjs')).href);
13+
return nextLoad(url, context);
14+
}, 1),
15+
});
16+
17+
import('../fixtures/value.cjs').then(common.mustCall((result) => {
18+
assert.strictEqual(result.value, 42);
19+
hook.deregister();
20+
}));

‎test/module-hooks/test-module-hooks-resolve-import-cjs.js‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
'use strict';
2+
// Test that resolve hook in imported CJS only gets invoked once.
3+
4+
const common = require('../common');
5+
const assert = require('assert');
6+
const { registerHooks } = require('module');
7+
8+
const hook = registerHooks({
9+
resolve: common.mustCall(function(specifier, context, nextResolve) {
10+
assert.strictEqual(specifier, '../fixtures/value.cjs');
11+
return nextResolve(specifier, context);
12+
}, 1),
13+
});
14+
15+
import('../fixtures/value.cjs').then(common.mustCall((result) => {
16+
assert.strictEqual(result.value, 42);
17+
hook.deregister();
18+
}));

0 commit comments

Comments
 (0)