From ef4c975f2784f70ab86a3e7e6e40c99279e5a3e3 Mon Sep 17 00:00:00 2001 From: Luke Karrys Date: Wed, 15 May 2024 19:37:10 -0700 Subject: [PATCH] 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. --- lib/commands/view.js | 20 ++- lib/utils/display.js | 4 +- .../test/lib/commands/view.js.test.cjs | 147 ++++++++++++++++++ test/fixtures/mock-logs.js | 7 + test/lib/commands/view.js | 110 ++++++++++++- 5 files changed, 282 insertions(+), 6 deletions(-) diff --git a/lib/commands/view.js b/lib/commands/view.js index 8c6b5be3a0643..a59b486f23e60 100644 --- a/lib/commands/view.js +++ b/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 + } } } diff --git a/lib/utils/display.js b/lib/utils/display.js index ede2588cacfc1..3716630dbd9b1 100644 --- a/lib/utils/display.js +++ b/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) } diff --git a/tap-snapshots/test/lib/commands/view.js.test.cjs b/tap-snapshots/test/lib/commands/view.js.test.cjs index 54ceb893ae58a..e6cd42d0d32a5 100644 --- a/tap-snapshots/test/lib/commands/view.js.test.cjs +++ b/tap-snapshots/test/lib/commands/view.js.test.cjs @@ -363,6 +363,153 @@ yellow@1.0.1 'claudia' yellow@1.0.2 'claudia' ` +exports[`test/lib/commands/view.js TAP workspaces 404 workspaces basic > must match snapshot 1`] = ` + +green@1.0.0 | 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 <c@yellow.com> +- isaacs <i@yellow.com> + +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": "c@yellow.com", + "twitter": "cyellow" + }, + { + "name": "isaacs", + "email": "i@yellow.com", + "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`] = ` + +green@1.0.0 | 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 <c@yellow.com> +- isaacs <i@yellow.com> + +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": { diff --git a/test/fixtures/mock-logs.js b/test/fixtures/mock-logs.js index ce4c189219467..a9277e4ce999c 100644 --- a/test/fixtures/mock-logs.js +++ b/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 diff --git a/test/lib/commands/view.js b/test/lib/commands/view.js index d15d62f8acdcb..2480a39a4a373 100644 --- a/test/lib/commands/view.js +++ b/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 => {