Skip to content

Commit

Permalink
fix(view): dont immediately exit on first workspace 404 (npm#7508)
Browse files Browse the repository at this point in the history
Fixes: npm#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.
  • Loading branch information
lukekarrys authored May 16, 2024
1 parent 328f63c commit ef4c975
Show file tree
Hide file tree
Showing 5 changed files with 282 additions and 6 deletions.
20 changes: 18 additions & 2 deletions lib/commands/view.js
Original file line number Diff line number Diff line change
@@ -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')
Expand All @@ -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)

Expand Down Expand Up @@ -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
}
}
}

Expand Down
4 changes: 1 addition & 3 deletions lib/utils/display.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand All @@ -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)
}
Expand Down
147 changes: 147 additions & 0 deletions tap-snapshots/test/lib/commands/view.js.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -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": {
Expand Down
7 changes: 7 additions & 0 deletions test/fixtures/mock-logs.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ const logsByTitle = (logs) => ({
module.exports = () => {
const outputs = []
const outputErrors = []
const fullOutput = []

const levelLogs = []
const logs = Object.defineProperties([], {
Expand Down Expand Up @@ -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
}

Expand All @@ -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))
},
},
}
Expand All @@ -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
Expand Down
110 changes: 109 additions & 1 deletion test/lib/commands/view.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {}) {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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 => {
Expand Down

0 comments on commit ef4c975

Please sign in to comment.