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: nodejs#52487
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>

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;
   }
 
   /**

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() {

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());

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());

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

@@ -0,0 +1,2 @@
+module.exports = require('dep');
+

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

@@ -0,0 +1,2 @@
+export * from 'dep';
+