From 86a0194746030dcbc651f230cf11222700da25af Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Fri, 12 Apr 2024 00:36:20 +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. --- 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 df7a26c9337c39..c6ff8932ff3b6f 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -308,7 +308,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')); @@ -347,7 +347,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; @@ -391,7 +391,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 43d9e141e04725..296dafe3ad20d6 100644 --- a/lib/internal/modules/esm/module_job.js +++ b/lib/internal/modules/esm/module_job.js @@ -19,6 +19,9 @@ const { StringPrototypeStartsWith, globalThis, } = primordials; +let debug = require('internal/util/debuglog').debuglog('esm', (fn) => { + debug = fn; +}); const { ModuleWrap, kEvaluated } = internalBinding('module_wrap'); const { @@ -53,8 +56,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; @@ -67,11 +69,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; @@ -94,7 +98,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; }); @@ -126,6 +131,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; } @@ -161,7 +168,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, @@ -172,7 +179,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. } @@ -265,18 +272,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" +} +