Skip to content

Commit

Permalink
fix(refactor): split error messaging into more fns
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
lukekarrys committed May 13, 2024
1 parent e40454c commit db68fd2
Show file tree
Hide file tree
Showing 11 changed files with 286 additions and 213 deletions.
12 changes: 7 additions & 5 deletions lib/commands/run-script.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 ? `\n\n${suggestions}` : '',
'To see a list of scripts, run:',
' npm run',
].join('\n'))
}

// positional args only added to the main event, not pre/post
Expand Down
96 changes: 18 additions & 78 deletions lib/npm.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 () {
Expand Down Expand Up @@ -287,110 +288,49 @@ 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
this.finish()

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) {
throw err
}
}

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 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) {
for (const [file, content] of (error.files || [])) {
const filePath = `${this.logPath}${file}`
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 () {
Expand Down
49 changes: 22 additions & 27 deletions lib/utils/did-you-mean.js
Original file line number Diff line number Diff line change
@@ -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 `Did 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
70 changes: 57 additions & 13 deletions lib/utils/display.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ const tryJsonParse = (value) => {
try {
return JSON.parse(value)
} catch {
return {}
return
}
}
return value
Expand All @@ -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)
Expand Down Expand Up @@ -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])
Expand Down
Loading

0 comments on commit db68fd2

Please sign in to comment.