module: fix submodules loaded by require() and import()

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

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

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';
+