Skip to content

Commit

Permalink
module: refine module type mismatch error cases
Browse files Browse the repository at this point in the history
PR-URL: #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>
  • Loading branch information
guybedford authored and danielleadams committed Oct 6, 2020
1 parent 7dc6b2f commit f23a0e2
Show file tree
Hide file tree
Showing 5 changed files with 55 additions and 26 deletions.
24 changes: 1 addition & 23 deletions lib/internal/modules/cjs/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,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');
Expand Down Expand Up @@ -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,
Expand All @@ -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) {
Expand Down
39 changes: 37 additions & 2 deletions lib/internal/modules/esm/translators.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,12 @@ const {
ObjectKeys,
PromisePrototypeCatch,
PromiseReject,
RegExpPrototypeTest,
SafeMap,
SafeSet,
StringPrototypeReplace,
StringPrototypeSplit,
StringPrototypeStartsWith,
} = primordials;

let _TYPES = null;
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
Expand All @@ -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) {
Expand Down Expand Up @@ -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);

Expand Down
16 changes: 15 additions & 1 deletion test/es-module/test-esm-cjs-exports.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -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\''));
}));
1 change: 1 addition & 0 deletions test/fixtures/es-modules/cjs-exports-invalid.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
import cjs from './invalid-cjs.js';
1 change: 1 addition & 0 deletions test/fixtures/es-modules/invalid-cjs.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export var name = 5;

0 comments on commit f23a0e2

Please sign in to comment.