From 59932d9180b6a9f1c50c513b0bfdb9733a56a712 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Wed, 30 Sep 2020 04:24:34 -0700 Subject: [PATCH] module: refine module type mismatch error cases --- lib/internal/modules/cjs/loader.js | 24 +----------- lib/internal/modules/esm/translators.js | 39 ++++++++++++++++++- test/es-module/test-esm-cjs-exports.js | 16 +++++++- .../es-modules/cjs-exports-invalid.mjs | 1 + test/fixtures/es-modules/invalid-cjs.js | 1 + 5 files changed, 55 insertions(+), 26 deletions(-) create mode 100644 test/fixtures/es-modules/cjs-exports-invalid.mjs create mode 100644 test/fixtures/es-modules/invalid-cjs.js diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index a745c66c6142a2..13a4d78647f942 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -71,7 +71,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'); @@ -114,6 +113,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, @@ -128,28 +128,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 efbb5d92d7e26d..94eb275a689671 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 37aa70d3880f2b..7db2c6fdb5971b 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 00000000000000..67428fc27c277d --- /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 00000000000000..15cae6690f1362 --- /dev/null +++ b/test/fixtures/es-modules/invalid-cjs.js @@ -0,0 +1 @@ +export var name = 5;