From 6ca8fb552d1d1fc5b0cbf16f19f5968422ed7c8b 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 Backport-PR-URL: https://github.com/nodejs/node/pull/35757 PR-URL: https://github.com/nodejs/node/pull/35426 Reviewed-By: Matteo Collina Reviewed-By: Bradley Farias Reviewed-By: Myles Borins --- lib/internal/modules/cjs/loader.js | 26 +----------- lib/internal/modules/esm/translators.js | 42 ++++++++++++++++++- 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, 58 insertions(+), 28 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 f07ff6c830c608..99579d6ad62c81 100644 --- 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 index 5e4de5d5af0f39..acbf1ab1480dea 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; @@ -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 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;