From 15babc4f1db8e5084c4694c441cb36079aa77d50 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Sun, 27 Jun 2021 15:29:02 -0700 Subject: [PATCH 01/17] esm: refine ERR_REQUIRE_ESM errors --- lib/internal/errors.js | 87 +++++++++++++++---- lib/internal/modules/cjs/helpers.js | 32 +++++++ lib/internal/modules/cjs/loader.js | 63 ++++++++++---- src/node_errors.cc | 6 ++ test/es-module/test-cjs-esm-warn.js | 77 ++++++++++------ test/es-module/test-esm-type-flag-errors.js | 4 +- test/fixtures/es-modules/cjs-esm-esm.js | 1 + .../es-modules/package-type-module/esm.js | 1 + 8 files changed, 206 insertions(+), 65 deletions(-) create mode 100644 test/fixtures/es-modules/cjs-esm-esm.js create mode 100644 test/fixtures/es-modules/package-type-module/esm.js diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 30a2a8c0399ba8..78b8eafca5ee92 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -14,6 +14,7 @@ const { AggregateError, ArrayFrom, ArrayIsArray, + ArrayPrototypeFilter, ArrayPrototypeIncludes, ArrayPrototypeIndexOf, ArrayPrototypeJoin, @@ -337,7 +338,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; @@ -389,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; @@ -788,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`. @@ -808,6 +844,7 @@ module.exports = { hideStackFrames, isErrorStackTraceLimitWritable, isStackOverflowError, + setArrowMessage, connResetException, uvErrmapGet, uvException, @@ -842,6 +879,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); @@ -1406,23 +1449,31 @@ 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); + 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 += `\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.' + + `\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', diff --git a/lib/internal/modules/cjs/helpers.js b/lib/internal/modules/cjs/helpers.js index 94602b5309efff..c3ea5e884c51b4 100644 --- a/lib/internal/modules/cjs/helpers.js +++ b/lib/internal/modules/cjs/helpers.js @@ -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]); @@ -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, diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index d969a6449cf545..41cae1d83282cc 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -58,6 +58,7 @@ const { StringPrototypeLastIndexOf, StringPrototypeIndexOf, StringPrototypeMatch, + StringPrototypeRepeat, StringPrototypeSlice, StringPrototypeSplit, StringPrototypeStartsWith, @@ -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'); @@ -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'); @@ -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; @@ -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; @@ -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); }; diff --git a/src/node_errors.cc b/src/node_errors.cc index a22493a140dfd2..d424344708c7fc 100644 --- a/src/node_errors.cc +++ b/src/node_errors.cc @@ -215,6 +215,12 @@ void AppendExceptionLine(Environment* env, Local err_obj; if (!er.IsEmpty() && er->IsObject()) { err_obj = er.As(); + // If arrow_message is already set, skip. + auto maybe_value = err_obj->GetPrivate(env->context(), + env->arrow_message_private_symbol()); + Local lvalue; + if (maybe_value.ToLocal(&lvalue) && lvalue->IsString()) + return; } bool added_exception_line = false; diff --git a/test/es-module/test-cjs-esm-warn.js b/test/es-module/test-cjs-esm-warn.js index ddeda72fc84b41..70d0524f421339 100644 --- a/test/es-module/test-cjs-esm-warn.js +++ b/test/es-module/test-cjs-esm-warn.js @@ -6,36 +6,59 @@ const { spawn } = require('child_process'); const assert = require('assert'); const path = require('path'); -const requiring = path.resolve(fixtures.path('/es-modules/cjs-esm.js')); -const required = path.resolve( - fixtures.path('/es-modules/package-type-module/cjs.js') -); +const requiringCjsAsEsm = path.resolve(fixtures.path('/es-modules/cjs-esm.js')); +const requiringEsm = path.resolve(fixtures.path('/es-modules/cjs-esm-esm.js')); const pjson = path.resolve( fixtures.path('/es-modules/package-type-module/package.json') ); -const basename = 'cjs.js'; +{ + const required = path.resolve( + fixtures.path('/es-modules/package-type-module/cjs.js') + ); + const basename = 'cjs.js'; + const child = spawn(process.execPath, [requiringCjsAsEsm]); + 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.replace(/\r/g, '').includes( + `Error [ERR_REQUIRE_ESM]: require() of ES Module ${required} from ${ + requiringCjsAsEsm} not supported.\n`)); + assert.ok(stderr.includes( + `Instead 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 ${pjson} to ` + + 'treat all .js files as CommonJS (using .mjs for all ES modules ' + + 'instead).\n')); + })); +} -const child = spawn(process.execPath, [requiring]); -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); +{ + const required = path.resolve( + fixtures.path('/es-modules/package-type-module/esm.js') + ); + const basename = 'esm.js'; + const child = spawn(process.execPath, [requiringEsm]); + 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.replace(/\r/g, '').includes( - `Error [ERR_REQUIRE_ESM]: Must use import to load ES Module: ${required}` + - '\nrequire() of ES modules is not supported.\nrequire() of ' + - `${required} from ${requiring} ` + - '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 ' + - `${pjson}.\n`)); - assert.ok(stderr.includes( - 'Error [ERR_REQUIRE_ESM]: Must use import to load ES Module')); -})); + assert.ok(stderr.replace(/\r/g, '').includes( + `Error [ERR_REQUIRE_ESM]: require() of ES Module ${required} from ${ + requiringEsm} not supported.\n`)); + assert.ok(stderr.includes( + `Instead change the require of ${basename} in ${requiringEsm} to` + + ' a dynamic import() which is available in all CommonJS modules.\n')); + })); +} diff --git a/test/es-module/test-esm-type-flag-errors.js b/test/es-module/test-esm-type-flag-errors.js index e0dedc16cabf8c..d9d264cc25aa25 100644 --- a/test/es-module/test-esm-type-flag-errors.js +++ b/test/es-module/test-esm-type-flag-errors.js @@ -29,8 +29,8 @@ try { } catch (e) { assert.strictEqual(e.name, 'Error'); assert.strictEqual(e.code, 'ERR_REQUIRE_ESM'); - assert(e.toString().match(/Must use import to load ES Module/g)); - assert(e.message.match(/Must use import to load ES Module/g)); + assert(e.toString().match(/require\(\) of ES Module/g)); + assert(e.message.match(/require\(\) of ES Module/g)); } function expect(opt = '', inputFile, want, wantsError = false) { diff --git a/test/fixtures/es-modules/cjs-esm-esm.js b/test/fixtures/es-modules/cjs-esm-esm.js new file mode 100644 index 00000000000000..0a0cdfb1e56327 --- /dev/null +++ b/test/fixtures/es-modules/cjs-esm-esm.js @@ -0,0 +1 @@ +require('./package-type-module/esm.js'); diff --git a/test/fixtures/es-modules/package-type-module/esm.js b/test/fixtures/es-modules/package-type-module/esm.js new file mode 100644 index 00000000000000..1413bf5968ad73 --- /dev/null +++ b/test/fixtures/es-modules/package-type-module/esm.js @@ -0,0 +1 @@ +export var p = 5; From 8b6d578e781408f195e0673ddd98ff30a8d92033 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Sun, 27 Jun 2021 15:53:11 -0700 Subject: [PATCH 02/17] fixup: space --- lib/internal/errors.js | 2 +- test/es-module/test-cjs-esm-warn.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 78b8eafca5ee92..af6ee960e856bb 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -1470,7 +1470,7 @@ E('ERR_REQUIRE_ESM', 'file whose nearest parent package.json contains "type": "module" ' + 'which declares all .js files in that package scope as ES modules.' + `\nInstead rename ${basename} to end in .cjs, change the requiring ` + - 'code to use dynamic import() which is available in all CommonJS' + + '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'; diff --git a/test/es-module/test-cjs-esm-warn.js b/test/es-module/test-cjs-esm-warn.js index 70d0524f421339..cdda3ed9064d6c 100644 --- a/test/es-module/test-cjs-esm-warn.js +++ b/test/es-module/test-cjs-esm-warn.js @@ -32,7 +32,7 @@ const pjson = path.resolve( requiringCjsAsEsm} not supported.\n`)); assert.ok(stderr.includes( `Instead rename ${basename} to end in .cjs, change the requiring ` + - 'code to use dynamic import() which is available in all CommonJS' + + 'code to use dynamic import() which is available in all CommonJS ' + `modules, or remove "type": "module" from ${pjson} to ` + 'treat all .js files as CommonJS (using .mjs for all ES modules ' + 'instead).\n')); From 0d129ba05c070345b6ded60fb4f3c906d25d77b3 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Sun, 27 Jun 2021 15:55:48 -0700 Subject: [PATCH 03/17] fixup: arrow fn --- lib/internal/errors.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/internal/errors.js b/lib/internal/errors.js index af6ee960e856bb..86dc365cd745d5 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -1449,7 +1449,7 @@ E('ERR_PERFORMANCE_INVALID_TIMESTAMP', '%d is not a valid timestamp', TypeError); E('ERR_PERFORMANCE_MEASURE_INVALID_OPTIONS', '%s', TypeError); E('ERR_REQUIRE_ESM', - function(filename, hasEsmSyntax, parentPath = null, packageJsonPath = null) { + (filename, hasEsmSyntax, parentPath = null, packageJsonPath = null) => { let msg = `require() of ES Module ${filename}${parentPath ? ` from ${ parentPath}` : ''} not supported.`; if (!packageJsonPath) { From 7765a14aebd87335aa08a5626f6307ce235316d4 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Sun, 27 Jun 2021 16:40:19 -0700 Subject: [PATCH 04/17] fixup: Apply suggestions from code review Co-authored-by: Antoine du Hamel --- lib/internal/errors.js | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 86dc365cd745d5..4f541154283bcc 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -115,6 +115,13 @@ const prepareStackTrace = (globalThis, error, trace) => { maybeOverridePrepareStackTrace(globalThis, error, trace); if (globalOverride !== kNoOverride) return globalOverride; + // try to avoid unnecessary getter calls + const code = error.code; + if (code && overrideStackTraceByCode.has(code)) { + const f = overrideStackTraceByCode.get(code); + return f(error, trace); + } + // Normal error formatting: // // Error: Message @@ -881,9 +888,8 @@ module.exports = { // Note: Node.js specific errors must begin with the prefix ERR_ // Custom error stack overrides. -const overrideStackTraceByCode = new SafeMap([ - ['ERR_REQUIRE_ESM', hideLeadingInternalFrames], -]); +const overrideStackTraceByCode = new SafeMap() + .set('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); From 187c7b948bab687923a8e2a78ea9eaf5df9d405f Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Mon, 28 Jun 2021 06:21:06 -0700 Subject: [PATCH 05/17] fixup: pr feedback --- lib/internal/errors.js | 28 ++++++++++++++-------------- lib/internal/modules/cjs/helpers.js | 12 +++++------- src/node_errors.cc | 2 +- 3 files changed, 20 insertions(+), 22 deletions(-) diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 4f541154283bcc..0cd32da53860d1 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -14,7 +14,7 @@ const { AggregateError, ArrayFrom, ArrayIsArray, - ArrayPrototypeFilter, + ArrayPrototypeFindIndex, ArrayPrototypeIncludes, ArrayPrototypeIndexOf, ArrayPrototypeJoin, @@ -122,6 +122,12 @@ const prepareStackTrace = (globalThis, error, trace) => { return f(error, trace); } + const { code } = error; + if (code && overrideStackTraceByCode.has(code)) { + const f = overrideStackTraceByCode.get(code); + return f(error, trace); + } + // Normal error formatting: // // Error: Message @@ -813,19 +819,13 @@ function setArrowMessage(err, arrowMessage) { function hideLeadingInternalFrames(error, stackFrames) { let frames = stackFrames; if (typeof stackFrames === 'object') { - let beforeUserCode = true; - frames = ArrayPrototypeFilter( + frames = ArrayPrototypeSlice( stackFrames, - (frm) => { - if (!beforeUserCode) - return true; - const isInternal = StringPrototypeStartsWith(frm.getFileName(), - 'node:internal'); - if (!isInternal) - beforeUserCode = false; - return !isInternal; - }, - ); + ArrayPrototypeFindIndex( + stackFrames, + (frm) => !StringPrototypeStartsWith(frm.getFileName(), + 'node:internal') + )); } ArrayPrototypeUnshift(frames, error); return ArrayPrototypeJoin(frames, '\n at '); @@ -1459,7 +1459,7 @@ E('ERR_REQUIRE_ESM', let msg = `require() of ES Module ${filename}${parentPath ? ` from ${ parentPath}` : ''} not supported.`; if (!packageJsonPath) { - if (filename.endsWith('.mjs')) + if (StringPrototypeEndsWith(filename, '.mjs')) msg += `\nInstead change the require of ${filename} to a dynamic ` + 'import() which is available in all CommonJS modules.'; return msg; diff --git a/lib/internal/modules/cjs/helpers.js b/lib/internal/modules/cjs/helpers.js index c3ea5e884c51b4..6d229a0422ea12 100644 --- a/lib/internal/modules/cjs/helpers.js +++ b/lib/internal/modules/cjs/helpers.js @@ -3,6 +3,7 @@ const { ArrayPrototypeForEach, ArrayPrototypeJoin, + ArrayPrototypeSome, ObjectDefineProperty, ObjectPrototypeHasOwnProperty, SafeMap, @@ -206,13 +207,10 @@ function hasEsmSyntax(code) { return false; } - for (const stmt of root.body) { - if (stmt.type === 'ExportNamedDeclaration' || - stmt.type === 'ImportDeclaration' || - stmt.type === 'ExportAllDeclaration') - return true; - } - return false; + return ArrayPrototypeSome(root.body, (stmt) => + stmt.type === 'ExportNamedDeclaration' || + stmt.type === 'ImportDeclaration' || + stmt.type === 'ExportAllDeclaration'); } module.exports = { diff --git a/src/node_errors.cc b/src/node_errors.cc index d424344708c7fc..b64c2d831eae21 100644 --- a/src/node_errors.cc +++ b/src/node_errors.cc @@ -219,7 +219,7 @@ void AppendExceptionLine(Environment* env, auto maybe_value = err_obj->GetPrivate(env->context(), env->arrow_message_private_symbol()); Local lvalue; - if (maybe_value.ToLocal(&lvalue) && lvalue->IsString()) + if (!maybe_value.ToLocal(&lvalue) || lvalue->IsString()) return; } From 8fbf23bfe27a52ada56da44c4cbfb8758f0d0fd7 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Mon, 28 Jun 2021 06:35:53 -0700 Subject: [PATCH 06/17] fixup: errors linting --- lib/internal/errors.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 0cd32da53860d1..7d64d92e511ed8 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -825,7 +825,8 @@ function hideLeadingInternalFrames(error, stackFrames) { stackFrames, (frm) => !StringPrototypeStartsWith(frm.getFileName(), 'node:internal') - )); + ) + ); } ArrayPrototypeUnshift(frames, error); return ArrayPrototypeJoin(frames, '\n at '); From fdd9b31f3eaf4125fdaca7edba3944ce76428ef2 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Thu, 8 Jul 2021 10:26:13 -0700 Subject: [PATCH 07/17] fixup: simplify internal stack frame hiding --- lib/internal/errors.js | 51 ++++++++++------------------- lib/internal/modules/cjs/helpers.js | 13 +------- 2 files changed, 18 insertions(+), 46 deletions(-) diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 7d64d92e511ed8..3a46157b2e3503 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -14,7 +14,7 @@ const { AggregateError, ArrayFrom, ArrayIsArray, - ArrayPrototypeFindIndex, + ArrayPrototypeFilter, ArrayPrototypeIncludes, ArrayPrototypeIndexOf, ArrayPrototypeJoin, @@ -115,19 +115,6 @@ const prepareStackTrace = (globalThis, error, trace) => { maybeOverridePrepareStackTrace(globalThis, error, trace); if (globalOverride !== kNoOverride) return globalOverride; - // try to avoid unnecessary getter calls - const code = error.code; - if (code && overrideStackTraceByCode.has(code)) { - const f = overrideStackTraceByCode.get(code); - return f(error, trace); - } - - const { code } = error; - if (code && overrideStackTraceByCode.has(code)) { - const f = overrideStackTraceByCode.get(code); - return f(error, trace); - } - // Normal error formatting: // // Error: Message @@ -351,7 +338,7 @@ function makeSystemErrorWithCode(key) { }; } -function makeNodeErrorWithCode(Base, key, deferStack) { +function makeNodeErrorWithCode(Base, key) { return function NodeError(...args) { const limit = Error.stackTraceLimit; if (isErrorStackTraceLimitWritable()) Error.stackTraceLimit = 0; @@ -403,19 +390,18 @@ 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, deferStack); + def = makeNodeErrorWithCode(def, sym); } if (otherClasses.length !== 0) { otherClasses.forEach((clazz) => { - def[clazz.name] = makeNodeErrorWithCode(clazz, sym, deferStack); + def[clazz.name] = makeNodeErrorWithCode(clazz, sym); }); } codes[sym] = def; @@ -816,20 +802,19 @@ function setArrowMessage(err, arrowMessage) { } // Hide stack lines before the first user code line. -function hideLeadingInternalFrames(error, stackFrames) { - let frames = stackFrames; - if (typeof stackFrames === 'object') { - frames = ArrayPrototypeSlice( - stackFrames, - ArrayPrototypeFindIndex( +function hideInternalStackFrames(error) { + overrideStackTrace.set(error, (error, stackFrames) => { + let frames = stackFrames; + if (typeof stackFrames === 'object') { + frames = ArrayPrototypeFilter( stackFrames, (frm) => !StringPrototypeStartsWith(frm.getFileName(), 'node:internal') - ) - ); - } - ArrayPrototypeUnshift(frames, error); - return ArrayPrototypeJoin(frames, '\n at '); + ); + } + ArrayPrototypeUnshift(frames, error); + return ArrayPrototypeJoin(frames, '\n at '); + }); } // Node uses an AbortError that isn't exactly the same as the DOMException @@ -850,6 +835,7 @@ module.exports = { exceptionWithHostPort, getMessage, hideStackFrames, + hideInternalStackFrames, isErrorStackTraceLimitWritable, isStackOverflowError, setArrowMessage, @@ -888,10 +874,6 @@ module.exports = { // // Note: Node.js specific errors must begin with the prefix ERR_ -// Custom error stack overrides. -const overrideStackTraceByCode = new SafeMap() - .set('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); @@ -1456,7 +1438,8 @@ 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, hasEsmSyntax, parentPath = null, packageJsonPath = null) => { + function(filename, hasEsmSyntax, parentPath = null, packageJsonPath = null) { + hideInternalStackFrames(this); let msg = `require() of ES Module ${filename}${parentPath ? ` from ${ parentPath}` : ''} not supported.`; if (!packageJsonPath) { diff --git a/lib/internal/modules/cjs/helpers.js b/lib/internal/modules/cjs/helpers.js index 6d229a0422ea12..16dd59747e23fc 100644 --- a/lib/internal/modules/cjs/helpers.js +++ b/lib/internal/modules/cjs/helpers.js @@ -30,13 +30,7 @@ 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'); +const parser = require('internal/deps/acorn/acorn/dist/acorn').Parser; // TODO: Use this set when resolving pkg#exports conditions in loader.js. const cjsConditions = new SafeSet(['require', 'node', ...userConditions]); @@ -195,11 +189,6 @@ function normalizeReferrerURL(referrer) { // 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' }); From af548b5f71f78d121bca878b2c7aae860e93ada1 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Thu, 8 Jul 2021 13:01:57 -0700 Subject: [PATCH 08/17] fixup: tests --- test/parallel/test-bootstrap-modules.js | 1 + test/parallel/test-require-mjs.js | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/test/parallel/test-bootstrap-modules.js b/test/parallel/test-bootstrap-modules.js index 0ca00f31adce8c..e87ce89ae25e90 100644 --- a/test/parallel/test-bootstrap-modules.js +++ b/test/parallel/test-bootstrap-modules.js @@ -50,6 +50,7 @@ const expectedModules = new Set([ 'NativeModule internal/console/constructor', 'NativeModule internal/console/global', 'NativeModule internal/constants', + 'NativeModule internal/deps/acorn/acorn/dist/acorn', 'NativeModule internal/encoding', 'NativeModule internal/errors', 'NativeModule internal/event_target', diff --git a/test/parallel/test-require-mjs.js b/test/parallel/test-require-mjs.js index 69f8527555db71..112f08879d4290 100644 --- a/test/parallel/test-require-mjs.js +++ b/test/parallel/test-require-mjs.js @@ -5,7 +5,7 @@ const assert = require('assert'); assert.throws( () => require('../fixtures/es-modules/test-esm-ok.mjs'), { - message: /Must use import to load ES Module/, + message: /dynamic import\(\) which is available in all CommonJS modules/, code: 'ERR_REQUIRE_ESM' } ); From cfe3db93161a228ac83daad097d2694449911b50 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Mon, 12 Jul 2021 23:59:35 +0200 Subject: [PATCH 09/17] fixup: code review Co-authored-by: Antoine du Hamel --- lib/internal/modules/cjs/loader.js | 4 ++-- test/es-module/test-cjs-esm-warn.js | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 41cae1d83282cc..b5c62035dbdee2 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -1134,9 +1134,9 @@ Module._extensions['.js'] = function(module, filename) { } catch {} if (parentSource) { const errLine = StringPrototypeSplit(StringPrototypeSlice( - err.stack, StringPrototypeIndexOf(err.stack, ' at ')), '\n')[0]; + err.stack, StringPrototypeIndexOf(err.stack, ' at ')), '\n', 1)[0]; const { 1: line, 2: col } = - StringPrototypeMatch(errLine, /(\d+):(\d+)\)/) || []; + RegExpPrototypeExec(/(\d+):(\d+)\)/, errLine) || []; if (line && col) { const srcLine = StringPrototypeSplit(parentSource, '\n')[line - 1]; const frame = `${parentPath}:${line}\n${srcLine}\n${ diff --git a/test/es-module/test-cjs-esm-warn.js b/test/es-module/test-cjs-esm-warn.js index cdda3ed9064d6c..3f0cf092501fa6 100644 --- a/test/es-module/test-cjs-esm-warn.js +++ b/test/es-module/test-cjs-esm-warn.js @@ -27,7 +27,7 @@ const pjson = path.resolve( assert.strictEqual(code, 1); assert.strictEqual(signal, null); - assert.ok(stderr.replace(/\r/g, '').includes( + assert.ok(stderr.replaceAll('\r', '').includes( `Error [ERR_REQUIRE_ESM]: require() of ES Module ${required} from ${ requiringCjsAsEsm} not supported.\n`)); assert.ok(stderr.includes( From 78640e481584f873d79f64f4765c97898705bfe5 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Mon, 12 Jul 2021 15:00:35 -0700 Subject: [PATCH 10/17] fixup: builtin require --- lib/internal/modules/cjs/loader.js | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index b5c62035dbdee2..7e1cf1a7ef7207 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -48,6 +48,7 @@ const { Proxy, ReflectApply, ReflectSet, + RegExpPrototypeExec, RegExpPrototypeTest, SafeMap, SafeWeakMap, From 16b687cf8f49e4a6180e9f47336876a3911703c6 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Tue, 13 Jul 2021 00:01:49 +0200 Subject: [PATCH 11/17] fixup: ERR_REQUIRE_ESM recommend "type": "commonjs" Co-authored-by: Antoine du Hamel --- test/es-module/test-cjs-esm-warn.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/es-module/test-cjs-esm-warn.js b/test/es-module/test-cjs-esm-warn.js index 3f0cf092501fa6..1b2524400b812b 100644 --- a/test/es-module/test-cjs-esm-warn.js +++ b/test/es-module/test-cjs-esm-warn.js @@ -33,7 +33,7 @@ const pjson = path.resolve( assert.ok(stderr.includes( `Instead 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 ${pjson} to ` + + `modules, or change "type": "module" to "type": "commonjs" from ${pjson} to ` + 'treat all .js files as CommonJS (using .mjs for all ES modules ' + 'instead).\n')); })); From e628c0a7f198c5e7aa0b58d9fd0d50d44a7c62fb Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Mon, 12 Jul 2021 15:05:05 -0700 Subject: [PATCH 12/17] fixup: line length --- lib/internal/modules/cjs/loader.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 7e1cf1a7ef7207..31ab3a8ee9c86f 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -1134,8 +1134,9 @@ Module._extensions['.js'] = function(module, filename) { parentSource = fs.readFileSync(parentPath, 'utf8'); } catch {} if (parentSource) { - const errLine = StringPrototypeSplit(StringPrototypeSlice( - err.stack, StringPrototypeIndexOf(err.stack, ' at ')), '\n', 1)[0]; + const errLine = StringPrototypeSplit( + StringPrototypeSlice(err.stack, StringPrototypeIndexOf( + err.stack, ' at ')), '\n', 1)[0]; const { 1: line, 2: col } = RegExpPrototypeExec(/(\d+):(\d+)\)/, errLine) || []; if (line && col) { From 12b200942de5ea192a57b3c08ed57c694a5c9ab8 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Mon, 12 Jul 2021 17:11:58 -0700 Subject: [PATCH 13/17] fixup: tests --- lib/internal/errors.js | 6 +++--- test/es-module/test-cjs-esm-warn.js | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 3a46157b2e3503..56a0ef8fb06a14 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -1461,9 +1461,9 @@ E('ERR_REQUIRE_ESM', 'which declares all .js files in that package scope as ES modules.' + `\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'; + 'modules, or change "type": "module" to "type": "commonjs" in ' + + `${packageJsonPath} to treat all .js files as CommonJS (using .mjs for ` + + 'all ES modules instead).\n'; return msg; }, Error); E('ERR_SCRIPT_EXECUTION_INTERRUPTED', diff --git a/test/es-module/test-cjs-esm-warn.js b/test/es-module/test-cjs-esm-warn.js index 1b2524400b812b..214b5982d9be10 100644 --- a/test/es-module/test-cjs-esm-warn.js +++ b/test/es-module/test-cjs-esm-warn.js @@ -33,7 +33,7 @@ const pjson = path.resolve( assert.ok(stderr.includes( `Instead rename ${basename} to end in .cjs, change the requiring ` + 'code to use dynamic import() which is available in all CommonJS ' + - `modules, or change "type": "module" to "type": "commonjs" from ${pjson} to ` + + `modules, or change "type": "module" to "type": "commonjs" in ${pjson} to ` + 'treat all .js files as CommonJS (using .mjs for all ES modules ' + 'instead).\n')); })); From 8871f4e87598f522635a3bc9d8cdf96a96f57d81 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Wed, 14 Jul 2021 12:44:37 -0700 Subject: [PATCH 14/17] fixup: windows tests --- test/es-module/test-cjs-esm-warn.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/es-module/test-cjs-esm-warn.js b/test/es-module/test-cjs-esm-warn.js index 214b5982d9be10..ade841b741db77 100644 --- a/test/es-module/test-cjs-esm-warn.js +++ b/test/es-module/test-cjs-esm-warn.js @@ -35,7 +35,7 @@ const pjson = path.resolve( 'code to use dynamic import() which is available in all CommonJS ' + `modules, or change "type": "module" to "type": "commonjs" in ${pjson} to ` + 'treat all .js files as CommonJS (using .mjs for all ES modules ' + - 'instead).\n')); + 'instead).')); })); } From dfd2397a08dc077ade56859a35a338aedb6759c5 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Wed, 14 Jul 2021 15:12:05 -0700 Subject: [PATCH 15/17] fixup: more windows fixes --- test/es-module/test-cjs-esm-warn.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/es-module/test-cjs-esm-warn.js b/test/es-module/test-cjs-esm-warn.js index ade841b741db77..d7eeb65b152a4a 100644 --- a/test/es-module/test-cjs-esm-warn.js +++ b/test/es-module/test-cjs-esm-warn.js @@ -30,12 +30,12 @@ const pjson = path.resolve( assert.ok(stderr.replaceAll('\r', '').includes( `Error [ERR_REQUIRE_ESM]: require() of ES Module ${required} from ${ requiringCjsAsEsm} not supported.\n`)); - assert.ok(stderr.includes( + assert.ok(stderr.replaceAll('\r', '').includes( `Instead rename ${basename} to end in .cjs, change the requiring ` + 'code to use dynamic import() which is available in all CommonJS ' + `modules, or change "type": "module" to "type": "commonjs" in ${pjson} to ` + 'treat all .js files as CommonJS (using .mjs for all ES modules ' + - 'instead).')); + 'instead).\n')); })); } @@ -57,7 +57,7 @@ const pjson = path.resolve( assert.ok(stderr.replace(/\r/g, '').includes( `Error [ERR_REQUIRE_ESM]: require() of ES Module ${required} from ${ requiringEsm} not supported.\n`)); - assert.ok(stderr.includes( + assert.ok(stderr.replace(/\r/g, '').includes( `Instead change the require of ${basename} in ${requiringEsm} to` + ' a dynamic import() which is available in all CommonJS modules.\n')); })); From b156244365bbd30d11cdf712a232f5d1b9926372 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Wed, 14 Jul 2021 19:20:48 -0700 Subject: [PATCH 16/17] fixup: lazily load acorn --- lib/internal/modules/cjs/helpers.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/internal/modules/cjs/helpers.js b/lib/internal/modules/cjs/helpers.js index 16dd59747e23fc..bac854e0fadbac 100644 --- a/lib/internal/modules/cjs/helpers.js +++ b/lib/internal/modules/cjs/helpers.js @@ -30,8 +30,6 @@ let debug = require('internal/util/debuglog').debuglog('module', (fn) => { debug = fn; }); -const parser = require('internal/deps/acorn/acorn/dist/acorn').Parser; - // TODO: Use this set when resolving pkg#exports conditions in loader.js. const cjsConditions = new SafeSet(['require', 'node', ...userConditions]); @@ -189,6 +187,7 @@ function normalizeReferrerURL(referrer) { // For error messages only - used to check if ESM syntax is in use. function hasEsmSyntax(code) { + const parser = require('internal/deps/acorn/acorn/dist/acorn').Parser; let root; try { root = parser.parse(code, { sourceType: 'module', ecmaVersion: 'latest' }); From cf5879f4ed6af519d4911b018ba780f44dea7e2e Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Wed, 14 Jul 2021 19:55:54 -0700 Subject: [PATCH 17/17] fixup: bootstrap list --- test/parallel/test-bootstrap-modules.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/parallel/test-bootstrap-modules.js b/test/parallel/test-bootstrap-modules.js index e87ce89ae25e90..0ca00f31adce8c 100644 --- a/test/parallel/test-bootstrap-modules.js +++ b/test/parallel/test-bootstrap-modules.js @@ -50,7 +50,6 @@ const expectedModules = new Set([ 'NativeModule internal/console/constructor', 'NativeModule internal/console/global', 'NativeModule internal/constants', - 'NativeModule internal/deps/acorn/acorn/dist/acorn', 'NativeModule internal/encoding', 'NativeModule internal/errors', 'NativeModule internal/event_target',