fix(view): dont immediately exit on first workspace 404 (#7508)

Fixes: #5444

This PR will continue running through all workspaces for `npm view` even
when a workspace encounters an `E404` error. This usually happens when
you run `npm view -ws` but have private workspaces. A future iteration
could log a different message if an `E404` is encountered on a private
workspace, but for this PR I wanted to keep it generic since there are a
number of reasons a request for a package manifest could 404.

Author: lukekarrys

lib/commands/view.js

@@ -1,7 +1,7 @@
 const columns = require('cli-columns')
 const { readFile } = require('fs/promises')
 const jsonParse = require('json-parse-even-better-errors')
-const { log, output } = require('proc-log')
+const { log, output, META } = require('proc-log')
 const npa = require('npm-package-arg')
 const { resolve } = require('path')
 const formatBytes = require('../utils/format-bytes.js')
@@ -11,6 +11,8 @@ const { inspect } = require('util')
 const { packument } = require('pacote')
 const Queryable = require('../utils/queryable.js')
 const BaseCommand = require('../base-cmd.js')
+const { getError } = require('../utils/error-message.js')
+const { jsonError, outputError } = require('../utils/output-error.js')
 
 const readJson = file => readFile(file, 'utf8').then(jsonParse)
 
@@ -76,10 +78,24 @@ class View extends BaseCommand {
       return this.exec([pkg, ...rest])
     }
 
+    const json = this.npm.config.get('json')
     await this.setWorkspaces()
 
     for (const name of this.workspaceNames) {
-      await this.#viewPackage(`${name}${pkg.slice(1)}`, rest, { workspace: true })
+      try {
+        await this.#viewPackage(`${name}${pkg.slice(1)}`, rest, { workspace: true })
+      } catch (e) {
+        const err = getError(e, { npm: this.npm, command: this })
+        if (err.code !== 'E404') {
+          throw e
+        }
+        if (json) {
+          output.buffer({ [META]: true, jsonError: { [name]: jsonError(err, this.npm) } })
+        } else {
+          outputError(err)
+        }
+        process.exitCode = err.exitCode
+      }
     }
   }
 

lib/utils/display.js

@@ -104,7 +104,6 @@ const mergeJson = ({ [JSON_ERROR_KEY]: metaError }, buffer) => {
     if (obj && typeof obj === 'object') {
       items.push(obj)
     }
-    /* istanbul ignore next - this is not used yet but will be */
     if (error) {
       errors.push(error)
     }
@@ -127,9 +126,8 @@ const mergeJson = ({ [JSON_ERROR_KEY]: metaError }, buffer) => {
     // 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`)
+      log.warn('', `overwriting existing ${ERROR_KEY} on json output`)
     }
     res[ERROR_KEY] = getArrayOrObject(errors)
   }

tap-snapshots/test/lib/commands/view.js.test.cjs

@@ -363,6 +363,153 @@ [email protected] 'claudia'
 [email protected] 'claudia'
 `
 
+exports[`test/lib/commands/view.js TAP workspaces 404 workspaces basic > must match snapshot 1`] = `
+
+[[email protected] | ACME | deps: 2 | versions: 2
+green is a very important color
+
+DEPRECATED!! - true
+
+keywords: colors, green, crayola
+
+bin: green
+
+dist
+.tarball: http://hm.green.com/1.0.0.tgz
+.shasum: 123
+.integrity: ---
+.unpackedSize: 1.0 GB
+
+dependencies:
+red: 1.0.0
+yellow: 1.0.0
+
+maintainers:
+- claudia <[[email protected]>
+- isaacs <[[email protected]>
+
+dist-tags:
+latest: 1.0.0
+error code E404
+error 404 404
+`
+
+exports[`test/lib/commands/view.js TAP workspaces 404 workspaces json > must match snapshot 1`] = `
+{
+  "green": {
+    "_id": "green",
+    "name": "green",
+    "dist-tags": {
+      "latest": "1.0.0"
+    },
+    "maintainers": [
+      {
+        "name": "claudia",
+        "email": "[email protected]",
+        "twitter": "cyellow"
+      },
+      {
+        "name": "isaacs",
+        "email": "[email protected]",
+        "twitter": "iyellow"
+      }
+    ],
+    "keywords": [
+      "colors",
+      "green",
+      "crayola"
+    ],
+    "versions": [
+      "1.0.0",
+      "1.0.1"
+    ],
+    "version": "1.0.0",
+    "description": "green is a very important color",
+    "bugs": {
+      "url": "http://bugs.green.com"
+    },
+    "deprecated": true,
+    "repository": {
+      "url": "http://repository.green.com"
+    },
+    "license": {
+      "type": "ACME"
+    },
+    "bin": {
+      "green": "bin/green.js"
+    },
+    "dependencies": {
+      "red": "1.0.0",
+      "yellow": "1.0.0"
+    },
+    "dist": {
+      "shasum": "123",
+      "tarball": "http://hm.green.com/1.0.0.tgz",
+      "integrity": "---",
+      "fileCount": 1,
+      "unpackedSize": 1000000000
+    }
+  },
+  "error": {
+    "missing-package": {
+      "code": "E404",
+      "summary": "404",
+      "detail": ""
+    }
+  }
+}
+`
+
+exports[`test/lib/commands/view.js TAP workspaces 404 workspaces json with package named error > must match snapshot 1`] = `
+warn overwriting existing error on json output
+{
+  "error": {
+    "missing-package": {
+      "code": "E404",
+      "summary": "404",
+      "detail": ""
+    }
+  }
+}
+`
+
+exports[`test/lib/commands/view.js TAP workspaces 404 workspaces non-404 error rejects > must match snapshot 1`] = `
+
+[[email protected] | ACME | deps: 2 | versions: 2
+green is a very important color
+
+DEPRECATED!! - true
+
+keywords: colors, green, crayola
+
+bin: green
+
+dist
+.tarball: http://hm.green.com/1.0.0.tgz
+.shasum: 123
+.integrity: ---
+.unpackedSize: 1.0 GB
+
+dependencies:
+red: 1.0.0
+yellow: 1.0.0
+
+maintainers:
+- claudia <[[email protected]>
+- isaacs <[[email protected]>
+
+dist-tags:
+latest: 1.0.0
+error Unknown error
+`
+
+exports[`test/lib/commands/view.js TAP workspaces 404 workspaces non-404 error rejects with single arg > must match snapshot 1`] = `
+green:
+1.0.0
+unknown-error:
+error Unknown error
+`
+
 exports[`test/lib/commands/view.js TAP workspaces all workspaces --json > must match snapshot 1`] = `
 {
   "green": {

test/fixtures/mock-logs.js

@@ -24,6 +24,7 @@ const logsByTitle = (logs) => ({
 module.exports = () => {
   const outputs = []
   const outputErrors = []
+  const fullOutput = []
 
   const levelLogs = []
   const logs = Object.defineProperties([], {
@@ -53,6 +54,7 @@ module.exports = () => {
         // in the future if/when we refactor what logs look like.
         if (!isLog(str)) {
           outputErrors.push(str)
+          fullOutput.push(str)
           return
         }
 
@@ -70,12 +72,14 @@ module.exports = () => {
         const level = stripAnsi(rawLevel)
 
         logs.push(str.replaceAll(prefix, `${level} `))
+        fullOutput.push(str.replaceAll(prefix, `${level} `))
         levelLogs.push({ level, message: str.replaceAll(prefix, '') })
       },
     },
     stdout: {
       write: (str) => {
         outputs.push(trimTrailingNewline(str))
+        fullOutput.push(trimTrailingNewline(str))
       },
     },
   }
@@ -88,9 +92,12 @@ module.exports = () => {
       clearOutput: () => {
         outputs.length = 0
         outputErrors.length = 0
+        fullOutput.length = 0
       },
       outputErrors,
       joinedOutputError: () => joinAndTrimTrailingNewlines(outputs),
+      fullOutput,
+      joinedFullOutput: () => joinAndTrimTrailingNewlines(fullOutput),
       logs,
       clearLogs: () => {
         levelLogs.length = 0

test/lib/commands/view.js

@@ -270,14 +270,45 @@ const packument = (nv, opts) => {
         },
       },
     },
+    // this is a packaged named error which will conflict with
+    // the error key in json output
+    error: {
+      _id: 'error',
+      name: 'error',
+      'dist-tags': {
+        latest: '1.0.0',
+      },
+      versions: {
+        '1.0.0': {
+          name: 'error',
+          version: '1.0.0',
+          dist: {
+            shasum: '123',
+            tarball: 'http://hm.error.com/1.0.0.tgz',
+            fileCount: 1,
+          },
+        },
+      },
+    },
   }
+
   if (nv.type === 'git') {
     return mocks[nv.hosted.project]
   }
+
   if (nv.raw === './blue') {
     return mocks.blue
   }
-  return mocks[nv.name]
+
+  if (mocks[nv.name]) {
+    return mocks[nv.name]
+  }
+
+  if (nv.name === 'unknown-error') {
+    throw new Error('Unknown error')
+  }
+
+  throw Object.assign(new Error('404'), { code: 'E404' })
 }
 
 const loadMockNpm = async function (t, opts = {}) {
@@ -543,6 +574,33 @@ t.test('workspaces', async t => {
     },
   }
 
+  const prefixDir404 = {
+    'test-workspace-b': {
+      'package.json': JSON.stringify({
+        name: 'missing-package',
+        version: '1.2.3',
+      }),
+    },
+  }
+
+  const prefixDirError = {
+    'test-workspace-b': {
+      'package.json': JSON.stringify({
+        name: 'unknown-error',
+        version: '1.2.3',
+      }),
+    },
+  }
+
+  const prefixPackageNamedError = {
+    'test-workspace-a': {
+      'package.json': JSON.stringify({
+        name: 'error',
+        version: '1.2.3',
+      }),
+    },
+  }
+
   t.test('all workspaces', async t => {
     const { view, joinedOutput } = await loadMockNpm(t, {
       prefixDir,
@@ -624,6 +682,56 @@ t.test('workspaces', async t => {
     t.matchSnapshot(joinedOutput())
     t.matchSnapshot(logs.warn, 'should have warning of ignoring workspaces')
   })
+
+  t.test('404 workspaces', async t => {
+    t.test('basic', async t => {
+      const { view, joinedFullOutput } = await loadMockNpm(t, {
+        prefixDir: { ...prefixDir, ...prefixDir404 },
+        config: { workspaces: true, loglevel: 'error' },
+      })
+      await view.exec([])
+      t.matchSnapshot(joinedFullOutput())
+      t.equal(process.exitCode, 1)
+    })
+
+    t.test('json', async t => {
+      const { view, joinedFullOutput } = await loadMockNpm(t, {
+        prefixDir: { ...prefixDir, ...prefixDir404 },
+        config: { workspaces: true, json: true, loglevel: 'error' },
+      })
+      await view.exec([])
+      t.matchSnapshot(joinedFullOutput())
+      t.equal(process.exitCode, 1)
+    })
+
+    t.test('json with package named error', async t => {
+      const { view, joinedFullOutput } = await loadMockNpm(t, {
+        prefixDir: { ...prefixDir, ...prefixDir404, ...prefixPackageNamedError },
+        config: { workspaces: true, json: true, loglevel: 'warn' },
+      })
+      await view.exec([])
+      t.matchSnapshot(joinedFullOutput())
+      t.equal(process.exitCode, 1)
+    })
+
+    t.test('non-404 error rejects', async t => {
+      const { view, joinedFullOutput } = await loadMockNpm(t, {
+        prefixDir: { ...prefixDir, ...prefixDirError },
+        config: { workspaces: true, loglevel: 'error' },
+      })
+      await t.rejects(view.exec([]))
+      t.matchSnapshot(joinedFullOutput())
+    })
+
+    t.test('non-404 error rejects with single arg', async t => {
+      const { view, joinedFullOutput } = await loadMockNpm(t, {
+        prefixDir: { ...prefixDir, ...prefixDirError },
+        config: { workspaces: true, loglevel: 'error' },
+      })
+      await t.rejects(view.exec(['.', 'version']))
+      t.matchSnapshot(joinedFullOutput())
+    })
+  })
 })
 
 t.test('completion', async t => {