[v20.x] backport require(esm) bugfixes and improvements

lib/internal/errors.js

@@ -1692,9 +1692,18 @@ E('ERR_PARSE_ARGS_UNKNOWN_OPTION', (option, allowPositionals) => {
 E('ERR_PERFORMANCE_INVALID_TIMESTAMP',
   '%d is not a valid timestamp', TypeError);
 E('ERR_PERFORMANCE_MEASURE_INVALID_OPTIONS', '%s', TypeError);
-E('ERR_REQUIRE_ASYNC_MODULE', 'require() cannot be used on an ESM ' +
-  'graph with top-level await. Use import() instead. To see where the' +
-  ' top-level await comes from, use --experimental-print-required-tla.', Error);
+E('ERR_REQUIRE_ASYNC_MODULE', function(filename, parentFilename) {
+  let message = 'require() cannot be used on an ESM ' +
+    'graph with top-level await. Use import() instead. To see where the' +
+    ' top-level await comes from, use --experimental-print-required-tla.';
+  if (parentFilename) {
+    message += `\n  From ${parentFilename} `;
+  }
+  if (filename) {
+    message += `\n  Requiring ${filename} `;
+  }
+  return message;
+}, Error);
 E('ERR_REQUIRE_CYCLE_MODULE', '%s', Error);
 E('ERR_REQUIRE_ESM',
   function(filename, hasEsmSyntax, parentPath = null, packageJsonPath = null) {

lib/internal/modules/cjs/loader.js

@@ -101,6 +101,9 @@ const kIsMainSymbol = Symbol('kIsMainSymbol');
 const kIsCachedByESMLoader = Symbol('kIsCachedByESMLoader');
 const kRequiredModuleSymbol = Symbol('kRequiredModuleSymbol');
 const kIsExecuting = Symbol('kIsExecuting');
+
+const kFormat = Symbol('kFormat');
+
 // Set first due to cycle with ESM loader functions.
 module.exports = {
   kModuleSource,
@@ -438,10 +441,6 @@ function initializeCJS() {
   // TODO(joyeecheung): deprecate this in favor of a proper hook?
   Module.runMain =
     require('internal/modules/run_main').executeUserEntryPoint;
-
-  if (getOptionValue('--experimental-require-module')) {
-    Module._extensions['.mjs'] = loadESMFromCJS;
-  }
 }
 
 // Given a module name, and a list of paths to test, returns the first
@@ -651,14 +650,7 @@ function resolveExports(nmPath, request) {
 // We don't cache this in case user extends the extensions.
 function getDefaultExtensions() {
   const extensions = ObjectKeys(Module._extensions);
-  if (!getOptionValue('--experimental-require-module')) {
-    return extensions;
-  }
-  // If the .mjs extension is added by --experimental-require-module,
-  // remove it from the supported default extensions to maintain
-  // compatibility.
-  // TODO(joyeecheung): allow both .mjs and .cjs?
-  return ArrayPrototypeFilter(extensions, (ext) => ext !== '.mjs' || Module._extensions['.mjs'] !== loadESMFromCJS);
+  return extensions;
 }
 
 /**
@@ -1270,10 +1262,6 @@ Module.prototype.load = function(filename) {
   this.paths = Module._nodeModulePaths(path.dirname(filename));
 
   const extension = findLongestRegisteredExtension(filename);
-  // allow .mjs to be overridden
-  if (StringPrototypeEndsWith(filename, '.mjs') && !Module._extensions['.mjs']) {
-    throw new ERR_REQUIRE_ESM(filename, true);
-  }
 
   Module._extensions[extension](this, filename);
   this.loaded = true;
@@ -1309,9 +1297,10 @@ let requireModuleWarningMode;
  * Resolve and evaluate it synchronously as ESM if it's ESM.
  * @param {Module} mod CJS module instance
  * @param {string} filename Absolute path of the file.
+ * @param {string} format Format of the module. If it had types, this would be what it is after type-stripping.
+ * @param {string} source Source the module. If it had types, this would have the type stripped.
  */
-function loadESMFromCJS(mod, filename) {
-  const source = getMaybeCachedSource(mod, filename);
+function loadESMFromCJS(mod, filename, format, source) {
   const cascadedLoader = require('internal/modules/esm/loader').getOrInitializeCascadedLoader();
   const isMain = mod[kIsMainSymbol];
   if (isMain) {
@@ -1487,7 +1476,9 @@ function wrapSafe(filename, content, cjsModuleInstance, format) {
  * `exports`) to the file. Returns exception, if any.
  * @param {string} content The source code of the module
  * @param {string} filename The file path of the module
- * @param {'module'|'commonjs'|undefined} format Intended format of the module.
+ * @param {
+ *    'module'|'commonjs'|'commonjs-typescript'|'module-typescript'
+ * } format Intended format of the module.
  */
 Module.prototype._compile = function(content, filename, format) {
   let moduleURL;
@@ -1509,9 +1500,7 @@ Module.prototype._compile = function(content, filename, format) {
   }
 
   if (format === 'module') {
-    // Pass the source into the .mjs extension handler indirectly through the cache.
-    this[kModuleSource] = content;
-    loadESMFromCJS(this, filename);
+    loadESMFromCJS(this, filename, format, content);
     return;
   }
 
@@ -1539,22 +1528,72 @@ Module.prototype._compile = function(content, filename, format) {
 
 /**
  * Get the source code of a module, using cached ones if it's cached.
+ * After this returns, mod[kFormat], mod[kModuleSource] and mod[kURL] will be set.
  * @param {Module} mod Module instance whose source is potentially already cached.
  * @param {string} filename Absolute path to the file of the module.
- * @returns {string}
+ * @returns {{source: string, format?: string}}
  */
-function getMaybeCachedSource(mod, filename) {
-  // If already analyzed the source, then it will be cached.
-  let content;
-  if (mod[kModuleSource] !== undefined) {
-    content = mod[kModuleSource];
+function loadSource(mod, filename, formatFromNode) {
+  if (formatFromNode !== undefined) {
+    mod[kFormat] = formatFromNode;
+  }
+  const format = mod[kFormat];
+
+  let source = mod[kModuleSource];
+  if (source !== undefined) {
     mod[kModuleSource] = undefined;
   } else {
     // TODO(joyeecheung): we can read a buffer instead to speed up
     // compilation.
-    content = fs.readFileSync(filename, 'utf8');
+    source = fs.readFileSync(filename, 'utf8');
+  }
+  return { source, format };
+}
+
+function reconstructErrorStack(err, parentPath, parentSource) {
+  const errLine = StringPrototypeSplit(
+    StringPrototypeSlice(err.stack, StringPrototypeIndexOf(
+      err.stack, '    at ')), '\n', 1)[0];
+  const { 1: line, 2: col } =
+    RegExpPrototypeExec(/(\d+):(\d+)\)/, errLine) || [];
+  if (line && col) {
+    const srcLine = StringPrototypeSplit(parentSource, '\n')[line - 1];
+    const frame = `${parentPath}:${line}\n${srcLine}\n${StringPrototypeRepeat(' ', col - 1)}^\n`;
+    setArrowMessage(err, frame);
+  }
+}
+
+/**
+ * Generate the legacy ERR_REQUIRE_ESM for the cases where require(esm) is disabled.
+ * @param {Module} mod The module being required.
+ * @param {undefined|object} pkg Data of the nearest package.json of the module.
+ * @param {string} content Source code of the module.
+ * @param {string} filename Filename of the module
+ * @returns {Error}
+ */
+function getRequireESMError(mod, pkg, content, filename) {
+  // This is an error path because `require` of a `.js` file in a `"type": "module"` scope is not allowed.
+  const parent = mod[kModuleParent];
+  const parentPath = parent?.filename;
+  const packageJsonPath = pkg?.path ? path.resolve(pkg.path, 'package.json') : null;
+  const usesEsm = containsModuleSyntax(content, filename);
+  const err = new ERR_REQUIRE_ESM(filename, usesEsm, parentPath,
+                                  packageJsonPath);
+  // Attempt to reconstruct the parent require frame.
+  const parentModule = Module._cache[parentPath];
+  if (parentModule) {
+    let parentSource;
+    try {
+      ({ source: parentSource } = loadSource(parentModule, parentPath));
+    } catch {
+      // Continue regardless of error.
+    }
+    if (parentSource) {
+      // TODO(joyeecheung): trim off internal frames from the stack.
+      reconstructErrorStack(err, parentPath, parentSource);
+    }
   }
-  return content;
+  return err;
 }
 
 /**
@@ -1563,57 +1602,25 @@ function getMaybeCachedSource(mod, filename) {
  * @param {string} filename The file path of the module
  */
 Module._extensions['.js'] = function(module, filename) {
-  // If already analyzed the source, then it will be cached.
-  const content = getMaybeCachedSource(module, filename);
-
-  let format;
-  if (StringPrototypeEndsWith(filename, '.js')) {
-    const pkg = packageJsonReader.readPackageScope(filename) || { __proto__: null };
-    // Function require shouldn't be used in ES modules.
-    if (pkg.data?.type === 'module') {
-      if (getOptionValue('--experimental-require-module')) {
-        module._compile(content, filename, 'module');
-        return;
-      }
-
-      // This is an error path because `require` of a `.js` file in a `"type": "module"` scope is not allowed.
-      const parent = module[kModuleParent];
-      const parentPath = parent?.filename;
-      const packageJsonPath = path.resolve(pkg.path, 'package.json');
-      const usesEsm = containsModuleSyntax(content, filename);
-      const err = new ERR_REQUIRE_ESM(filename, usesEsm, parentPath,
-                                      packageJsonPath);
-      // Attempt to reconstruct the parent require frame.
-      if (Module._cache[parentPath]) {
-        let parentSource;
-        try {
-          parentSource = fs.readFileSync(parentPath, 'utf8');
-        } catch {
-          // Continue regardless of error.
-        }
-        if (parentSource) {
-          const errLine = StringPrototypeSplit(
-            StringPrototypeSlice(err.stack, StringPrototypeIndexOf(
-              err.stack, '    at ')), '\n', 1)[0];
-          const { 1: line, 2: col } =
-              RegExpPrototypeExec(/(\d+):(\d+)\)/, errLine) || [];
-          if (line && col) {
-            const srcLine = StringPrototypeSplit(parentSource, '\n')[line - 1];
-            const frame = `${parentPath}:${line}\n${srcLine}\n${
-              StringPrototypeRepeat(' ', col - 1)}^\n`;
-            setArrowMessage(err, frame);
-          }
-        }
-      }
-      throw err;
-    } else if (pkg.data?.type === 'commonjs') {
-      format = 'commonjs';
-    }
-  } else if (StringPrototypeEndsWith(filename, '.cjs')) {
+  let format, pkg;
+  if (StringPrototypeEndsWith(filename, '.cjs')) {
     format = 'commonjs';
+  } else if (StringPrototypeEndsWith(filename, '.mjs')) {
+    format = 'module';
+  } else if (StringPrototypeEndsWith(filename, '.js')) {
+    pkg = packageJsonReader.readPackageScope(filename) || { __proto__: null };
+    const typeFromPjson = pkg.data?.type;
+    if (typeFromPjson === 'module' || typeFromPjson === 'commonjs' || !typeFromPjson) {
+      format = typeFromPjson;
+    }
   }
-
-  module._compile(content, filename, format);
+  const { source, format: loadedFormat } = loadSource(module, filename, format);
+  // Function require shouldn't be used in ES modules when require(esm) is disabled.
+  if (loadedFormat === 'module' && !getOptionValue('--experimental-require-module')) {
+    const err = getRequireESMError(module, pkg, source, filename);
+    throw err;
+  }
+  module._compile(source, filename, loadedFormat);
 };
 
 /**

lib/internal/modules/esm/loader.js

@@ -36,12 +36,23 @@ const {
   getDefaultConditions,
 } = require('internal/modules/esm/utils');
 const { kImplicitAssertType } = require('internal/modules/esm/assert');
-const { ModuleWrap, kEvaluating, kEvaluated } = internalBinding('module_wrap');
+const {
+  ModuleWrap,
+  kEvaluated,
+  kEvaluating,
+  kInstantiated,
+  kErrored,
+  throwIfPromiseRejected,
+} = internalBinding('module_wrap');
 const {
   urlToFilename,
 } = require('internal/modules/helpers');
 let defaultResolve, defaultLoad, defaultLoadSync, importMetaInitializer;
 
+let debug = require('internal/util/debuglog').debuglog('esm', (fn) => {
+  debug = fn;
+});
+
 /**
  * @typedef {import('./hooks.js').HooksProxy} HooksProxy
  * @typedef {import('./module_job.js').ModuleJobBase} ModuleJobBase
@@ -75,6 +86,23 @@ function getTranslators() {
   return translators;
 }
 
+/**
+ * Generate message about potential race condition caused by requiring a cached module that has started
+ * async linking.
+ * @param {string} filename Filename of the module being required.
+ * @param {string|undefined} parentFilename Filename of the module calling require().
+ * @returns {string} Error message.
+ */
+function getRaceMessage(filename, parentFilename) {
+  let raceMessage = `Cannot require() ES Module ${filename} because it is not yet fully loaded. `;
+  raceMessage += 'This may be caused by a race condition if the module is simultaneously dynamically ';
+  raceMessage += 'import()-ed via Promise.all(). Try await-ing the import() sequentially in a loop instead.';
+  if (parentFilename) {
+    raceMessage += ` (from ${parentFilename})`;
+  }
+  return raceMessage;
+}
+
 /**
  * @type {HooksProxy}
  * Multiple loader instances exist for various, specific reasons (see code comments at site).
@@ -295,20 +323,58 @@ class ModuleLoader {
     //    TODO(joyeecheung): ensure that imported synchronous graphs are evaluated
     //    synchronously so that any previously imported synchronous graph is already
     //    evaluated at this point.
+    // TODO(joyeecheung): add something similar to CJS loader's requireStack to help
+    // debugging the the problematic links in the graph for import.
+    debug('importSyncForRequire', parent?.filename, '->', filename, job);
     if (job !== undefined) {
       mod[kRequiredModuleSymbol] = job.module;
+      const parentFilename = urlToFilename(parent?.filename);
+      // TODO(node:55782): this race may stop to happen when the ESM resolution and loading become synchronous.
+      if (!job.module) {
+        assert.fail(getRaceMessage(filename, parentFilename));
+      }
       if (job.module.async) {
-        throw new ERR_REQUIRE_ASYNC_MODULE();
+        throw new ERR_REQUIRE_ASYNC_MODULE(filename, parentFilename);
       }
-      if (job.module.getStatus() !== kEvaluated) {
-        const parentFilename = urlToFilename(parent?.filename);
-        let message = `Cannot require() ES Module ${filename} in a cycle.`;
-        if (parentFilename) {
-          message += ` (from ${parentFilename})`;
-        }
-        throw new ERR_REQUIRE_CYCLE_MODULE(message);
+      const status = job.module.getStatus();
+      debug('Module status', job, status);
+      if (status === kEvaluated) {
+        return { wrap: job.module, namespace: job.module.getNamespaceSync(filename, parentFilename) };
+      } else if (status === kInstantiated) {
+        // When it's an async job cached by another import request,
+        // which has finished linking but has not started its
+        // evaluation because the async run() task would be later
+        // in line. Then start the evaluation now with runSync(), which
+        // is guaranteed to finish by the time the other run() get to it,
+        // and the other task would just get the cached evaluation results,
+        // similar to what would happen when both are async.
+        mod[kRequiredModuleSymbol] = job.module;
+        const { namespace } = job.runSync(parent);
+        return { wrap: job.module, namespace: namespace || job.module.getNamespace() };
+      } else if (status === kErrored) {
+        // If the module was previously imported and errored, throw the error.
+        throw job.module.getError();
+      }
+      // When the cached async job have already encountered a linking
+      // error that gets wrapped into a rejection, but is still later
+      // in line to throw on it, just unwrap and throw the linking error
+      // from require().
+      if (job.instantiated) {
+        throwIfPromiseRejected(job.instantiated);
       }
-      return { wrap: job.module, namespace: job.module.getNamespaceSync() };
+      if (status !== kEvaluating) {
+        assert.fail(`Unexpected module status ${status}. ` +
+                    getRaceMessage(filename, parentFilename));
+      }
+      let message = `Cannot require() ES Module ${filename} in a cycle.`;
+      if (parentFilename) {
+        message += ` (from ${parentFilename})`;
+      }
+      message += ' A cycle involving require(esm) is not allowed to maintain ';
+      message += 'invariants mandated by the ECMAScript specification. ';
+      message += 'Try making at least part of the dependency in the graph lazily loaded.';
+      throw new ERR_REQUIRE_CYCLE_MODULE(message);
+
     }
     // TODO(joyeecheung): refactor this so that we pre-parse in C++ and hit the
     // cache here, or use a carrier object to carry the compiled module script
@@ -320,7 +386,7 @@ class ModuleLoader {
     job = new ModuleJobSync(this, url, kEmptyObject, wrap, isMain, inspectBrk);
     this.loadCache.set(url, kImplicitAssertType, job);
     mod[kRequiredModuleSymbol] = job.module;
-    return { wrap: job.module, namespace: job.runSync().namespace };
+    return { wrap: job.module, namespace: job.runSync(parent).namespace };
   }
 
   /**