Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit d147c08

Browse filesBrowse files
joyeecheungaduh95
joyeecheung
authored and
aduh95
committed
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 17122e5 commit d147c08
Copy full SHA for d147c08

5 files changed

+59-11Lines changed: 59 additions & 11 deletions

File tree

Expand file treeCollapse file tree
Open diff view settings
Filter options
Expand file treeCollapse file tree
Open diff view settings
Collapse file

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

Copy file name to clipboardExpand all lines: lib/internal/modules/cjs/loader.js
+10-8Lines 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

Collapse file

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

Copy file name to clipboardExpand all lines: lib/internal/modules/esm/translators.js
+10-3Lines 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,
Collapse file

‎test/fixtures/value.cjs‎

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

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

Copy file name to clipboard
+20Lines 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+
}));
Collapse file

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

Copy file name to clipboard
+18Lines 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 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.