FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

module: fix submodules loaded by require() and import() · nodejs/node@7625dc4 · GitHub

/ node Public

Commit 7625dc4

Browse files
joyeecheung
authored and
marco-ippolito
committed
module: fix submodules loaded by require() and import()
Previously there is an edge case where submodules loaded by require() may not be loaded by import() again from different intermediate edges in the graph. This patch fixes that, added tests, and added debug logs. Drive-by: make loader a private field so it doesn't show up in logs. PR-URL: #52487 Backport-PR-URL: #53500 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
1 parent 6c4f477 commit 7625dc4

8 files changed

Lines changed: 67 additions & 12 deletions

File tree

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -316,7 +316,7 @@ class ModuleLoader {
316316
* @param {string} specifier Specifier of the the imported module.
317317
* @param {string} parentURL Where the import comes from.
318318
* @param {object} importAttributes import attributes from the import statement.
319-
* @returns {ModuleWrap}
319+
* @returns {ModuleJobBase}
320320
*/
321321
getModuleWrapForRequire(specifier, parentURL, importAttributes) {
322322
assert(getOptionValue('--experimental-require-module'));
@@ -355,7 +355,7 @@ class ModuleLoader {
355355
// completed (e.g. the require call is lazy) so it's okay. We will return the
356356
// module now and check asynchronicity of the entire graph later, after the
357357
// graph is instantiated.
358-
return job.module;
358+
return job;
359359
}
360360

361361
defaultLoadSync ??= require('internal/modules/esm/load').defaultLoadSync;
@@ -403,7 +403,7 @@ class ModuleLoader {
403403
job = new ModuleJobSync(this, url, importAttributes, wrap, isMain, inspectBrk);
404404

405405
this.loadCache.set(url, importAttributes.type, job);
406-
return job.module;
406+
return job;
407407
}
408408

409409
/**

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

Lines changed: 25 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,9 @@ const {
1818
StringPrototypeSplit,
1919
StringPrototypeStartsWith,
2020
} = primordials;
21+
let debug = require('internal/util/debuglog').debuglog('esm', (fn) => {
22+
debug = fn;
23+
});
2124

2225
const { ModuleWrap, kEvaluated } = internalBinding('module_wrap');
2326

@@ -48,8 +51,7 @@ const isCommonJSGlobalLikeNotDefinedError = (errorMessage) =>
4851
);
4952

5053
class ModuleJobBase {
51-
constructor(loader, url, importAttributes, moduleWrapMaybePromise, isMain, inspectBrk) {
52-
this.loader = loader;
54+
constructor(url, importAttributes, moduleWrapMaybePromise, isMain, inspectBrk) {
5355
this.importAttributes = importAttributes;
5456
this.isMain = isMain;
5557
this.inspectBrk = inspectBrk;
@@ -62,11 +64,13 @@ class ModuleJobBase {
6264
/* A ModuleJob tracks the loading of a single Module, and the ModuleJobs of
6365
* its dependencies, over time. */
6466
class ModuleJob extends ModuleJobBase {
67+
#loader = null;
6568
// `loader` is the Loader instance used for loading dependencies.
6669
constructor(loader, url, importAttributes = { __proto__: null },
6770
moduleProvider, isMain, inspectBrk, sync = false) {
6871
const modulePromise = ReflectApply(moduleProvider, loader, [url, isMain]);
69-
super(loader, url, importAttributes, modulePromise, isMain, inspectBrk);
72+
super(url, importAttributes, modulePromise, isMain, inspectBrk);
73+
this.#loader = loader;
7074
// Expose the promise to the ModuleWrap directly for linking below.
7175
// `this.module` is also filled in below.
7276
this.modulePromise = modulePromise;
@@ -89,7 +93,8 @@ class ModuleJob extends ModuleJobBase {
8993
// these `link` callbacks depending on each other.
9094
const dependencyJobs = [];
9195
const promises = this.module.link(async (specifier, attributes) => {
92-
const job = await this.loader.getModuleJob(specifier, url, attributes);
96+
const job = await this.#loader.getModuleJob(specifier, url, attributes);
97+
debug(`async link() ${this.url} -> ${specifier}`, job);
9398
ArrayPrototypePush(dependencyJobs, job);
9499
return job.modulePromise;
95100
});
@@ -121,6 +126,8 @@ class ModuleJob extends ModuleJobBase {
121126
async _instantiate() {
122127
const jobsInGraph = new SafeSet();
123128
const addJobsToDependencyGraph = async (moduleJob) => {
129+
debug(`async addJobsToDependencyGraph() ${this.url}`, moduleJob);
130+
124131
if (jobsInGraph.has(moduleJob)) {
125132
return;
126133
}
@@ -156,7 +163,7 @@ class ModuleJob extends ModuleJobBase {
156163
const { 1: childSpecifier, 2: name } = RegExpPrototypeExec(
157164
/module '(.*)' does not provide an export named '(.+)'/,
158165
e.message);
159-
const { url: childFileURL } = await this.loader.resolve(
166+
const { url: childFileURL } = await this.#loader.resolve(
160167
childSpecifier,
161168
parentFileUrl,
162169
kEmptyObject,
@@ -167,7 +174,7 @@ class ModuleJob extends ModuleJobBase {
167174
// in the import attributes and some formats require them; but we only
168175
// care about CommonJS for the purposes of this error message.
169176
({ format } =
170-
await this.loader.load(childFileURL));
177+
await this.#loader.load(childFileURL));
171178
} catch {
172179
// Continue regardless of error.
173180
}
@@ -257,18 +264,27 @@ class ModuleJob extends ModuleJobBase {
257264
// All the steps are ensured to be synchronous and it throws on instantiating
258265
// an asynchronous graph.
259266
class ModuleJobSync extends ModuleJobBase {
267+
#loader = null;
260268
constructor(loader, url, importAttributes, moduleWrap, isMain, inspectBrk) {
261-
super(loader, url, importAttributes, moduleWrap, isMain, inspectBrk, true);
269+
super(url, importAttributes, moduleWrap, isMain, inspectBrk, true);
262270
assert(this.module instanceof ModuleWrap);
271+
this.#loader = loader;
263272
const moduleRequests = this.module.getModuleRequestsSync();
273+
const linked = [];
264274
for (let i = 0; i < moduleRequests.length; ++i) {
265275
const { 0: specifier, 1: attributes } = moduleRequests[i];
266-
const wrap = this.loader.getModuleWrapForRequire(specifier, url, attributes);
276+
const job = this.#loader.getModuleWrapForRequire(specifier, url, attributes);
267277
const isLast = (i === moduleRequests.length - 1);
268278
// TODO(joyeecheung): make the resolution callback deal with both promisified
269279
// an raw module wraps, then we don't need to wrap it with a promise here.
270-
this.module.cacheResolvedWrapsSync(specifier, PromiseResolve(wrap), isLast);
280+
this.module.cacheResolvedWrapsSync(specifier, PromiseResolve(job.module), isLast);
281+
ArrayPrototypePush(linked, job);
271282
}
283+
this.linked = linked;
284+
}
285+
286+
get modulePromise() {
287+
return PromiseResolve(this.module);
272288
}
273289

274290
async run() {

‎test/es-module/test-require-module-dynamic-import-3.js‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
// Flags: --experimental-require-module
2+
'use strict';
3+
4+
// This tests that previously synchronously loaded submodule can still
5+
// be loaded by dynamic import().
6+
7+
const common = require('../common');
8+
const assert = require('assert');
9+
10+
(async () => {
11+
const required = require('../fixtures/es-modules/require-and-import/load.cjs');
12+
const imported = await import('../fixtures/es-modules/require-and-import/load.mjs');
13+
assert.deepStrictEqual({ ...required }, { ...imported });
14+
})().then(common.mustCall());

‎test/es-module/test-require-module-dynamic-import-4.js‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
// Flags: --experimental-require-module
2+
'use strict';
3+
4+
// This tests that previously asynchronously loaded submodule can still
5+
// be loaded by require().
6+
7+
const common = require('../common');
8+
const assert = require('assert');
9+
10+
(async () => {
11+
const imported = await import('../fixtures/es-modules/require-and-import/load.mjs');
12+
const required = require('../fixtures/es-modules/require-and-import/load.cjs');
13+
assert.deepStrictEqual({ ...required }, { ...imported });
14+
})().then(common.mustCall());

‎test/fixtures/es-modules/require-and-import/load.cjs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
module.exports = require('dep');
2+

‎test/fixtures/es-modules/require-and-import/load.mjs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
export * from 'dep';
2+

‎test/fixtures/es-modules/require-and-import/node_modules/dep/mod.js‎

Lines changed: 2 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎test/fixtures/es-modules/require-and-import/node_modules/dep/package.json‎

Lines changed: 5 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL