Skip to content

Commit

Permalink
fix(refactor): use output buffer and error for more commands (#7513)
Browse files Browse the repository at this point in the history 
This changes a bunch of commands to use the new `output.buffer`
capabilities from `proc-log` as well as the `outputError` helper from
`utils/output-error.js`.

This also adds a few comments about future display related breaking
changes that npm should make.

There is some new behavior around `run-script` and how it outputs
errors. It now displays the error for each workspace similar to how that
error would get displayed when the process exits.
  • Loading branch information
@lukekarrys
lukekarrys committed May 15, 2024
1 parent 12f103c commit d5c3289
Show file tree
Hide file tree
Showing 12 changed files with 342 additions and 375 deletions.
2 changes: 1 addition & 1 deletion lib/commands/ls.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -554,7 +554,7 @@ const jsonOutput = ({ path, problems, result, rootError, seenItems }) => {
}
}

return JSON.stringify(result, null, 2)
return result
}

const parseableOutput = ({ global, long, seenNodes }) => {
Expand Down
18 changes: 8 additions & 10 deletions lib/commands/pack.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,18 +53,16 @@ class Pack extends BaseCommand {
prefix: this.npm.localPrefix,
workspaces: this.workspacePaths,
})
const pkgContents = await getContents(manifest, tarballData)
tarballs.push(pkgContents)
tarballs.push(await getContents(manifest, tarballData))
}

if (json) {
output.standard(JSON.stringify(tarballs, null, 2))
return
}

for (const tar of tarballs) {
logTar(tar, { unicode })
output.standard(tar.filename.replace(/^@/, '').replace(/\//, '-'))
for (const [index, tar] of Object.entries(tarballs)) {
// XXX(BREAKING_CHANGE): publish outputs a json object with package
// names as keys. Pack should do the same here instead of an array
logTar(tar, { unicode, json, key: index })
if (!json) {
output.standard(tar.filename.replace(/^@/, '').replace(/\//, '-'))
}
}
}

Expand Down
74 changes: 29 additions & 45 deletions lib/commands/pkg.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,7 @@ class Pkg extends BaseCommand {
static workspaces = true
static ignoreImplicitWorkspace = false

async exec (args, { prefix } = {}) {
if (!prefix) {
this.prefix = this.npm.localPrefix
} else {
this.prefix = prefix
}

async exec (args, { path = this.npm.localPrefix, workspace } = {}) {
if (this.npm.global) {
throw Object.assign(
new Error(`There's no package.json file to manage on global mode`),
Expand All @@ -42,57 +36,54 @@ class Pkg extends BaseCommand {
const [cmd, ..._args] = args
switch (cmd) {
case 'get':
return this.get(_args)
return this.get(_args, { path, workspace })
case 'set':
return this.set(_args)
return this.set(_args, { path, workspace }).then(p => p.save())
case 'delete':
return this.delete(_args)
return this.delete(_args, { path, workspace }).then(p => p.save())
case 'fix':
return this.fix(_args)
return PackageJson.fix(path).then(p => p.save())
default:
throw this.usageError()
}
}

async execWorkspaces (args) {
await this.setWorkspaces()
const result = {}
for (const [workspaceName, workspacePath] of this.workspaces.entries()) {
this.prefix = workspacePath
result[workspaceName] = await this.exec(args, { prefix: workspacePath })
for (const [workspace, path] of this.workspaces.entries()) {
await this.exec(args, { path, workspace })
}
// when running in workspaces names, make sure to key by workspace
// name the results of each value retrieved in each ws
output.standard(JSON.stringify(result, null, 2))
}

async get (args) {
const pkgJson = await PackageJson.load(this.prefix)

const { content } = pkgJson
let result = !args.length && content
async get (args, { path, workspace }) {
this.npm.config.set('json', true)
const pkgJson = await PackageJson.load(path)

if (!result) {
const q = new Queryable(content)
result = q.query(args)
let unwrap = false
let result = pkgJson.content

if (args.length) {
result = new Queryable(result).query(args)
// in case there's only a single result from the query
// just prints that one element to stdout
if (Object.keys(result).length === 1) {
unwrap = true
result = result[args]
}
}

// only outputs if not running with workspaces config
// execWorkspaces will handle the output otherwise
if (!this.workspaces) {
output.standard(JSON.stringify(result, null, 2))
if (workspace) {
// workspaces are always json
output.buffer({ [workspace]: result })
} else {
// if the result was unwrapped, stringify as json which will add quotes around strings
// TODO: https://github.com/npm/cli/issues/5508 a raw mode has been requested similar
// to jq -r. If that was added then it would conditionally not call JSON.stringify here
output.buffer(unwrap ? JSON.stringify(result) : result)
}

return result
}

async set (args) {
async set (args, { path }) {
const setError = () =>
this.usageError('npm pkg set expects a key=value pair of args.')

Expand All @@ -102,7 +93,7 @@ class Pkg extends BaseCommand {

const force = this.npm.config.get('force')
const json = this.npm.config.get('json')
const pkgJson = await PackageJson.load(this.prefix)
const pkgJson = await PackageJson.load(path)
const q = new Queryable(pkgJson.content)
for (const arg of args) {
const [key, ...rest] = arg.split('=')
Expand All @@ -114,19 +105,18 @@ class Pkg extends BaseCommand {
q.set(key, json ? JSON.parse(value) : value, { force })
}

pkgJson.update(q.toJSON())
await pkgJson.save()
return pkgJson.update(q.toJSON())
}

async delete (args) {
async delete (args, { path }) {
const setError = () =>
this.usageError('npm pkg delete expects key args.')

if (!args.length) {
throw setError()
}

const pkgJson = await PackageJson.load(this.prefix)
const pkgJson = await PackageJson.load(path)
const q = new Queryable(pkgJson.content)
for (const key of args) {
if (!key) {
Expand All @@ -136,13 +126,7 @@ class Pkg extends BaseCommand {
q.delete(key)
}

pkgJson.update(q.toJSON())
await pkgJson.save()
}

async fix () {
const pkgJson = await PackageJson.fix(this.prefix)
await pkgJson.save()
return pkgJson.update(q.toJSON())
}
}

Expand Down
89 changes: 35 additions & 54 deletions lib/commands/publish.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,26 @@ class Publish extends BaseCommand {
throw this.usageError()
}

await this.#publish(args)
}

async execWorkspaces () {
await this.setWorkspaces()

for (const [name, workspace] of this.workspaces.entries()) {
try {
await this.#publish([workspace], { workspace: name })
} catch (err) {
if (err.code !== 'EPRIVATE') {
throw err
}
// eslint-disable-next-line max-len
log.warn('publish', `Skipping workspace ${this.npm.chalk.cyan(name)}, marked as ${this.npm.chalk.bold('private')}`)
}
}
}

async #publish (args, { workspace } = {}) {
log.verbose('publish', replaceInfo(args))

const unicode = this.npm.config.get('unicode')
Expand All @@ -61,7 +81,7 @@ class Publish extends BaseCommand {
// you can publish name@version, ./foo.tgz, etc.
// even though the default is the 'file:.' cwd.
const spec = npa(args[0])
let manifest = await this.getManifest(spec, opts)
let manifest = await this.#getManifest(spec, opts)

// only run scripts for directory type publishes
if (spec.type === 'directory' && !ignoreScripts) {
Expand All @@ -84,15 +104,17 @@ class Publish extends BaseCommand {
workspaces: this.workspacePaths,
})
const pkgContents = await getContents(manifest, tarballData)
const logPkg = () => logTar(pkgContents, { unicode, json, key: workspace })

// The purpose of re-reading the manifest is in case it changed,
// so that we send the latest and greatest thing to the registry
// note that publishConfig might have changed as well!
manifest = await this.getManifest(spec, opts, true)
manifest = await this.#getManifest(spec, opts, true)

// JSON already has the package contents
// If we are not in JSON mode then we show the user the contents of the tarball
// before it is published so they can see it while their otp is pending
if (!json) {
logTar(pkgContents, { unicode })
logPkg()
}

const resolved = npa.resolve(manifest.name, manifest.version)
Expand Down Expand Up @@ -126,6 +148,12 @@ class Publish extends BaseCommand {
await otplease(this.npm, opts, o => libpub(manifest, tarballData, o))
}

// In json mode we dont log until the publish has completed as this will
// add it to the output only if completes successfully
if (json) {
logPkg()
}

if (spec.type === 'directory' && !ignoreScripts) {
await runScript({
event: 'publish',
Expand All @@ -142,62 +170,15 @@ class Publish extends BaseCommand {
})
}

if (!this.suppressOutput) {
if (!silent && json) {
output.standard(JSON.stringify(pkgContents, null, 2))
} else if (!silent) {
output.standard(`+ ${pkgContents.id}`)
}
}

return pkgContents
}

async execWorkspaces () {
// Suppresses JSON output in publish() so we can handle it here
this.suppressOutput = true

const results = {}
const json = this.npm.config.get('json')
const { silent } = this.npm
await this.setWorkspaces()

for (const [name, workspace] of this.workspaces.entries()) {
let pkgContents
try {
pkgContents = await this.exec([workspace])
} catch (err) {
if (err.code === 'EPRIVATE') {
log.warn(
'publish',
`Skipping workspace ${
this.npm.chalk.cyan(name)
}, marked as ${
this.npm.chalk.bold('private')
}`
)
continue
}
throw err
}
// This needs to be in-line w/ the rest of the output that non-JSON
// publish generates
if (!silent && !json) {
output.standard(`+ ${pkgContents.id}`)
} else {
results[name] = pkgContents
}
}

if (!silent && json) {
output.standard(JSON.stringify(results, null, 2))
if (!json && !silent) {
output.standard(`+ ${pkgContents.id}`)
}
}

// if it's a directory, read it from the file system
// otherwise, get the full metadata from whatever it is
// XXX can't pacote read the manifest from a directory?
async getManifest (spec, opts, logWarnings = false) {
async #getManifest (spec, opts, logWarnings = false) {
let manifest
if (spec.type === 'directory') {
const changes = []
Expand Down

0 comments on commit d5c3289

Please sign in to comment.