fix: pass strings to JSON.stringify in --json mode

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 } })