fix: pass strings to JSON.stringify in --json mode (#7540)

In refactoring this behavior previously plain strings were no longer
being passed through JSON.stringify even in json mode. This commit
changes that to the previous behavior which fixes the bug in `npm view`.
This also required changing the behavior of `npm pkg` which relied on
this.

Fixes #7537

Author: lukekarrys

lib/commands/pkg.js

@@ -59,28 +59,24 @@ class Pkg extends BaseCommand {
     this.npm.config.set('json', true)
     const pkgJson = await PackageJson.load(path)
 
-    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
+      // TODO(BREAKING_CHANGE): much like other places where we unwrap single
+      // item arrays this should go away. it makes the behavior unknown for users
+      // who don't already know the shape of the data.
       if (Object.keys(result).length === 1) {
-        unwrap = true
         result = result[args]
       }
     }
 
-    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)
-    }
+    // The display layer is responsible for calling JSON.stringify on the result
+    // TODO: https://github.com/npm/cli/issues/5508 a raw mode has been requested similar
+    // to jq -r. If that was added then this method should no longer set `json:true` all the time
+    output.buffer(workspace ? { [workspace]: result } : result)
   }
 
   async set (args, { path }) {

lib/utils/display.js

@@ -82,26 +82,31 @@ const ERROR_KEY = 'error'
 // is a json error that should be merged into the finished output
 const JSON_ERROR_KEY = 'jsonError'
 
+const isPlainObject = (v) => v && typeof v === 'object' && !Array.isArray(v)
+
 const getArrayOrObject = (items) => {
-  // Arrays cant be merged, if the first item is an array return that
-  if (Array.isArray(items[0])) {
-    return items[0]
-  }
-  // We use objects with 0,1,2,etc keys to merge array
-  if (items.length && items.every((o, i) => Object.hasOwn(o, i))) {
-    return Object.assign([], ...items)
+  if (items.length) {
+    const foundNonObject = items.find(o => !isPlainObject(o))
+    // Non-objects and arrays cant be merged, so just return the first item
+    if (foundNonObject) {
+      return foundNonObject
+    }
+    // We use objects with 0,1,2,etc keys to merge array
+    if (items.every((o, i) => Object.hasOwn(o, i))) {
+      return Object.assign([], ...items)
+    }
   }
-  // Otherwise its an object with all items merged together
-  return Object.assign({}, ...items)
+  // Otherwise its an object with all object items merged together
+  return Object.assign({}, ...items.filter(o => isPlainObject(o)))
 }
 
-const mergeJson = ({ [JSON_ERROR_KEY]: metaError }, buffer) => {
+const getJsonBuffer = ({ [JSON_ERROR_KEY]: metaError }, buffer) => {
   const items = []
   // meta also contains the meta object passed to flush
   const errors = metaError ? [metaError] : []
   // index 1 is the meta, 2 is the logged argument
   for (const [, { [JSON_ERROR_KEY]: error }, obj] of buffer) {
-    if (obj && typeof obj === 'object') {
+    if (obj) {
       items.push(obj)
     }
     if (error) {
@@ -113,13 +118,12 @@ const mergeJson = ({ [JSON_ERROR_KEY]: metaError }, buffer) => {
     return null
   }
 
-  // If all items are keyed with array indexes, then we return the
-  // array. This skips any error checking since we cant really set
-  // an error property on an array in a way that can be stringified
-  // XXX(BREAKING_CHANGE): remove this in favor of always returning an object
   const res = getArrayOrObject(items)
 
-  if (!Array.isArray(res) && errors.length) {
+  // This skips any error checking since we can only set an error property
+  // on an object that can be stringified
+  // XXX(BREAKING_CHANGE): remove this in favor of always returning an object with result and error keys
+  if (isPlainObject(res) && errors.length) {
     // This is not ideal. JSON output has always been keyed at the root with an `error`
     // key, so we cant change that without it being a breaking change. At the same time
     // some commands output arbitrary keys at the top level of the output, such as package
@@ -292,9 +296,11 @@ class Display {
     switch (level) {
       case output.KEYS.flush: {
         this.#outputState.buffering = false
-        const json = this.#json ? mergeJson(meta, this.#outputState.buffer) : null
-        if (json) {
-          this.#writeOutput(output.KEYS.standard, meta, JSON.stringify(json, null, 2))
+        if (this.#json) {
+          const json = getJsonBuffer(meta, this.#outputState.buffer)
+          if (json) {
+            this.#writeOutput(output.KEYS.standard, meta, JSON.stringify(json, null, 2))
+          }
         } else {
           this.#outputState.buffer.forEach((item) => this.#writeOutput(...item))
         }

test/lib/cli/exit-handler.js

@@ -277,7 +277,6 @@ t.test('merges output buffers errors with --json', async (t) => {
 
   output.buffer({ output_data: 1 })
   output.buffer({ more_data: 2 })
-  output.buffer('not json, will be ignored')
 
   await exitHandler()
 

test/lib/commands/view.js

@@ -406,6 +406,12 @@ t.test('package with --json and no versions', async t => {
   t.equal(joinedOutput(), '', 'no info to display')
 })
 
+t.test('package with --json and single string arg', async t => {
+  const { view, joinedOutput } = await loadMockNpm(t, { config: { json: true } })
+  await view.exec(['blue', 'dist-tags.latest'])
+  t.equal(JSON.parse(joinedOutput()), '1.0.0', 'no info to display')
+})
+
 t.test('package with single version', async t => {
   t.test('full json', async t => {
     const { view, joinedOutput } = await loadMockNpm(t, { config: { json: true } })