fix(refactor): create new error output primitives (#7515)

These will be used to generate normal and json error messages in the same format from both commands and the exit handler. This also does a few others things: - makes `did-you-mean` take a package so it can be sync and called more easily from the error handlers - standardize all error messages with 2 space indentation to match the rest of npm
npm · May 13, 2024 · b54cdb8 · b54cdb8
1 parent e40454c
commit b54cdb8
Show file tree

Hide file tree

Showing 11 changed files with 408 additions and 387 deletions.
diff --git 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
@@ -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,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 { 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) {
       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 () {

diff --git 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
@@ -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])