From 9e3cae248a841b3bf8e84262be5c0f5d5cfa58fe Mon Sep 17 00:00:00 2001 From: Luke Karrys Date: Sun, 12 May 2024 12:59:08 -0700 Subject: [PATCH 1/2] fix(refactor): split error messaging into more fns These will be used to generate error messages from both commands and the exit handler. Also makes the did-you-mean function take a package so it can be sync and called more easily from the error handlers. --- lib/commands/run-script.js | 12 +- lib/npm.js | 94 ++--------- lib/utils/did-you-mean.js | 49 +++--- lib/utils/display.js | 70 ++++++-- lib/utils/error-message.js | 157 +++++++++++++----- lib/utils/output-error.js | 29 ++++ .../test/lib/utils/error-message.js.test.cjs | 18 +- test/lib/cli/exit-handler.js | 4 + test/lib/npm.js | 6 +- test/lib/utils/did-you-mean.js | 43 ++--- test/lib/utils/error-message.js | 15 +- 11 files changed, 285 insertions(+), 212 deletions(-) create mode 100644 lib/utils/output-error.js diff --git a/lib/commands/run-script.js b/lib/commands/run-script.js index dd00c98fc8b6e..3030ba0f7a2c1 100644 --- a/lib/commands/run-script.js +++ b/lib/commands/run-script.js @@ -74,11 +74,13 @@ class RunScript extends BaseCommand { return } - const didYouMean = require('../utils/did-you-mean.js') - const suggestions = await didYouMean(path, event) - throw new Error( - `Missing script: "${event}"${suggestions}\n\nTo see a list of scripts, run:\n npm run` - ) + const suggestions = require('../utils/did-you-mean.js')(pkg, event) + throw new Error([ + `Missing script: "${event}"`, + suggestions, + 'To see a list of scripts, run:', + ' npm run', + ].join('\n')) } // positional args only added to the main event, not pre/post diff --git a/lib/npm.js b/lib/npm.js index dcd6bdfc046df..28acdbbdd6fd7 100644 --- a/lib/npm.js +++ b/lib/npm.js @@ -11,6 +11,7 @@ const { log, time, output, META } = require('proc-log') const { redactLog: replaceInfo } = require('@npmcli/redact') const pkg = require('../package.json') const { deref } = require('./utils/cmd-list.js') +const { jsonError, outputError } = require('./utils/output-error.js') class Npm { static get version () { @@ -287,7 +288,12 @@ class Npm { async #handleError (err) { if (err) { - Object.assign(err, await this.#getError(err)) + // Get the local package if it exists for a more helpful error message + const localPkg = await require('@npmcli/package-json') + .normalize(this.localPrefix) + .then(p => p.content) + .catch(() => null) + Object.assign(err, this.#getError(err, { pkg: localPkg })) } // TODO: make this not need to be public @@ -295,11 +301,7 @@ class Npm { output.flush({ [META]: true, - jsonError: err && this.loaded && this.config.get('json') ? { - code: err.code, - summary: (err.summary || []).map(l => l.slice(1).join(' ')).join('\n'), - detail: (err.detail || []).map(l => l.slice(1).join(' ')).join('\n'), - } : null, + jsonError: jsonError(err, this), }) if (err) { @@ -307,60 +309,12 @@ class Npm { } } - async #getError (err) { - const { errorMessage, getExitCodeFromError } = require('./utils/error-message.js') - - // if we got a command that just shells out to something else, then it - // will presumably print its own errors and exit with a proper status - // code if there's a problem. If we got an error with a code=0, then... - // something else went wrong along the way, so maybe an npm problem? - if (this.#command?.constructor?.isShellout && typeof err.code === 'number' && err.code) { - return { - exitCode: err.code, - suppressError: true, - } - } - - // XXX: we should stop throwing strings - if (typeof err === 'string') { - log.error('', err) - return { - exitCode: 1, - suppressError: true, - } - } - - // XXX: we should stop throwing other non-errors - if (!(err instanceof Error)) { - log.error('weird error', err) - return { - exitCode: 1, - suppressError: true, - } - } - - if (err.code === 'EUNKNOWNCOMMAND') { - const didYouMean = require('./utils/did-you-mean.js') - const suggestions = await didYouMean(this.localPrefix, err.command) - output.standard(`Unknown command: "${err.command}"${suggestions}\n`) - output.standard('To see a list of supported npm commands, run:\n npm help') - return { - exitCode: 1, - suppressError: true, - } - } - - err.code ??= err.message.match(/^(?:Error: )?(E[A-Z]+)/)?.[1] - - for (const k of ['type', 'stack', 'statusCode', 'pkgid']) { - const v = err[k] - if (v) { - log.verbose(k, replaceInfo(v)) - } - } - - const exitCode = getExitCodeFromError(err) || 1 - const { summary, detail, files } = errorMessage(err, this) + #getError (rawErr, opts) { + const { files = [], ...error } = require('./utils/error-message.js').getError(rawErr, { + npm: this, + command: this.#command, + ...opts, + }) const { writeFileSync } = require('node:fs') for (const [file, content] of files) { @@ -368,29 +322,15 @@ class Npm { const fileContent = `'Log files:\n${this.logFiles.join('\n')}\n\n${content.trim()}\n` try { writeFileSync(filePath, fileContent) - detail.push(['', `\n\nFor a full report see:\n${filePath}`]) + error.detail.push(['', `\n\nFor a full report see:\n${filePath}`]) } catch (fileErr) { log.warn('', `Could not write error message to ${file} due to ${fileErr}`) } } - for (const k of ['code', 'syscall', 'file', 'path', 'dest', 'errno']) { - const v = err[k] - if (v) { - log.error(k, v) - } - } + outputError(error) - for (const errline of [...summary, ...detail]) { - log.error(...errline) - } - - return { - exitCode, - summary, - detail, - suppressError: false, - } + return error } get title () { diff --git a/lib/utils/did-you-mean.js b/lib/utils/did-you-mean.js index 54c8ff2e35aa6..7428ed5df85e9 100644 --- a/lib/utils/did-you-mean.js +++ b/lib/utils/did-you-mean.js @@ -1,39 +1,34 @@ const Npm = require('../npm') const { distance } = require('fastest-levenshtein') -const pkgJson = require('@npmcli/package-json') const { commands } = require('./cmd-list.js') -const didYouMean = async (path, scmd) => { - const close = commands.filter(cmd => distance(scmd, cmd) < scmd.length * 0.4 && scmd !== cmd) - let best = [] - for (const str of close) { - const cmd = Npm.cmd(str) - best.push(` npm ${str} # ${cmd.description}`) - } - // We would already be suggesting this in `npm x` so omit them here - const runScripts = ['stop', 'start', 'test', 'restart'] - try { - const { content: { scripts, bin } } = await pkgJson.normalize(path) - best = best.concat( - Object.keys(scripts || {}) - .filter(cmd => distance(scmd, cmd) < scmd.length * 0.4 && !runScripts.includes(cmd)) - .map(str => ` npm run ${str} # run the "${str}" package script`), - Object.keys(bin || {}) - .filter(cmd => distance(scmd, cmd) < scmd.length * 0.4) - /* eslint-disable-next-line max-len */ - .map(str => ` npm exec ${str} # run the "${str}" command from either this or a remote npm package`) - ) - } catch { - // gracefully ignore not being in a folder w/ a package.json - } +const runScripts = ['stop', 'start', 'test', 'restart'] + +const isClose = (scmd, cmd) => distance(scmd, cmd) < scmd.length * 0.4 + +const didYouMean = (pkg, scmd) => { + const { scripts = {}, bin = {} } = pkg || {} + + const best = [ + ...commands + .filter(cmd => isClose(scmd, cmd) && scmd !== cmd) + .map(str => [str, Npm.cmd(str).description]), + ...Object.keys(scripts) + // We would already be suggesting this in `npm x` so omit them here + .filter(cmd => isClose(scmd, cmd) && !runScripts.includes(cmd)) + .map(str => [`run ${str}`, `run the "${str}" package script`]), + ...Object.keys(bin) + .filter(cmd => isClose(scmd, cmd)) + /* eslint-disable-next-line max-len */ + .map(str => [`exec ${str}`, `run the "${str}" command from either this or a remote npm package`]), + ] if (best.length === 0) { return '' } - return best.length === 1 - ? `\n\nDid you mean this?\n${best[0]}` - : `\n\nDid you mean one of these?\n${best.slice(0, 3).join('\n')}` + return `\n\nDid you mean ${best.length === 1 ? 'this' : 'one of these'}?\n` + + best.slice(0, 3).map(([msg, comment]) => ` npm ${msg} # ${comment}`).join('\n') } module.exports = didYouMean diff --git a/lib/utils/display.js b/lib/utils/display.js index 29a1f7951d506..dbb1834645710 100644 --- a/lib/utils/display.js +++ b/lib/utils/display.js @@ -70,7 +70,7 @@ const tryJsonParse = (value) => { try { return JSON.parse(value) } catch { - return {} + return } } return value @@ -86,6 +86,57 @@ const setBlocking = (stream) => { return stream } +// These are important +// This is the key that is returned to the user for errors +const ERROR_KEY = 'error' +// This is the key producers use to indicate that there +// is a json error that should be merged into the finished output +const JSON_ERROR_KEY = 'jsonError' + +const mergeJson = (meta, buffer) => { + const buffered = buffer.reduce((acc, i) => { + // index 2 is the logged argument + acc[0].push(tryJsonParse(i[2])) + // index 1 is the meta object + acc[1].push(i[1][JSON_ERROR_KEY]) + return acc + }, [ + // meta also contains the meta object passed to flush + [], [meta[JSON_ERROR_KEY]], + ]) + + const items = buffered[0].filter(Boolean) + const errors = buffered[1].filter(Boolean) + + // If all items are keyed with array indexes, then we return the + // array. This skips any error checking since we cant really set + // an error property on an array in a way that can be stringified + // XXX(BREAKING_CHANGE): remove this in favor of always returning an object + /* istanbul ignore next - premature optimization for a PR that comes next */ + if (items.length && items.every((o, i) => Object.hasOwn(o, i))) { + return Object.assign([], ...items) + } + + const res = Object.assign({}, ...items) + + if (errors.length) { + // This is not ideal. JSON output has always been keyed at the root with an `error` + // key, so we cant change that without it being a breaking change. At the same time + // some commands output arbitrary keys at the top level of the output, such as package + // names. So the output could already have the same key. The choice here is to overwrite + // it with our error since that is (probably?) more important. + // XXX(BREAKING_CHANGE): all json output should be keyed under well known keys, eg `result` and `error` + /* istanbul ignore next */ + if (res[ERROR_KEY]) { + log.warn('display', `overwriting existing ${ERROR_KEY} on json output`) + } + res[ERROR_KEY] = Object.assign({}, ...errors) + } + + // Only write output if we have some json buffered + return Object.keys(res).length ? res : null +} + const withMeta = (handler) => (level, ...args) => { let meta = {} const last = args.at(-1) @@ -240,24 +291,17 @@ class Display { // directly as a listener and still reference "this" #outputHandler = withMeta((level, meta, ...args) => { switch (level) { - case output.KEYS.flush: + case output.KEYS.flush: { this.#outputState.buffering = false - if (meta.jsonError && this.#json) { - const json = {} - for (const item of this.#outputState.buffer) { - // index 2 skips the level and meta - Object.assign(json, tryJsonParse(item[2])) - } - this.#writeOutput( - output.KEYS.standard, - meta, - JSON.stringify({ ...json, error: meta.jsonError }, null, 2) - ) + const json = this.#json ? mergeJson(meta, this.#outputState.buffer) : null + if (json) { + this.#writeOutput(output.KEYS.standard, meta, JSON.stringify(json, null, 2)) } else { this.#outputState.buffer.forEach((item) => this.#writeOutput(...item)) } this.#outputState.buffer.length = 0 break + } case output.KEYS.buffer: this.#outputState.buffer.push([output.KEYS.standard, meta, ...args]) diff --git a/lib/utils/error-message.js b/lib/utils/error-message.js index 3d1b18f29dab6..b47cc5690a4f3 100644 --- a/lib/utils/error-message.js +++ b/lib/utils/error-message.js @@ -4,21 +4,17 @@ const { redactLog: replaceInfo } = require('@npmcli/redact') const { log } = require('proc-log') const errorMessage = (er, npm) => { - const short = [] + const summary = [] const detail = [] const files = [] - if (er.message) { - er.message = replaceInfo(er.message) - } - if (er.stack) { - er.stack = replaceInfo(er.stack) - } + er.message &&= replaceInfo(er.message) + er.stack &&= replaceInfo(er.stack) switch (er.code) { case 'ERESOLVE': { const { report } = require('./explain-eresolve.js') - short.push(['ERESOLVE', er.message]) + summary.push(['ERESOLVE', er.message]) detail.push(['', '']) // XXX(display): error messages are logged so we use the logColor since that is based // on stderr. This should be handled solely by the display layer so it could also be @@ -31,18 +27,18 @@ const errorMessage = (er, npm) => { case 'ENOLOCK': { const cmd = npm.command || '' - short.push([cmd, 'This command requires an existing lockfile.']) + summary.push([cmd, 'This command requires an existing lockfile.']) detail.push([cmd, 'Try creating one first with: npm i --package-lock-only']) detail.push([cmd, `Original error: ${er.message}`]) break } case 'ENOAUDIT': - short.push(['audit', er.message]) + summary.push(['audit', er.message]) break case 'ECONNREFUSED': - short.push(['', er]) + summary.push(['', er]) detail.push([ '', [ @@ -62,7 +58,7 @@ const errorMessage = (er, npm) => { if (process.platform !== 'win32' && (isCachePath || isCacheDest)) { // user probably doesn't need this, but still add it to the debug log log.verbose(er.stack) - short.push([ + summary.push([ '', [ '', @@ -76,7 +72,7 @@ const errorMessage = (er, npm) => { ].join('\n'), ]) } else { - short.push(['', er]) + summary.push(['', er]) detail.push([ '', [ @@ -97,7 +93,7 @@ const errorMessage = (er, npm) => { } case 'ENOGIT': - short.push(['', er.message]) + summary.push(['', er.message]) detail.push([ '', ['', 'Failed using git.', 'Please check if you have git installed and in your PATH.'].join( @@ -123,7 +119,7 @@ const errorMessage = (er, npm) => { break } } - short.push(['JSON.parse', er.message]) + summary.push(['JSON.parse', er.message]) detail.push([ 'JSON.parse', [ @@ -137,7 +133,7 @@ const errorMessage = (er, npm) => { case 'E401': // E401 is for places where we accidentally neglect OTP stuff if (er.code === 'EOTP' || /one-time pass/.test(er.message)) { - short.push(['', 'This operation requires a one-time password from your authenticator.']) + summary.push(['', 'This operation requires a one-time password from your authenticator.']) detail.push([ '', [ @@ -155,16 +151,16 @@ const errorMessage = (er, npm) => { : er.headers['www-authenticate'].map(au => au.split(/[,\s]+/))[0] if (auth.includes('Bearer')) { - short.push([ + summary.push([ '', 'Unable to authenticate, your authentication token seems to be invalid.', ]) detail.push([ '', - ['To correct this please try logging in again with:', ' npm login'].join('\n'), + ['To correct this please try logging in again with:', ' npm login'].join('\n'), ]) } else if (auth.includes('Basic')) { - short.push(['', 'Incorrect or missing password.']) + summary.push(['', 'Incorrect or missing password.']) detail.push([ '', [ @@ -172,22 +168,22 @@ const errorMessage = (er, npm) => { 'authentication token or enable two-factor authentication then', 'that means you likely typed your password in incorrectly.', 'Please try again, or recover your password at:', - ' https://www.npmjs.com/forgot', + ' https://www.npmjs.com/forgot', '', 'If you were doing some other operation then your saved credentials are', 'probably out of date. To correct this please try logging in again with:', - ' npm login', + ' npm login', ].join('\n'), ]) } else { - short.push(['', er.message || er]) + summary.push(['', er.message || er]) } } break case 'E404': // There's no need to have 404 in the message as well. - short.push(['404', er.message.replace(/^404\s+/, '')]) + summary.push(['404', er.message.replace(/^404\s+/, '')]) if (er.pkgid && er.pkgid !== '-') { const pkg = er.pkgid.replace(/(?!^)@.*$/, '') @@ -210,16 +206,16 @@ const errorMessage = (er, npm) => { break case 'EPUBLISHCONFLICT': - short.push(['publish fail', 'Cannot publish over existing version.']) + summary.push(['publish fail', 'Cannot publish over existing version.']) detail.push(['publish fail', "Update the 'version' field in package.json and try again."]) detail.push(['publish fail', '']) detail.push(['publish fail', 'To automatically increment version numbers, see:']) - detail.push(['publish fail', ' npm help version']) + detail.push(['publish fail', ' npm help version']) break case 'EISGIT': - short.push(['git', er.message]) - short.push(['git', ' ' + er.path]) + summary.push(['git', er.message]) + summary.push(['git', ` ${er.path}`]) detail.push([ 'git', ['Refusing to remove it. Update manually,', 'or move it out of the way first.'].join('\n'), @@ -255,7 +251,7 @@ const errorMessage = (er, npm) => { detailEntry.push(`Actual ${key}:${' '.repeat(padding)}${actual[key]}`) } - short.push([ + summary.push([ 'notsup', [ format( @@ -274,14 +270,14 @@ const errorMessage = (er, npm) => { } case 'EEXIST': - short.push(['', er.message]) - short.push(['', 'File exists: ' + (er.dest || er.path)]) + summary.push(['', er.message]) + summary.push(['', 'File exists: ' + (er.dest || er.path)]) detail.push(['', 'Remove the existing file and try again, or run npm']) detail.push(['', 'with --force to overwrite files recklessly.']) break case 'ENEEDAUTH': - short.push(['need auth', er.message]) + summary.push(['need auth', er.message]) detail.push(['need auth', 'You need to authorize this machine using `npm adduser`']) break @@ -290,7 +286,7 @@ const errorMessage = (er, npm) => { case 'ETIMEDOUT': case 'ERR_SOCKET_TIMEOUT': case 'EAI_FAIL': - short.push(['network', er.message]) + summary.push(['network', er.message]) detail.push([ 'network', [ @@ -303,7 +299,7 @@ const errorMessage = (er, npm) => { break case 'ETARGET': - short.push(['notarget', er.message]) + summary.push(['notarget', er.message]) detail.push([ 'notarget', [ @@ -314,7 +310,7 @@ const errorMessage = (er, npm) => { break case 'E403': - short.push(['403', er.message]) + summary.push(['403', er.message]) detail.push([ '403', [ @@ -326,8 +322,8 @@ const errorMessage = (er, npm) => { break case 'EBADENGINE': - short.push(['engine', er.message]) - short.push(['engine', 'Not compatible with your version of node/npm: ' + er.pkgid]) + summary.push(['engine', er.message]) + summary.push(['engine', 'Not compatible with your version of node/npm: ' + er.pkgid]) detail.push([ 'notsup', [ @@ -343,7 +339,7 @@ const errorMessage = (er, npm) => { break case 'ENOSPC': - short.push(['nospc', er.message]) + summary.push(['nospc', er.message]) detail.push([ 'nospc', [ @@ -354,7 +350,7 @@ const errorMessage = (er, npm) => { break case 'EROFS': - short.push(['rofs', er.message]) + summary.push(['rofs', er.message]) detail.push([ 'rofs', [ @@ -365,7 +361,7 @@ const errorMessage = (er, npm) => { break case 'ENOENT': - short.push(['enoent', er.message]) + summary.push(['enoent', er.message]) detail.push([ 'enoent', [ @@ -379,18 +375,18 @@ const errorMessage = (er, npm) => { case 'EUNKNOWNTYPE': case 'EINVALIDTYPE': case 'ETOOMANYARGS': - short.push(['typeerror', er.stack]) + summary.push(['typeerror', er.stack]) detail.push([ 'typeerror', [ 'This is an error with npm itself. Please report this error at:', - ' https://github.com/npm/cli/issues', + ' https://github.com/npm/cli/issues', ].join('\n'), ]) break default: - short.push(['', er.message || er]) + summary.push(['', er.message || er]) if (er.cause) { detail.push(['cause', er.cause.message]) } @@ -408,7 +404,12 @@ const errorMessage = (er, npm) => { } break } - return { summary: short, detail, files } + + return { + summary, + detail, + files, + } } const getExitCodeFromError = (err) => { @@ -419,7 +420,77 @@ const getExitCodeFromError = (err) => { } } +const getError = (err, { npm, command, pkg }) => { + // if we got a command that just shells out to something else, then it + // will presumably print its own errors and exit with a proper status + // code if there's a problem. If we got an error with a code=0, then... + // something else went wrong along the way, so maybe an npm problem? + if (command?.constructor?.isShellout && typeof err.code === 'number' && err.code) { + return { + exitCode: err.code, + suppressError: true, + } + } + + // XXX: we should stop throwing strings + if (typeof err === 'string') { + return { + exitCode: 1, + suppressError: true, + summary: [['', err]], + } + } + + // XXX: we should stop throwing other non-errors + if (!(err instanceof Error)) { + return { + exitCode: 1, + suppressError: true, + summary: [['weird error', err]], + } + } + + if (err.code === 'EUNKNOWNCOMMAND') { + const suggestions = require('./did-you-mean.js')(pkg, err.command) + return { + exitCode: 1, + suppressError: true, + standard: [ + `Unknown command: "${err.command}"`, + suggestions, + 'To see a list of supported npm commands, run:', + ' npm help', + ], + } + } + + // Anything after this is not suppressed and get more logged information + + // add a code to the error if it doesnt have one and mutate some properties + // so they have redacted information + err.code ??= err.message.match(/^(?:Error: )?(E[A-Z]+)/)?.[1] + // this mutates the error and redacts stack/message + const { summary, detail, files } = errorMessage(err, npm) + + return { + err, + code: err.code, + exitCode: getExitCodeFromError(err) || 1, + suppressError: false, + summary, + detail, + files, + verbose: ['type', 'stack', 'statusCode', 'pkgid'] + .filter(k => err[k]) + .map(k => [k, replaceInfo(err[k])]), + error: ['code', 'syscall', 'file', 'path', 'dest', 'errno'] + .filter(k => err[k]) + .map(k => [k, err[k]]), + } +} + module.exports = { getExitCodeFromError, errorMessage, + getError, } diff --git a/lib/utils/output-error.js b/lib/utils/output-error.js new file mode 100644 index 0000000000000..27128e9f03a8c --- /dev/null +++ b/lib/utils/output-error.js @@ -0,0 +1,29 @@ +const { log, output } = require('proc-log') + +const outputError = ({ standard = [], verbose = [], error = [], summary = [], detail = [] }) => { + for (const line of standard) { + // Each output line is just a single string + output.standard(line) + } + for (const line of verbose) { + log.verbose(...line) + } + for (const line of [...error, ...summary, ...detail]) { + log.error(...line) + } +} + +const jsonError = (error, npm) => { + if (error && npm?.loaded && npm?.config.get('json')) { + return { + code: error.code, + summary: (error.summary || []).map(l => l.slice(1).join(' ')).join('\n').trim(), + detail: (error.detail || []).map(l => l.slice(1).join(' ')).join('\n').trim(), + } + } +} + +module.exports = { + outputError, + jsonError, +} diff --git a/tap-snapshots/test/lib/utils/error-message.js.test.cjs b/tap-snapshots/test/lib/utils/error-message.js.test.cjs index ab0f5501deb62..969a501505c30 100644 --- a/tap-snapshots/test/lib/utils/error-message.js.test.cjs +++ b/tap-snapshots/test/lib/utils/error-message.js.test.cjs @@ -1021,11 +1021,11 @@ Object { authentication token or enable two-factor authentication then that means you likely typed your password in incorrectly. Please try again, or recover your password at: - https://www.npmjs.com/forgot + https://www.npmjs.com/forgot If you were doing some other operation then your saved credentials are probably out of date. To correct this please try logging in again with: - npm login + npm login ), ], ], @@ -1045,7 +1045,7 @@ Object { "", String( To correct this please try logging in again with: - npm login + npm login ), ], ], @@ -1283,7 +1283,7 @@ Object { "typeerror", String( This is an error with npm itself. Please report this error at: - https://github.com/npm/cli/issues + https://github.com/npm/cli/issues ), ], ], @@ -1303,7 +1303,7 @@ Object { "typeerror", String( This is an error with npm itself. Please report this error at: - https://github.com/npm/cli/issues + https://github.com/npm/cli/issues ), ], ], @@ -1323,7 +1323,7 @@ Object { "typeerror", String( This is an error with npm itself. Please report this error at: - https://github.com/npm/cli/issues + https://github.com/npm/cli/issues ), ], ], @@ -1364,7 +1364,7 @@ Object { "typeerror", String( This is an error with npm itself. Please report this error at: - https://github.com/npm/cli/issues + https://github.com/npm/cli/issues ), ], ], @@ -1505,7 +1505,7 @@ Object { ], Array [ "publish fail", - " npm help version", + " npm help version", ], ], "summary": Array [ @@ -1535,7 +1535,7 @@ Object { ], Array [ "git", - " /some/path", + " /some/path", ], ], } diff --git a/test/lib/cli/exit-handler.js b/test/lib/cli/exit-handler.js index a95ac75e56c14..0077596fab642 100644 --- a/test/lib/cli/exit-handler.js +++ b/test/lib/cli/exit-handler.js @@ -74,6 +74,10 @@ const mockExitHandler = async (t, { ...errorMessage.errorMessage(err), ...(files ? { files } : {}), }), + getError: (...args) => ({ + ...errorMessage.getError(...args), + ...(files ? { files } : {}), + }), } if (error) { diff --git a/test/lib/npm.js b/test/lib/npm.js index 6525708dc5795..a4f12f01c500a 100644 --- a/test/lib/npm.js +++ b/test/lib/npm.js @@ -561,9 +561,9 @@ t.test('usage', async t => { }) t.test('print usage if non-command param provided', async t => { - const { npm, outputs } = await loadMockNpm(t) + const { npm, joinedOutput } = await loadMockNpm(t) await t.rejects(npm.exec('tset'), { command: 'tset', exitCode: 1 }) - t.match(outputs[0], 'Unknown command: "tset"') - t.match(outputs[0], 'Did you mean this?') + t.match(joinedOutput(), 'Unknown command: "tset"') + t.match(joinedOutput(), 'Did you mean this?') }) diff --git a/test/lib/utils/did-you-mean.js b/test/lib/utils/did-you-mean.js index d111c2f002960..752999bf9c143 100644 --- a/test/lib/utils/did-you-mean.js +++ b/test/lib/utils/did-you-mean.js @@ -1,54 +1,47 @@ const t = require('tap') - const dym = require('../../../lib/utils/did-you-mean.js') t.test('did-you-mean', async t => { t.test('with package.json', async t => { - const testdir = t.testdir({ - 'package.json': JSON.stringify({ - bin: { - npx: 'exists', - }, - scripts: { - install: 'exists', - posttest: 'exists', - }, - }), - }) + const pkg = { + bin: { + npx: 'exists', + }, + scripts: { + install: 'exists', + posttest: 'exists', + }, + } t.test('nistall', async t => { - const result = await dym(testdir, 'nistall') + const result = dym(pkg, 'nistall') t.match(result, 'npm install') }) t.test('sttest', async t => { - const result = await dym(testdir, 'sttest') + const result = dym(pkg, 'sttest') t.match(result, 'npm test') t.match(result, 'npm run posttest') }) t.test('npz', async t => { - const result = await dym(testdir, 'npxx') + const result = dym(pkg, 'npxx') t.match(result, 'npm exec npx') }) t.test('qwuijbo', async t => { - const result = await dym(testdir, 'qwuijbo') + const result = dym(pkg, 'qwuijbo') t.match(result, '') }) }) t.test('with no package.json', t => { - const testdir = t.testdir({}) t.test('nistall', async t => { - const result = await dym(testdir, 'nistall') + const result = dym(null, 'nistall') t.match(result, 'npm install') }) t.end() }) t.test('missing bin and script properties', async t => { - const testdir = t.testdir({ - 'package.json': JSON.stringify({ - name: 'missing-bin', - }), - }) - - const result = await dym(testdir, 'nistall') + const pkg = { + name: 'missing-bin', + } + const result = dym(pkg, 'nistall') t.match(result, 'npm install') }) }) diff --git a/test/lib/utils/error-message.js b/test/lib/utils/error-message.js index 55b6053985a8c..c99971fd41233 100644 --- a/test/lib/utils/error-message.js +++ b/test/lib/utils/error-message.js @@ -93,17 +93,12 @@ t.test('just simple messages', async t => { t.test('replace message/stack sensistive info', async t => { const { errorMessage } = await loadMockNpm(t, { command: 'audit' }) - const path = '/some/path' - const pkgid = 'some@package' - const file = '/some/file' - const stack = 'dummy stack trace at https://user:pass@registry.npmjs.org/' - const message = 'Error at registry: https://user:pass@registry.npmjs.org/' - const er = Object.assign(new Error(message), { + const er = Object.assign(new Error('Error at registry: https://user:pass@registry.npmjs.org/'), { code: 'ENOAUDIT', - path, - pkgid, - file, - stack, + path: '/some/path', + pkgid: 'some@package', + file: '/some/file', + stack: 'dummy stack trace at https://user:pass@registry.npmjs.org/', }) t.matchSnapshot(errorMessage(er)) }) From df42eaf2edbcc96ffe655f91cbbf847fd194cb52 Mon Sep 17 00:00:00 2001 From: Luke Karrys Date: Sun, 12 May 2024 23:15:54 -0700 Subject: [PATCH 2/2] fixup! fix(refactor): split error messaging into more fns --- lib/utils/error-message.js | 276 +++++++----------- .../test/lib/utils/error-message.js.test.cjs | 36 ++- 2 files changed, 130 insertions(+), 182 deletions(-) diff --git a/lib/utils/error-message.js b/lib/utils/error-message.js index b47cc5690a4f3..c484c1ab9c2b6 100644 --- a/lib/utils/error-message.js +++ b/lib/utils/error-message.js @@ -39,13 +39,11 @@ const errorMessage = (er, npm) => { case 'ECONNREFUSED': summary.push(['', er]) - detail.push([ + detail.push(['', [ '', - [ - '\nIf you are behind a proxy, please make sure that the', - "'proxy' config is set properly. See: 'npm help config'", - ].join('\n'), - ]) + 'If you are behind a proxy, please make sure that the', + "'proxy' config is set properly. See: 'npm help config'", + ].join('\n')]) break case 'EACCES': @@ -58,48 +56,41 @@ const errorMessage = (er, npm) => { if (process.platform !== 'win32' && (isCachePath || isCacheDest)) { // user probably doesn't need this, but still add it to the debug log log.verbose(er.stack) - summary.push([ + summary.push(['', [ '', - [ - '', - 'Your cache folder contains root-owned files, due to a bug in', - 'previous versions of npm which has since been addressed.', - '', - 'To permanently fix this problem, please run:', - ` sudo chown -R ${process.getuid()}:${process.getgid()} ${JSON.stringify( - npm.config.get('cache') - )}`, - ].join('\n'), - ]) + 'Your cache folder contains root-owned files, due to a bug in', + 'previous versions of npm which has since been addressed.', + '', + 'To permanently fix this problem, please run:', + ` sudo chown -R ${process.getuid()}:${process.getgid()} "${npm.config.get('cache')}"`, + ].join('\n')]) } else { summary.push(['', er]) - detail.push([ + detail.push(['', [ '', - [ - '\nThe operation was rejected by your operating system.', - process.platform === 'win32' - /* eslint-disable-next-line max-len */ - ? "It's possible that the file was already in use (by a text editor or antivirus),\n" + - 'or that you lack permissions to access it.' - /* eslint-disable-next-line max-len */ - : 'It is likely you do not have the permissions to access this file as the current user', - '\nIf you believe this might be a permissions issue, please double-check the', - 'permissions of the file and its containing directories, or try running', - 'the command again as root/Administrator.', - ].join('\n'), - ]) + 'The operation was rejected by your operating system.', + ...process.platform === 'win32' ? [ + "It's possible that the file was already in use (by a text editor or antivirus),", + 'or that you lack permissions to access it.', + ] : [ + 'It is likely you do not have the permissions to access this file as the current user', + ], + '', + 'If you believe this might be a permissions issue, please double-check the', + 'permissions of the file and its containing directories, or try running', + 'the command again as root/Administrator.', + ].join('\n')]) } break } case 'ENOGIT': summary.push(['', er.message]) - detail.push([ + detail.push(['', [ '', - ['', 'Failed using git.', 'Please check if you have git installed and in your PATH.'].join( - '\n' - ), - ]) + 'Failed using git.', + 'Please check if you have git installed and in your PATH.', + ].join('\n')]) break case 'EJSONPARSE': @@ -108,25 +99,19 @@ const errorMessage = (er, npm) => { const { isDiff } = require('parse-conflict-json') const txt = require('fs').readFileSync(er.path, 'utf8').replace(/\r\n/g, '\n') if (isDiff(txt)) { - detail.push([ + detail.push(['', [ + 'Merge conflict detected in your package.json.', '', - [ - 'Merge conflict detected in your package.json.', - '', - 'Please resolve the package.json conflict and retry.', - ].join('\n'), - ]) + 'Please resolve the package.json conflict and retry.', + ].join('\n')]) break } } summary.push(['JSON.parse', er.message]) - detail.push([ - 'JSON.parse', - [ - 'Failed to parse JSON data.', - 'Note: package.json must be actual JSON, not just JavaScript.', - ].join('\n'), - ]) + detail.push(['JSON.parse', [ + 'Failed to parse JSON data.', + 'Note: package.json must be actual JSON, not just JavaScript.', + ].join('\n')]) break case 'EOTP': @@ -134,47 +119,39 @@ const errorMessage = (er, npm) => { // E401 is for places where we accidentally neglect OTP stuff if (er.code === 'EOTP' || /one-time pass/.test(er.message)) { summary.push(['', 'This operation requires a one-time password from your authenticator.']) - detail.push([ - '', - [ - 'You can provide a one-time password by passing --otp= to the command you ran.', - 'If you already provided a one-time password then it is likely that you either typoed', - 'it, or it timed out. Please try again.', - ].join('\n'), - ]) + detail.push(['', [ + 'You can provide a one-time password by passing --otp= to the command you ran.', + 'If you already provided a one-time password then it is likely that you either typoed', + 'it, or it timed out. Please try again.', + ].join('\n')]) } else { // npm ERR! code E401 // npm ERR! Unable to authenticate, need: Basic - const auth = - !er.headers || !er.headers['www-authenticate'] - ? [] - : er.headers['www-authenticate'].map(au => au.split(/[,\s]+/))[0] + const auth = !er.headers || !er.headers['www-authenticate'] + ? [] + : er.headers['www-authenticate'].map(au => au.split(/[,\s]+/))[0] if (auth.includes('Bearer')) { - summary.push([ - '', + summary.push(['', 'Unable to authenticate, your authentication token seems to be invalid.', ]) - detail.push([ - '', - ['To correct this please try logging in again with:', ' npm login'].join('\n'), - ]) + detail.push(['', [ + 'To correct this please try logging in again with:', + ' npm login', + ].join('\n')]) } else if (auth.includes('Basic')) { summary.push(['', 'Incorrect or missing password.']) - detail.push([ + detail.push(['', [ + 'If you were trying to login, change your password, create an', + 'authentication token or enable two-factor authentication then', + 'that means you likely typed your password in incorrectly.', + 'Please try again, or recover your password at:', + ' https://www.npmjs.com/forgot', '', - [ - 'If you were trying to login, change your password, create an', - 'authentication token or enable two-factor authentication then', - 'that means you likely typed your password in incorrectly.', - 'Please try again, or recover your password at:', - ' https://www.npmjs.com/forgot', - '', - 'If you were doing some other operation then your saved credentials are', - 'probably out of date. To correct this please try logging in again with:', - ' npm login', - ].join('\n'), - ]) + 'If you were doing some other operation then your saved credentials are', + 'probably out of date. To correct this please try logging in again with:', + ' npm login', + ].join('\n')]) } else { summary.push(['', er.message || er]) } @@ -200,7 +177,8 @@ const errorMessage = (er, npm) => { errorsArray.forEach((item, idx) => detail.push(['404', ' ' + (idx + 1) + '. ' + item])) } - detail.push(['404', '\nNote that you can also install from a']) + detail.push(['404', '']) + detail.push(['404', 'Note that you can also install from a']) detail.push(['404', 'tarball, folder, http url, or git url.']) } break @@ -216,10 +194,10 @@ const errorMessage = (er, npm) => { case 'EISGIT': summary.push(['git', er.message]) summary.push(['git', ` ${er.path}`]) - detail.push([ - 'git', - ['Refusing to remove it. Update manually,', 'or move it out of the way first.'].join('\n'), - ]) + detail.push(['git', [ + 'Refusing to remove it. Update manually,', + 'or move it out of the way first.', + ].join('\n')]) break case 'EBADPLATFORM': { @@ -251,21 +229,13 @@ const errorMessage = (er, npm) => { detailEntry.push(`Actual ${key}:${' '.repeat(padding)}${actual[key]}`) } - summary.push([ - 'notsup', - [ - format( - 'Unsupported platform for %s: wanted %j (current: %j)', - er.pkgid, - expected, - actual - ), - ].join('\n'), - ]) - detail.push([ - 'notsup', - detailEntry.join('\n'), - ]) + summary.push(['notsup', format( + 'Unsupported platform for %s: wanted %j (current: %j)', + er.pkgid, + expected, + actual + )]) + detail.push(['notsup', detailEntry.join('\n')]) break } @@ -287,88 +257,65 @@ const errorMessage = (er, npm) => { case 'ERR_SOCKET_TIMEOUT': case 'EAI_FAIL': summary.push(['network', er.message]) - detail.push([ - 'network', - [ - 'This is a problem related to network connectivity.', - 'In most cases you are behind a proxy or have bad network settings.', - '\nIf you are behind a proxy, please make sure that the', - "'proxy' config is set properly. See: 'npm help config'", - ].join('\n'), - ]) + detail.push(['network', [ + 'This is a problem related to network connectivity.', + 'In most cases you are behind a proxy or have bad network settings.', + '', + 'If you are behind a proxy, please make sure that the', + "'proxy' config is set properly. See: 'npm help config'", + ].join('\n')]) break case 'ETARGET': summary.push(['notarget', er.message]) - detail.push([ - 'notarget', - [ - 'In most cases you or one of your dependencies are requesting', - "a package version that doesn't exist.", - ].join('\n'), - ]) + detail.push(['notarget', [ + 'In most cases you or one of your dependencies are requesting', + "a package version that doesn't exist.", + ].join('\n')]) break case 'E403': summary.push(['403', er.message]) - detail.push([ - '403', - [ - 'In most cases, you or one of your dependencies are requesting', - 'a package version that is forbidden by your security policy, or', - 'on a server you do not have access to.', - ].join('\n'), - ]) + detail.push(['403', [ + 'In most cases, you or one of your dependencies are requesting', + 'a package version that is forbidden by your security policy, or', + 'on a server you do not have access to.', + ].join('\n')]) break case 'EBADENGINE': summary.push(['engine', er.message]) summary.push(['engine', 'Not compatible with your version of node/npm: ' + er.pkgid]) - detail.push([ - 'notsup', - [ - 'Not compatible with your version of node/npm: ' + er.pkgid, - 'Required: ' + JSON.stringify(er.required), - 'Actual: ' + - JSON.stringify({ - npm: npm.version, - node: process.version, - }), - ].join('\n'), - ]) + detail.push(['notsup', [ + 'Not compatible with your version of node/npm: ' + er.pkgid, + 'Required: ' + JSON.stringify(er.required), + 'Actual: ' + + JSON.stringify({ npm: npm.version, node: process.version }), + ].join('\n')]) break case 'ENOSPC': summary.push(['nospc', er.message]) - detail.push([ - 'nospc', - [ - 'There appears to be insufficient space on your system to finish.', - 'Clear up some disk space and try again.', - ].join('\n'), - ]) + detail.push(['nospc', [ + 'There appears to be insufficient space on your system to finish.', + 'Clear up some disk space and try again.', + ].join('\n')]) break case 'EROFS': summary.push(['rofs', er.message]) - detail.push([ - 'rofs', - [ - 'Often virtualized file systems, or other file systems', - "that don't support symlinks, give this error.", - ].join('\n'), - ]) + detail.push(['rofs', [ + 'Often virtualized file systems, or other file systems', + "that don't support symlinks, give this error.", + ].join('\n')]) break case 'ENOENT': summary.push(['enoent', er.message]) - detail.push([ - 'enoent', - [ - 'This is related to npm not being able to find a file.', - er.file ? "\nCheck if the file '" + er.file + "' is present." : '', - ].join('\n'), - ]) + detail.push(['enoent', [ + 'This is related to npm not being able to find a file.', + er.file ? `\nCheck if the file '${er.file}' is present.` : '', + ].join('\n')]) break case 'EMISSINGARG': @@ -376,13 +323,10 @@ const errorMessage = (er, npm) => { case 'EINVALIDTYPE': case 'ETOOMANYARGS': summary.push(['typeerror', er.stack]) - detail.push([ - 'typeerror', - [ - 'This is an error with npm itself. Please report this error at:', - ' https://github.com/npm/cli/issues', - ].join('\n'), - ]) + detail.push(['typeerror', [ + 'This is an error with npm itself. Please report this error at:', + ' https://github.com/npm/cli/issues', + ].join('\n')]) break default: diff --git a/tap-snapshots/test/lib/utils/error-message.js.test.cjs b/tap-snapshots/test/lib/utils/error-message.js.test.cjs index 969a501505c30..4cfd91fd278cd 100644 --- a/tap-snapshots/test/lib/utils/error-message.js.test.cjs +++ b/tap-snapshots/test/lib/utils/error-message.js.test.cjs @@ -28,10 +28,11 @@ Object { ], Array [ "404", - String( - - Note that you can also install from a - ), + "", + ], + Array [ + "404", + "Note that you can also install from a", ], Array [ "404", @@ -70,10 +71,11 @@ Object { ], Array [ "404", - String( - - Note that you can also install from a - ), + "", + ], + Array [ + "404", + "Note that you can also install from a", ], Array [ "404", @@ -112,10 +114,11 @@ Object { ], Array [ "404", - String( - - Note that you can also install from a - ), + "", + ], + Array [ + "404", + "Note that you can also install from a", ], Array [ "404", @@ -157,10 +160,11 @@ Object { ], Array [ "404", - String( - - Note that you can also install from a - ), + "", + ], + Array [ + "404", + "Note that you can also install from a", ], Array [ "404",