Skip to content

Commit

Permalink
esm: refine ERR_REQUIRE_ESM errors
Browse files Browse the repository at this point in the history
  • Loading branch information
@guybedford
guybedford committed Jun 27, 2021
1 parent fdb097c commit ff5d5f7
Show file tree
Hide file tree
Showing 8 changed files with 218 additions and 69 deletions.
103 changes: 81 additions & 22 deletions lib/internal/errors.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ const {
AggregateError,
ArrayFrom,
ArrayIsArray,
ArrayPrototypeFilter,
ArrayPrototypeIncludes,
ArrayPrototypeIndexOf,
ArrayPrototypeJoin,
Expand Down Expand Up @@ -116,6 +117,11 @@ const prepareStackTrace = (globalThis, error, trace) => {
// Error: Message
// at function (file)
// at file
const code = 'code' in error ? error.code : null;
if (code && overrideStackTraceByCode.has(code)) {
const f = overrideStackTraceByCode.get(code);
return f(error, trace);
}
const errorString = ErrorPrototypeToString(error);
if (trace.length === 0) {
return errorString;
Expand Down Expand Up @@ -186,14 +192,16 @@ function lazyBuffer() {
return buffer;
}

const addCodeToName = hideStackFrames(function addCodeToName(err, name, code) {
const addCodeToName = hideStackFrames(function addCodeToName(err, name, code,
deferStack) {
// Set the stack
err = captureLargerStackTrace(err);
// Add the error code to the name to include it in the stack trace.
err.name = `${name} [${code}]`;
// Access the stack to generate the error message including the error code
// from the name.
err.stack; // eslint-disable-line no-unused-expressions
if (!deferStack)
err.stack; // eslint-disable-line no-unused-expressions
// Reset the name to the actual name.
if (name === 'SystemError') {
ObjectDefineProperty(err, 'name', {
Expand Down Expand Up @@ -248,7 +256,7 @@ class SystemError extends Error {
writable: true,
configurable: true
});
addCodeToName(this, 'SystemError', key);
addCodeToName(this, 'SystemError', key, false);

this.code = key;

Expand Down Expand Up @@ -338,7 +346,7 @@ function makeSystemErrorWithCode(key) {
};
}

function makeNodeErrorWithCode(Base, key) {
function makeNodeErrorWithCode(Base, key, deferStack) {
return function NodeError(...args) {
const limit = Error.stackTraceLimit;
if (isErrorStackTraceLimitWritable()) Error.stackTraceLimit = 0;
Expand All @@ -360,7 +368,7 @@ function makeNodeErrorWithCode(Base, key) {
writable: true,
configurable: true,
});
addCodeToName(error, Base.name, key);
addCodeToName(error, Base.name, key, deferStack);
error.code = key;
return error;
};
Expand All @@ -382,18 +390,19 @@ function hideStackFrames(fn) {
// Utility function for registering the error codes. Only used here. Exported
// *only* to allow for testing.
function E(sym, val, def, ...otherClasses) {
const deferStack = overrideStackTraceByCode.has(sym);
// Special case for SystemError that formats the error message differently
// The SystemErrors only have SystemError as their base classes.
messages.set(sym, val);
if (def === SystemError) {
def = makeSystemErrorWithCode(sym);
} else {
def = makeNodeErrorWithCode(def, sym);
def = makeNodeErrorWithCode(def, sym, deferStack);
}

if (otherClasses.length !== 0) {
otherClasses.forEach((clazz) => {
def[clazz.name] = makeNodeErrorWithCode(clazz, sym);
def[clazz.name] = makeNodeErrorWithCode(clazz, sym, deferStack);
});
}
codes[sym] = def;
Expand Down Expand Up @@ -781,6 +790,40 @@ const fatalExceptionStackEnhancers = {
}
};

// Ensures the printed error line is from user code.
let _kArrowMessagePrivateSymbol, _setHiddenValue;
function setArrowMessage(err, arrowMessage) {
if (!_kArrowMessagePrivateSymbol) {
({
arrow_message_private_symbol: _kArrowMessagePrivateSymbol,
setHiddenValue: _setHiddenValue,
} = internalBinding('util'));
}
_setHiddenValue(err, _kArrowMessagePrivateSymbol, arrowMessage);
}

// Hide stack lines before the first user code line.
function hideLeadingInternalFrames(error, stackFrames) {
let frames = stackFrames;
if (typeof stackFrames === 'object') {
let beforeUserCode = true;
frames = ArrayPrototypeFilter(
stackFrames,
(frm) => {
if (!beforeUserCode)
return true;
const isInternal = StringPrototypeStartsWith(frm.getFileName(),
'node:internal');
if (!isInternal)
beforeUserCode = false;
return !isInternal;
},
);
}
ArrayPrototypeUnshift(frames, error);
return ArrayPrototypeJoin(frames, '\n at ');
}

// Node uses an AbortError that isn't exactly the same as the DOMException
// to make usage of the error in userland and readable-stream easier.
// It is a regular error with `.code` and `.name`.
Expand All @@ -802,6 +845,7 @@ module.exports = {
hideStackFrames,
isErrorStackTraceLimitWritable,
isStackOverflowError,
setArrowMessage,
connResetException,
uvErrmapGet,
uvException,
Expand Down Expand Up @@ -834,6 +878,12 @@ module.exports = {
// Note: Please try to keep these in alphabetical order
//
// Note: Node.js specific errors must begin with the prefix ERR_

// Custom error stack overrides.
const overrideStackTraceByCode = new SafeMap([
['ERR_REQUIRE_ESM', hideLeadingInternalFrames],
]);

E('ERR_AMBIGUOUS_ARGUMENT', 'The "%s" argument is ambiguous. %s', TypeError);
E('ERR_ARG_NOT_ITERABLE', '%s must be iterable', TypeError);
E('ERR_ASSERTION', '%s', Error);
Expand Down Expand Up @@ -1397,23 +1447,32 @@ E('ERR_PERFORMANCE_INVALID_TIMESTAMP',
'%d is not a valid timestamp', TypeError);
E('ERR_PERFORMANCE_MEASURE_INVALID_OPTIONS', '%s', TypeError);
E('ERR_REQUIRE_ESM',
(filename, parentPath = null, packageJsonPath = null) => {
let msg = `Must use import to load ES Module: ${filename}`;
if (parentPath && packageJsonPath) {
const path = require('path');
const basename = path.basename(filename) === path.basename(parentPath) ?
filename : path.basename(filename);
msg +=
'\nrequire() of ES modules is not supported.\nrequire() of ' +
`${filename} from ${parentPath} ` +
'is an ES module file as it is a .js file whose nearest parent ' +
'package.json contains "type": "module" which defines all .js ' +
'files in that package scope as ES modules.\nInstead rename ' +
`${basename} to end in .cjs, change the requiring code to use ` +
'import(), or remove "type": "module" from ' +
`${packageJsonPath}.\n`;
function(filename, hasEsmSyntax, parentPath = null, packageJsonPath = null) {
let msg = `require() of ES Module ${filename}${parentPath ? ` from ${
parentPath}` : ''} not supported.`;
if (!packageJsonPath) {
if (filename.endsWith('.mjs'))
msg += `\nInstead change the require of ${filename} to a dynamic ` +
'import() which is available in all CommonJS modules.';
return msg;
}
const path = require('path');
const basename = path.basename(filename) === path.basename(parentPath) ?
filename : path.basename(filename);
msg += `\n${basename} is treated as an ES module file as it is a .js ` +
'file whose nearest parent package.json contains "type": "module" ' +
'which declares all .js files in that package scope as ES modules.';
if (hasEsmSyntax) {
msg += `\nInstead change the require of ${basename} in ${parentPath} to` +
' a dynamic import() which is available in all CommonJS modules.';
return msg;
}
msg +=
`\nInstead rename ${basename} to end in .cjs, change the requiring ` +
'code to use dynamic import() which is available in all CommonJS' +
`modules, or remove "type": "module" from ${packageJsonPath} to ` +
'treat all .js files as CommonJS (using .mjs for all ES modules ' +
'instead).\n';
return msg;
}, Error);
E('ERR_SCRIPT_EXECUTION_INTERRUPTED',
Expand Down
32 changes: 32 additions & 0 deletions lib/internal/modules/cjs/helpers.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,14 @@ let debug = require('internal/util/debuglog').debuglog('module', (fn) => {
debug = fn;
});

const acorn = require('internal/deps/acorn/acorn/dist/acorn');
const privateMethods =
require('internal/deps/acorn-plugins/acorn-private-methods/index');
const classFields =
require('internal/deps/acorn-plugins/acorn-class-fields/index');
const staticClassFeatures =
require('internal/deps/acorn-plugins/acorn-static-class-features/index');

// TODO: Use this set when resolving pkg#exports conditions in loader.js.
const cjsConditions = new SafeSet(['require', 'node', ...userConditions]);

Expand Down Expand Up @@ -184,9 +192,33 @@ function normalizeReferrerURL(referrer) {
return new URL(referrer).href;
}

// For error messages only - used to check if ESM syntax is in use.
function hasEsmSyntax(code) {
const parser = acorn.Parser.extend(
privateMethods,
classFields,
staticClassFeatures
);
let root;
try {
root = parser.parse(code, { sourceType: 'module', ecmaVersion: 'latest' });
} catch {
return false;
}

for (const stmt of root.body) {
if (stmt.type === 'ExportNamedDeclaration' ||
stmt.type === 'ImportDeclaration' ||
stmt.type === 'ExportAllDeclaration')
return true;
}
return false;
}

module.exports = {
addBuiltinLibsToObject,
cjsConditions,
hasEsmSyntax,
loadNativeModule,
makeRequireFunction,
normalizeReferrerURL,
Expand Down
63 changes: 45 additions & 18 deletions lib/internal/modules/cjs/loader.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ const {
StringPrototypeLastIndexOf,
StringPrototypeIndexOf,
StringPrototypeMatch,
StringPrototypeRepeat,
StringPrototypeSlice,
StringPrototypeSplit,
StringPrototypeStartsWith,
Expand Down Expand Up @@ -88,11 +89,12 @@ const { internalModuleStat } = internalBinding('fs');
const packageJsonReader = require('internal/modules/package_json_reader');
const { safeGetenv } = internalBinding('credentials');
const {
cjsConditions,
hasEsmSyntax,
loadNativeModule,
makeRequireFunction,
normalizeReferrerURL,
stripBOM,
cjsConditions,
loadNativeModule
} = require('internal/modules/cjs/helpers');
const { getOptionValue } = require('internal/options');
const preserveSymlinks = getOptionValue('--preserve-symlinks');
Expand All @@ -107,11 +109,14 @@ const policy = getOptionValue('--experimental-policy') ?
let hasLoadedAnyUserCJSModule = false;

const {
ERR_INVALID_ARG_VALUE,
ERR_INVALID_MODULE_SPECIFIER,
ERR_REQUIRE_ESM,
ERR_UNKNOWN_BUILTIN_MODULE,
} = require('internal/errors').codes;
codes: {
ERR_INVALID_ARG_VALUE,
ERR_INVALID_MODULE_SPECIFIER,
ERR_REQUIRE_ESM,
ERR_UNKNOWN_BUILTIN_MODULE,
},
setArrowMessage,
} = require('internal/errors');
const { validateString } = require('internal/validators');
const pendingDeprecation = getOptionValue('--pending-deprecation');

Expand Down Expand Up @@ -970,7 +975,7 @@ Module.prototype.load = function(filename) {
const extension = findLongestRegisteredExtension(filename);
// allow .mjs to be overridden
if (StringPrototypeEndsWith(filename, '.mjs') && !Module._extensions['.mjs'])
throw new ERR_REQUIRE_ESM(filename);
throw new ERR_REQUIRE_ESM(filename, true);

Module._extensions[extension](this, filename);
this.loaded = true;
Expand Down Expand Up @@ -1102,16 +1107,6 @@ Module.prototype._compile = function(content, filename) {

// Native extension for .js
Module._extensions['.js'] = function(module, filename) {
if (StringPrototypeEndsWith(filename, '.js')) {
const pkg = readPackageScope(filename);
// Function require shouldn't be used in ES modules.
if (pkg?.data?.type === 'module') {
const parent = moduleParentCache.get(module);
const parentPath = parent?.filename;
const packageJsonPath = path.resolve(pkg.path, 'package.json');
throw new ERR_REQUIRE_ESM(filename, parentPath, packageJsonPath);
}
}
// If already analyzed the source, then it will be cached.
const cached = cjsParseCache.get(module);
let content;
Expand All @@ -1121,6 +1116,38 @@ Module._extensions['.js'] = function(module, filename) {
} else {
content = fs.readFileSync(filename, 'utf8');
}
if (StringPrototypeEndsWith(filename, '.js')) {
const pkg = readPackageScope(filename);
// Function require shouldn't be used in ES modules.
if (pkg?.data?.type === 'module') {
const parent = moduleParentCache.get(module);
const parentPath = parent?.filename;
const packageJsonPath = path.resolve(pkg.path, 'package.json');
const usesEsm = hasEsmSyntax(content);
const err = new ERR_REQUIRE_ESM(filename, usesEsm, parentPath,
packageJsonPath);
// Attempt to reconstruct the parent require frame.
if (Module._cache[parentPath]) {
let parentSource;
try {
parentSource = fs.readFileSync(parentPath, 'utf8');
} catch {}
if (parentSource) {
const errLine = StringPrototypeSplit(StringPrototypeSlice(
err.stack, StringPrototypeIndexOf(err.stack, ' at ')), '\n')[0];
const { 1: line, 2: col } =
StringPrototypeMatch(errLine, /(\d+):(\d+)\)/) || [];
if (line && col) {
const srcLine = StringPrototypeSplit(parentSource, '\n')[line - 1];
const frame = `${parentPath}:${line}\n${srcLine}\n${
StringPrototypeRepeat(' ', col - 1)}^\n`;
setArrowMessage(err, frame);
}
}
}
throw err;
}
}
module._compile(content, filename);
};

Expand Down
6 changes: 6 additions & 0 deletions src/node_errors.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,12 @@ void AppendExceptionLine(Environment* env,
Local<Object> err_obj;
if (!er.IsEmpty() && er->IsObject()) {
err_obj = er.As<Object>();
// If arrow_message is already set, skip.
auto maybe_value = err_obj->GetPrivate(env->context(),
env->arrow_message_private_symbol());
Local<Value> lvalue;
if (maybe_value.ToLocal(&lvalue) && lvalue->IsString())
return;
}

bool added_exception_line = false;
Expand Down
Loading

0 comments on commit ff5d5f7

Please sign in to comment.