module: refine module type mismatch error cases

PR-URL: https://github.com/nodejs/node/pull/35426
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>

5 files changed, 55 insertions, 26 deletions

diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js
index 6f47bbc7144..9b5752e1d26 100644
--- a/lib/internal/modules/cjs/loader.js
+++ b/lib/internal/modules/cjs/loader.js
@@ -72,7 +72,6 @@ const fs = require('fs');
 const internalFS = require('internal/fs/utils');
 const path = require('path');
 const { sep } = path;
-const { emitWarningSync } = require('internal/process/warning');
 const { internalModuleStat } = internalBinding('fs');
 const packageJsonReader = require('internal/modules/package_json_reader');
 const { safeGetenv } = internalBinding('credentials');
@@ -115,6 +114,7 @@ const {
 } = require('internal/util/types');
 
 const asyncESM = require('internal/process/esm_loader');
+const { enrichCJSError } = require('internal/modules/esm/translators');
 const { kEvaluated } = internalBinding('module_wrap');
 const {
   encodedSepRegEx,
@@ -129,28 +129,6 @@ const relativeResolveCache = ObjectCreate(null);
 let requireDepth = 0;
 let statCache = null;
 
-function enrichCJSError(err) {
-  const stack = err.stack.split('\n');
-
-  const lineWithErr = stack[1];
-
-  /*
-    The regular expression below targets the most common import statement
-    usage. However, some cases are not matching, cases like import statement
-    after a comment block and/or after a variable definition.
-  */
-  if (err.message.startsWith('Unexpected token \'export\'') ||
-    (RegExpPrototypeTest(/^\s*import(?=[ {'"*])\s*(?![ (])/, lineWithErr))) {
-    // Emit the warning synchronously because we are in the middle of handling
-    // a SyntaxError that will throw and likely terminate the process before an
-    // asynchronous warning would be emitted.
-    emitWarningSync(
-      'To load an ES module, set "type": "module" in the package.json or use ' +
-      'the .mjs extension.'
-    );
-  }
-}
-
 function stat(filename) {
   filename = path.toNamespacedPath(filename);
   if (statCache !== null) {
diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js
index efbb5d92d7e..94eb275a689 100644
--- a/lib/internal/modules/esm/translators.js
+++ b/lib/internal/modules/esm/translators.js
@@ -9,9 +9,12 @@ const {
   ObjectKeys,
   PromisePrototypeCatch,
   PromiseReject,
+  RegExpPrototypeTest,
   SafeMap,
   SafeSet,
   StringPrototypeReplace,
+  StringPrototypeSplit,
+  StringPrototypeStartsWith,
 } = primordials;
 
 let _TYPES = null;
@@ -54,9 +57,11 @@ const experimentalImportMetaResolve =
     getOptionValue('--experimental-import-meta-resolve');
 const asyncESM = require('internal/process/esm_loader');
 const cjsParse = require('internal/deps/cjs-module-lexer/lexer');
+const { emitWarningSync } = require('internal/process/warning');
 
 const translators = new SafeMap();
 exports.translators = translators;
+exports.enrichCJSError = enrichCJSError;
 
 let DECODER = null;
 function assertBufferSource(body, allowString, hookName) {
@@ -130,6 +135,25 @@ translators.set('module', async function moduleStrategy(url) {
   return module;
 });
 
+function enrichCJSError(err) {
+  const stack = StringPrototypeSplit(err.stack, '\n');
+  /*
+  * The regular expression below targets the most common import statement
+  * usage. However, some cases are not matching, cases like import statement
+  * after a comment block and/or after a variable definition.
+  */
+  if (StringPrototypeStartsWith(err.message, 'Unexpected token \'export\'') ||
+    RegExpPrototypeTest(/^\s*import(?=[ {'"*])\s*(?![ (])/, stack[1])) {
+    // Emit the warning synchronously because we are in the middle of handling
+    // a SyntaxError that will throw and likely terminate the process before an
+    // asynchronous warning would be emitted.
+    emitWarningSync(
+      'To load an ES module, set "type": "module" in the package.json or use ' +
+      'the .mjs extension.'
+    );
+  }
+}
+
 // Strategy for loading a node-style CommonJS module
 const isWindows = process.platform === 'win32';
 const winSepRegEx = /\//g;
@@ -152,7 +176,12 @@ translators.set('commonjs', async function commonjsStrategy(url, isMain) {
       exports = asyncESM.ESMLoader.cjsCache.get(module);
       asyncESM.ESMLoader.cjsCache.delete(module);
     } else {
-      exports = CJSModule._load(filename, undefined, isMain);
+      try {
+        exports = CJSModule._load(filename, undefined, isMain);
+      } catch (err) {
+        enrichCJSError(err);
+        throw err;
+      }
     }
 
     for (const exportName of exportNames) {
@@ -190,7 +219,13 @@ function cjsPreparseModuleExports(filename) {
     source = readFileSync(filename, 'utf8');
   } catch {}
 
-  const { exports, reexports } = cjsParse(source || '');
+  let exports, reexports;
+  try {
+    ({ exports, reexports } = cjsParse(source || ''));
+  } catch {
+    exports = [];
+    reexports = [];
+  }
 
   const exportNames = new SafeSet(exports);
 
diff --git a/test/es-module/test-esm-cjs-exports.js b/test/es-module/test-esm-cjs-exports.js
index 37aa70d3880..7db2c6fdb59 100644
--- a/test/es-module/test-esm-cjs-exports.js
+++ b/test/es-module/test-esm-cjs-exports.js
@@ -7,7 +7,7 @@ const assert = require('assert');
 
 const entry = fixtures.path('/es-modules/cjs-exports.mjs');
 
-const child = spawn(process.execPath, [entry]);
+let child = spawn(process.execPath, [entry]);
 child.stderr.setEncoding('utf8');
 let stdout = '';
 child.stdout.setEncoding('utf8');
@@ -19,3 +19,17 @@ child.on('close', common.mustCall((code, signal) => {
   assert.strictEqual(signal, null);
   assert.strictEqual(stdout, 'ok\n');
 }));
+
+const entryInvalid = fixtures.path('/es-modules/cjs-exports-invalid.mjs');
+child = spawn(process.execPath, [entryInvalid]);
+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.includes('Warning: To load an ES module'));
+  assert.ok(stderr.includes('Unexpected token \'export\''));
+}));
diff --git a/test/fixtures/es-modules/cjs-exports-invalid.mjs b/test/fixtures/es-modules/cjs-exports-invalid.mjs
new file mode 100644
index 00000000000..67428fc27c2
--- /dev/null
+++ b/test/fixtures/es-modules/cjs-exports-invalid.mjs
@@ -0,0 +1 @@
+import cjs from './invalid-cjs.js';
diff --git a/test/fixtures/es-modules/invalid-cjs.js b/test/fixtures/es-modules/invalid-cjs.js
new file mode 100644
index 00000000000..15cae6690f1
--- /dev/null
+++ b/test/fixtures/es-modules/invalid-cjs.js
@@ -0,0 +1 @@
+export var name = 5;