fix(refactor): use output buffer and error for more commands

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`.

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.

Author: lukekarrys

lib/commands/ls.js

@@ -554,7 +554,7 @@ const jsonOutput = ({ path, problems, result, rootError, seenItems }) => {
     }
   }
 
-  return JSON.stringify(result, null, 2)
+  return result
 }
 
 const parseableOutput = ({ global, long, seenNodes }) => {

lib/commands/pack.js

@@ -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(/\//, '-'))
+      }
     }
   }
 

lib/commands/pkg.js

@@ -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`),
@@ -42,57 +36,53 @@ 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 }) {
+    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.')
 
@@ -102,7 +92,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('=')
@@ -114,19 +104,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) {
@@ -136,13 +125,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())
   }
 }
 

lib/commands/publish.js

@@ -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')
@@ -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) {
@@ -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)
@@ -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',
@@ -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 = []