esm: refine ERR_REQUIRE_ESM errors

PR-URL: nodejs#39175 Reviewed-By: Bradley Farias <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: James M Snell <[email protected]>
jasnell · Jul 15, 2021 · e2a6399 · e2a6399
1 parent 499f693
commit e2a6399
Show file tree

Hide file tree

Showing 9 changed files with 182 additions and 63 deletions.
diff --git a/lib/internal/errors.js b/lib/internal/errors.js
@@ -14,6 +14,7 @@ const {
   AggregateError,
   ArrayFrom,
   ArrayIsArray,
+  ArrayPrototypeFilter,
   ArrayPrototypeIncludes,
   ArrayPrototypeIndexOf,
   ArrayPrototypeJoin,
@@ -788,6 +789,34 @@ const fatalExceptionStackEnhancers = {
   }
 };
 
+// Ensures the printed error line is from user code.
+let _kArrowMessagePrivateSymbol, _setHiddenValue;
+function setArrowMessage(err, arrowMessage) {
+  if (!_kArrowMessagePrivateSymbol) {
+    ({
+      arrow_message_private_symbol: _kArrowMessagePrivateSymbol,
+      setHiddenValue: _setHiddenValue,
+    } = internalBinding('util'));
+  }
+  _setHiddenValue(err, _kArrowMessagePrivateSymbol, arrowMessage);
+}
+
+// Hide stack lines before the first user code line.
+function hideInternalStackFrames(error) {
+  overrideStackTrace.set(error, (error, stackFrames) => {
+    let frames = stackFrames;
+    if (typeof stackFrames === 'object') {
+      frames = ArrayPrototypeFilter(
+        stackFrames,
+        (frm) => !StringPrototypeStartsWith(frm.getFileName(),
+                                            'node:internal')
+      );
+    }
+    ArrayPrototypeUnshift(frames, error);
+    return ArrayPrototypeJoin(frames, '\n    at ');
+  });
+}
+
 // Node uses an AbortError that isn't exactly the same as the DOMException
 // to make usage of the error in userland and readable-stream easier.
 // It is a regular error with `.code` and `.name`.
@@ -806,8 +835,10 @@ module.exports = {
   exceptionWithHostPort,
   getMessage,
   hideStackFrames,
+  hideInternalStackFrames,
   isErrorStackTraceLimitWritable,
   isStackOverflowError,
+  setArrowMessage,
   connResetException,
   uvErrmapGet,
   uvException,
@@ -842,6 +873,7 @@ module.exports = {
 // Note: Please try to keep these in alphabetical order
 //
 // Note: Node.js specific errors must begin with the prefix ERR_
+
 E('ERR_AMBIGUOUS_ARGUMENT', 'The "%s" argument is ambiguous. %s', TypeError);
 E('ERR_ARG_NOT_ITERABLE', '%s must be iterable', TypeError);
 E('ERR_ASSERTION', '%s', Error);
@@ -1406,23 +1438,32 @@ E('ERR_PERFORMANCE_INVALID_TIMESTAMP',
   '%d is not a valid timestamp', TypeError);
 E('ERR_PERFORMANCE_MEASURE_INVALID_OPTIONS', '%s', TypeError);
 E('ERR_REQUIRE_ESM',
-  (filename, parentPath = null, packageJsonPath = null) => {
-    let msg = `Must use import to load ES Module: ${filename}`;
-    if (parentPath && packageJsonPath) {
-      const path = require('path');
-      const basename = path.basename(filename) === path.basename(parentPath) ?
-        filename : path.basename(filename);
-      msg +=
-        '\nrequire() of ES modules is not supported.\nrequire() of ' +
-        `${filename} from ${parentPath} ` +
-        'is an ES module file as it is a .js file whose nearest parent ' +
-        'package.json contains "type": "module" which defines all .js ' +
-        'files in that package scope as ES modules.\nInstead rename ' +
-        `${basename} to end in .cjs, change the requiring code to use ` +
-        'import(), or remove "type": "module" from ' +
-        `${packageJsonPath}.\n`;
+  function(filename, hasEsmSyntax, parentPath = null, packageJsonPath = null) {
+    hideInternalStackFrames(this);
+    let msg = `require() of ES Module ${filename}${parentPath ? ` from ${
+      parentPath}` : ''} not supported.`;
+    if (!packageJsonPath) {
+      if (StringPrototypeEndsWith(filename, '.mjs'))
+        msg += `\nInstead change the require of ${filename} to a dynamic ` +
+            'import() which is available in all CommonJS modules.';
+      return msg;
+    }
+    const path = require('path');
+    const basename = path.basename(filename) === path.basename(parentPath) ?
+      filename : path.basename(filename);
+    if (hasEsmSyntax) {
+      msg += `\nInstead change the require of ${basename} in ${parentPath} to` +
+        ' a dynamic import() which is available in all CommonJS modules.';
       return msg;
     }
+    msg += `\n${basename} is treated as an ES module file as it is a .js ` +
+      'file whose nearest parent package.json contains "type": "module" ' +
+      'which declares all .js files in that package scope as ES modules.' +
+      `\nInstead rename ${basename} to end in .cjs, change the requiring ` +
+      'code to use dynamic import() which is available in all CommonJS ' +
+      'modules, or change "type": "module" to "type": "commonjs" in ' +
+      `${packageJsonPath} to treat all .js files as CommonJS (using .mjs for ` +
+      'all ES modules instead).\n';
     return msg;
   }, Error);
 E('ERR_SCRIPT_EXECUTION_INTERRUPTED',

diff --git a/lib/internal/modules/cjs/helpers.js b/lib/internal/modules/cjs/helpers.js
@@ -3,6 +3,7 @@
 const {
   ArrayPrototypeForEach,
   ArrayPrototypeJoin,
+  ArrayPrototypeSome,
   ObjectDefineProperty,
   ObjectPrototypeHasOwnProperty,
   SafeMap,
@@ -184,9 +185,26 @@ function normalizeReferrerURL(referrer) {
   return new URL(referrer).href;
 }
 
+// For error messages only - used to check if ESM syntax is in use.
+function hasEsmSyntax(code) {
+  const parser = require('internal/deps/acorn/acorn/dist/acorn').Parser;
+  let root;
+  try {
+    root = parser.parse(code, { sourceType: 'module', ecmaVersion: 'latest' });
+  } catch {
+    return false;
+  }
+
+  return ArrayPrototypeSome(root.body, (stmt) =>
+    stmt.type === 'ExportNamedDeclaration' ||
+    stmt.type === 'ImportDeclaration' ||
+    stmt.type === 'ExportAllDeclaration');
+}
+
 module.exports = {
   addBuiltinLibsToObject,
   cjsConditions,
+  hasEsmSyntax,
   loadNativeModule,
   makeRequireFunction,
   normalizeReferrerURL,

diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js
@@ -48,6 +48,7 @@ const {
   Proxy,
   ReflectApply,
   ReflectSet,
+  RegExpPrototypeExec,
   RegExpPrototypeTest,
   SafeMap,
   SafeWeakMap,
@@ -58,6 +59,7 @@ const {
   StringPrototypeLastIndexOf,
   StringPrototypeIndexOf,
   StringPrototypeMatch,
+  StringPrototypeRepeat,
   StringPrototypeSlice,
   StringPrototypeSplit,
   StringPrototypeStartsWith,
@@ -88,11 +90,12 @@ const { internalModuleStat } = internalBinding('fs');
 const packageJsonReader = require('internal/modules/package_json_reader');
 const { safeGetenv } = internalBinding('credentials');
 const {
+  cjsConditions,
+  hasEsmSyntax,
+  loadNativeModule,
   makeRequireFunction,
   normalizeReferrerURL,
   stripBOM,
-  cjsConditions,
-  loadNativeModule
 } = require('internal/modules/cjs/helpers');
 const { getOptionValue } = require('internal/options');
 const preserveSymlinks = getOptionValue('--preserve-symlinks');
@@ -107,11 +110,14 @@ const policy = getOptionValue('--experimental-policy') ?
 let hasLoadedAnyUserCJSModule = false;
 
 const {
-  ERR_INVALID_ARG_VALUE,
-  ERR_INVALID_MODULE_SPECIFIER,
-  ERR_REQUIRE_ESM,
-  ERR_UNKNOWN_BUILTIN_MODULE,
-} = require('internal/errors').codes;
+  codes: {
+    ERR_INVALID_ARG_VALUE,
+    ERR_INVALID_MODULE_SPECIFIER,
+    ERR_REQUIRE_ESM,
+    ERR_UNKNOWN_BUILTIN_MODULE,
+  },
+  setArrowMessage,
+} = require('internal/errors');
 const { validateString } = require('internal/validators');
 const pendingDeprecation = getOptionValue('--pending-deprecation');
 
@@ -970,7 +976,7 @@ Module.prototype.load = function(filename) {
   const extension = findLongestRegisteredExtension(filename);
   // allow .mjs to be overridden
   if (StringPrototypeEndsWith(filename, '.mjs') && !Module._extensions['.mjs'])
-    throw new ERR_REQUIRE_ESM(filename);
+    throw new ERR_REQUIRE_ESM(filename, true);
 
   Module._extensions[extension](this, filename);
   this.loaded = true;
@@ -1102,16 +1108,6 @@ Module.prototype._compile = function(content, filename) {
 
 // Native extension for .js
 Module._extensions['.js'] = function(module, filename) {
-  if (StringPrototypeEndsWith(filename, '.js')) {
-    const pkg = readPackageScope(filename);
-    // Function require shouldn't be used in ES modules.
-    if (pkg?.data?.type === 'module') {
-      const parent = moduleParentCache.get(module);
-      const parentPath = parent?.filename;
-      const packageJsonPath = path.resolve(pkg.path, 'package.json');
-      throw new ERR_REQUIRE_ESM(filename, parentPath, packageJsonPath);
-    }
-  }
   // If already analyzed the source, then it will be cached.
   const cached = cjsParseCache.get(module);
   let content;
@@ -1121,6 +1117,39 @@ Module._extensions['.js'] = function(module, filename) {
   } else {
     content = fs.readFileSync(filename, 'utf8');
   }
+  if (StringPrototypeEndsWith(filename, '.js')) {
+    const pkg = readPackageScope(filename);
+    // Function require shouldn't be used in ES modules.
+    if (pkg?.data?.type === 'module') {
+      const parent = moduleParentCache.get(module);
+      const parentPath = parent?.filename;
+      const packageJsonPath = path.resolve(pkg.path, 'package.json');
+      const usesEsm = hasEsmSyntax(content);
+      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 {}
+        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;
+    }
+  }
   module._compile(content, filename);
 };
 

diff --git a/src/node_errors.cc b/src/node_errors.cc
@@ -215,6 +215,12 @@ void AppendExceptionLine(Environment* env,
   Local<Object> err_obj;
   if (!er.IsEmpty() && er->IsObject()) {
     err_obj = er.As<Object>();
+    // If arrow_message is already set, skip.
+    auto maybe_value = err_obj->GetPrivate(env->context(),
+                                          env->arrow_message_private_symbol());
+    Local<Value> lvalue;
+    if (!maybe_value.ToLocal(&lvalue) || lvalue->IsString())
+      return;
   }
 
   bool added_exception_line = false;

diff --git a/test/es-module/test-cjs-esm-warn.js b/test/es-module/test-cjs-esm-warn.js
@@ -6,36 +6,59 @@ const { spawn } = require('child_process');
 const assert = require('assert');
 const path = require('path');
 
-const requiring = path.resolve(fixtures.path('/es-modules/cjs-esm.js'));
-const required = path.resolve(
-  fixtures.path('/es-modules/package-type-module/cjs.js')
-);
+const requiringCjsAsEsm = path.resolve(fixtures.path('/es-modules/cjs-esm.js'));
+const requiringEsm = path.resolve(fixtures.path('/es-modules/cjs-esm-esm.js'));
 const pjson = path.resolve(
   fixtures.path('/es-modules/package-type-module/package.json')
 );
 
-const basename = 'cjs.js';
+{
+  const required = path.resolve(
+    fixtures.path('/es-modules/package-type-module/cjs.js')
+  );
+  const basename = 'cjs.js';
+  const child = spawn(process.execPath, [requiringCjsAsEsm]);
+  let stderr = '';
+  child.stderr.setEncoding('utf8');
+  child.stderr.on('data', (data) => {
+    stderr += data;
+  });
+  child.on('close', common.mustCall((code, signal) => {
+    assert.strictEqual(code, 1);
+    assert.strictEqual(signal, null);
+
+    assert.ok(stderr.replaceAll('\r', '').includes(
+      `Error [ERR_REQUIRE_ESM]: require() of ES Module ${required} from ${
+        requiringCjsAsEsm} not supported.\n`));
+    assert.ok(stderr.replaceAll('\r', '').includes(
+      `Instead rename ${basename} to end in .cjs, change the requiring ` +
+      'code to use dynamic import() which is available in all CommonJS ' +
+      `modules, or change "type": "module" to "type": "commonjs" in ${pjson} to ` +
+      'treat all .js files as CommonJS (using .mjs for all ES modules ' +
+      'instead).\n'));
+  }));
+}
 
-const child = spawn(process.execPath, [requiring]);
-let stderr = '';
-child.stderr.setEncoding('utf8');
-child.stderr.on('data', (data) => {
-  stderr += data;
-});
-child.on('close', common.mustCall((code, signal) => {
-  assert.strictEqual(code, 1);
-  assert.strictEqual(signal, null);
+{
+  const required = path.resolve(
+    fixtures.path('/es-modules/package-type-module/esm.js')
+  );
+  const basename = 'esm.js';
+  const child = spawn(process.execPath, [requiringEsm]);
+  let stderr = '';
+  child.stderr.setEncoding('utf8');
+  child.stderr.on('data', (data) => {
+    stderr += data;
+  });
+  child.on('close', common.mustCall((code, signal) => {
+    assert.strictEqual(code, 1);
+    assert.strictEqual(signal, null);
 
-  assert.ok(stderr.replace(/\r/g, '').includes(
-    `Error [ERR_REQUIRE_ESM]: Must use import to load ES Module: ${required}` +
-    '\nrequire() of ES modules is not supported.\nrequire() of ' +
-    `${required} from ${requiring} ` +
-    'is an ES module file as it is a .js file whose nearest parent ' +
-    'package.json contains "type": "module" which defines all .js ' +
-    'files in that package scope as ES modules.\nInstead rename ' +
-    `${basename} to end in .cjs, change the requiring code to use ` +
-    'import(), or remove "type": "module" from ' +
-    `${pjson}.\n`));
-  assert.ok(stderr.includes(
-    'Error [ERR_REQUIRE_ESM]: Must use import to load ES Module'));
-}));
+    assert.ok(stderr.replace(/\r/g, '').includes(
+      `Error [ERR_REQUIRE_ESM]: require() of ES Module ${required} from ${
+        requiringEsm} not supported.\n`));
+    assert.ok(stderr.replace(/\r/g, '').includes(
+      `Instead change the require of ${basename} in ${requiringEsm} to` +
+      ' a dynamic import() which is available in all CommonJS modules.\n'));
+  }));
+}
diff --git a/test/es-module/test-esm-type-flag-errors.js b/test/es-module/test-esm-type-flag-errors.js
@@ -29,8 +29,8 @@ try {
 } catch (e) {
   assert.strictEqual(e.name, 'Error');
   assert.strictEqual(e.code, 'ERR_REQUIRE_ESM');
-  assert(e.toString().match(/Must use import to load ES Module/g));
-  assert(e.message.match(/Must use import to load ES Module/g));
+  assert(e.toString().match(/require\(\) of ES Module/g));
+  assert(e.message.match(/require\(\) of ES Module/g));
 }
 
 function expect(opt = '', inputFile, want, wantsError = false) {

diff --git a/test/fixtures/es-modules/cjs-esm-esm.js b/test/fixtures/es-modules/cjs-esm-esm.js
@@ -0,0 +1 @@
+require('./package-type-module/esm.js');
diff --git a/test/fixtures/es-modules/package-type-module/esm.js b/test/fixtures/es-modules/package-type-module/esm.js
@@ -0,0 +1 @@
+export var p = 5;
diff --git a/test/parallel/test-require-mjs.js b/test/parallel/test-require-mjs.js
@@ -5,7 +5,7 @@ const assert = require('assert');
 assert.throws(
   () => require('../fixtures/es-modules/test-esm-ok.mjs'),
   {
-    message: /Must use import to load ES Module/,
+    message: /dynamic import\(\) which is available in all CommonJS modules/,
     code: 'ERR_REQUIRE_ESM'
   }
 );