module: refine module type mismatch error cases

PR-URL: nodejs#35426 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Bradley Farias <[email protected]> Reviewed-By: Myles Borins <[email protected]>
guybedford · Oct 1, 2020 · 0708932 · 0708932
1 parent aab2a8d
commit 0708932
Show file tree

Hide file tree

Showing 5 changed files with 58 additions and 28 deletions.
diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js
@@ -105,6 +105,7 @@ const {
 } = require('internal/constants');
 
 const asyncESM = require('internal/process/esm_loader');
+const { enrichCJSError } = require('internal/modules/esm/translators');
 const { kEvaluated } = internalBinding('module_wrap');
 const {
   encodedSepRegEx,
@@ -119,31 +120,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.
-    process.emitWarning(
-      'To load an ES module, set "type": "module" in the package.json or use ' +
-      'the .mjs extension.',
-      undefined,
-      undefined,
-      undefined,
-      true);
-  }
-}
-
 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
@@ -9,9 +9,12 @@ const {
   ObjectKeys,
   PromisePrototypeCatch,
   PromiseReject,
+  RegExpPrototypeTest,
   SafeMap,
   SafeSet,
   StringPrototypeReplace,
+  StringPrototypeSplit,
+  StringPrototypeStartsWith,
 } = primordials;
 
 let _TYPES = null;
@@ -57,6 +60,7 @@ const cjsParse = require('internal/deps/cjs-module-lexer/lexer');
 
 const translators = new SafeMap();
 exports.translators = translators;
+exports.enrichCJSError = enrichCJSError;
 
 let DECODER = null;
 function assertBufferSource(body, allowString, hookName) {
@@ -130,6 +134,29 @@ 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.
+    process.emitWarning(
+      'To load an ES module, set "type": "module" in the package.json or use ' +
+      'the .mjs extension.',
+      undefined,
+      undefined,
+      undefined,
+      true);
+  }
+}
+
 // Strategy for loading a node-style CommonJS module
 const isWindows = process.platform === 'win32';
 const winSepRegEx = /\//g;
@@ -152,7 +179,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 +222,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
@@ -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
@@ -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
@@ -0,0 +1 @@
+export var name = 5;