From 7298d8dc193b4c9d94337a4688e4bfd96d9f5195 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Tue, 16 Apr 2024 16:07:44 +0200 Subject: [PATCH] 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: https://github.com/nodejs/node/pull/52487 Reviewed-By: Geoffrey Booth Reviewed-By: Benjamin Gruenbaum --- lib/internal/modules/esm/loader.js | 6 ++-- lib/internal/modules/esm/module_job.js | 34 ++++++++++++++----- .../test-require-module-dynamic-import-3.js | 14 ++++++++ .../test-require-module-dynamic-import-4.js | 14 ++++++++ .../es-modules/require-and-import/load.cjs | 2 ++ .../es-modules/require-and-import/load.mjs | 2 ++ .../node_modules/dep/mod.js | 2 ++ .../node_modules/dep/package.json | 5 +++ 8 files changed, 67 insertions(+), 12 deletions(-) create mode 100644 test/es-module/test-require-module-dynamic-import-3.js create mode 100644 test/es-module/test-require-module-dynamic-import-4.js create mode 100644 test/fixtures/es-modules/require-and-import/load.cjs create mode 100644 test/fixtures/es-modules/require-and-import/load.mjs create mode 100644 test/fixtures/es-modules/require-and-import/node_modules/dep/mod.js create mode 100644 test/fixtures/es-modules/require-and-import/node_modules/dep/package.json diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index 418bbbfe7bdf39..4b88ed67703f66 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -316,7 +316,7 @@ class ModuleLoader { * @param {string} specifier Specifier of the the imported module. * @param {string} parentURL Where the import comes from. * @param {object} importAttributes import attributes from the import statement. - * @returns {ModuleWrap} + * @returns {ModuleJobBase} */ getModuleWrapForRequire(specifier, parentURL, importAttributes) { assert(getOptionValue('--experimental-require-module')); @@ -355,7 +355,7 @@ class ModuleLoader { // completed (e.g. the require call is lazy) so it's okay. We will return the // module now and check asynchronicity of the entire graph later, after the // graph is instantiated. - return job.module; + return job; } defaultLoadSync ??= require('internal/modules/esm/load').defaultLoadSync; @@ -403,7 +403,7 @@ class ModuleLoader { job = new ModuleJobSync(this, url, importAttributes, wrap, isMain, inspectBrk); this.loadCache.set(url, importAttributes.type, job); - return job.module; + return job; } /** diff --git a/lib/internal/modules/esm/module_job.js b/lib/internal/modules/esm/module_job.js index a9e0f72579b6f8..4bb6a72c72aa06 100644 --- a/lib/internal/modules/esm/module_job.js +++ b/lib/internal/modules/esm/module_job.js @@ -18,6 +18,9 @@ const { StringPrototypeSplit, StringPrototypeStartsWith, } = primordials; +let debug = require('internal/util/debuglog').debuglog('esm', (fn) => { + debug = fn; +}); const { ModuleWrap, kEvaluated } = internalBinding('module_wrap'); @@ -48,8 +51,7 @@ const isCommonJSGlobalLikeNotDefinedError = (errorMessage) => ); class ModuleJobBase { - constructor(loader, url, importAttributes, moduleWrapMaybePromise, isMain, inspectBrk) { - this.loader = loader; + constructor(url, importAttributes, moduleWrapMaybePromise, isMain, inspectBrk) { this.importAttributes = importAttributes; this.isMain = isMain; this.inspectBrk = inspectBrk; @@ -62,11 +64,13 @@ class ModuleJobBase { /* A ModuleJob tracks the loading of a single Module, and the ModuleJobs of * its dependencies, over time. */ class ModuleJob extends ModuleJobBase { + #loader = null; // `loader` is the Loader instance used for loading dependencies. constructor(loader, url, importAttributes = { __proto__: null }, moduleProvider, isMain, inspectBrk, sync = false) { const modulePromise = ReflectApply(moduleProvider, loader, [url, isMain]); - super(loader, url, importAttributes, modulePromise, isMain, inspectBrk); + super(url, importAttributes, modulePromise, isMain, inspectBrk); + this.#loader = loader; // Expose the promise to the ModuleWrap directly for linking below. // `this.module` is also filled in below. this.modulePromise = modulePromise; @@ -89,7 +93,8 @@ class ModuleJob extends ModuleJobBase { // these `link` callbacks depending on each other. const dependencyJobs = []; const promises = this.module.link(async (specifier, attributes) => { - const job = await this.loader.getModuleJob(specifier, url, attributes); + const job = await this.#loader.getModuleJob(specifier, url, attributes); + debug(`async link() ${this.url} -> ${specifier}`, job); ArrayPrototypePush(dependencyJobs, job); return job.modulePromise; }); @@ -121,6 +126,8 @@ class ModuleJob extends ModuleJobBase { async _instantiate() { const jobsInGraph = new SafeSet(); const addJobsToDependencyGraph = async (moduleJob) => { + debug(`async addJobsToDependencyGraph() ${this.url}`, moduleJob); + if (jobsInGraph.has(moduleJob)) { return; } @@ -156,7 +163,7 @@ class ModuleJob extends ModuleJobBase { const { 1: childSpecifier, 2: name } = RegExpPrototypeExec( /module '(.*)' does not provide an export named '(.+)'/, e.message); - const { url: childFileURL } = await this.loader.resolve( + const { url: childFileURL } = await this.#loader.resolve( childSpecifier, parentFileUrl, kEmptyObject, @@ -167,7 +174,7 @@ class ModuleJob extends ModuleJobBase { // in the import attributes and some formats require them; but we only // care about CommonJS for the purposes of this error message. ({ format } = - await this.loader.load(childFileURL)); + await this.#loader.load(childFileURL)); } catch { // Continue regardless of error. } @@ -257,18 +264,27 @@ class ModuleJob extends ModuleJobBase { // All the steps are ensured to be synchronous and it throws on instantiating // an asynchronous graph. class ModuleJobSync extends ModuleJobBase { + #loader = null; constructor(loader, url, importAttributes, moduleWrap, isMain, inspectBrk) { - super(loader, url, importAttributes, moduleWrap, isMain, inspectBrk, true); + super(url, importAttributes, moduleWrap, isMain, inspectBrk, true); assert(this.module instanceof ModuleWrap); + this.#loader = loader; const moduleRequests = this.module.getModuleRequestsSync(); + const linked = []; for (let i = 0; i < moduleRequests.length; ++i) { const { 0: specifier, 1: attributes } = moduleRequests[i]; - const wrap = this.loader.getModuleWrapForRequire(specifier, url, attributes); + const job = this.#loader.getModuleWrapForRequire(specifier, url, attributes); const isLast = (i === moduleRequests.length - 1); // TODO(joyeecheung): make the resolution callback deal with both promisified // an raw module wraps, then we don't need to wrap it with a promise here. - this.module.cacheResolvedWrapsSync(specifier, PromiseResolve(wrap), isLast); + this.module.cacheResolvedWrapsSync(specifier, PromiseResolve(job.module), isLast); + ArrayPrototypePush(linked, job); } + this.linked = linked; + } + + get modulePromise() { + return PromiseResolve(this.module); } async run() { diff --git a/test/es-module/test-require-module-dynamic-import-3.js b/test/es-module/test-require-module-dynamic-import-3.js new file mode 100644 index 00000000000000..7a5fbf1a137f96 --- /dev/null +++ b/test/es-module/test-require-module-dynamic-import-3.js @@ -0,0 +1,14 @@ +// Flags: --experimental-require-module +'use strict'; + +// This tests that previously synchronously loaded submodule can still +// be loaded by dynamic import(). + +const common = require('../common'); +const assert = require('assert'); + +(async () => { + const required = require('../fixtures/es-modules/require-and-import/load.cjs'); + const imported = await import('../fixtures/es-modules/require-and-import/load.mjs'); + assert.deepStrictEqual({ ...required }, { ...imported }); +})().then(common.mustCall()); diff --git a/test/es-module/test-require-module-dynamic-import-4.js b/test/es-module/test-require-module-dynamic-import-4.js new file mode 100644 index 00000000000000..414cd70d82d33a --- /dev/null +++ b/test/es-module/test-require-module-dynamic-import-4.js @@ -0,0 +1,14 @@ +// Flags: --experimental-require-module +'use strict'; + +// This tests that previously asynchronously loaded submodule can still +// be loaded by require(). + +const common = require('../common'); +const assert = require('assert'); + +(async () => { + const imported = await import('../fixtures/es-modules/require-and-import/load.mjs'); + const required = require('../fixtures/es-modules/require-and-import/load.cjs'); + assert.deepStrictEqual({ ...required }, { ...imported }); +})().then(common.mustCall()); diff --git a/test/fixtures/es-modules/require-and-import/load.cjs b/test/fixtures/es-modules/require-and-import/load.cjs new file mode 100644 index 00000000000000..ec9c535a04352d --- /dev/null +++ b/test/fixtures/es-modules/require-and-import/load.cjs @@ -0,0 +1,2 @@ +module.exports = require('dep'); + diff --git a/test/fixtures/es-modules/require-and-import/load.mjs b/test/fixtures/es-modules/require-and-import/load.mjs new file mode 100644 index 00000000000000..f5d2135d4dca44 --- /dev/null +++ b/test/fixtures/es-modules/require-and-import/load.mjs @@ -0,0 +1,2 @@ +export * from 'dep'; + diff --git a/test/fixtures/es-modules/require-and-import/node_modules/dep/mod.js b/test/fixtures/es-modules/require-and-import/node_modules/dep/mod.js new file mode 100644 index 00000000000000..0554fa3d5c3bfb --- /dev/null +++ b/test/fixtures/es-modules/require-and-import/node_modules/dep/mod.js @@ -0,0 +1,2 @@ +export const hello = 'world'; + diff --git a/test/fixtures/es-modules/require-and-import/node_modules/dep/package.json b/test/fixtures/es-modules/require-and-import/node_modules/dep/package.json new file mode 100644 index 00000000000000..7aeabf8c45119d --- /dev/null +++ b/test/fixtures/es-modules/require-and-import/node_modules/dep/package.json @@ -0,0 +1,5 @@ +{ + "type": "module", + "main": "mod.js" +} +