From 8b03990e16559841db1e60c663cba9873660b15d Mon Sep 17 00:00:00 2001 From: Luke Karrys Date: Thu, 16 May 2024 16:56:39 -0700 Subject: [PATCH] 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 --- lib/commands/pkg.js | 18 ++++++--------- lib/utils/display.js | 44 ++++++++++++++++++++---------------- test/lib/cli/exit-handler.js | 1 - test/lib/commands/view.js | 6 +++++ 4 files changed, 38 insertions(+), 31 deletions(-) diff --git a/lib/commands/pkg.js b/lib/commands/pkg.js index a108d79c51517..a011fc10be107 100644 --- a/lib/commands/pkg.js +++ b/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 }) { diff --git a/lib/utils/display.js b/lib/utils/display.js index 3716630dbd9b1..67a3b98c0417a 100644 --- a/lib/utils/display.js +++ b/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)) } diff --git a/test/lib/cli/exit-handler.js b/test/lib/cli/exit-handler.js index da050bbb0708e..90d130a399265 100644 --- a/test/lib/cli/exit-handler.js +++ b/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() diff --git a/test/lib/commands/view.js b/test/lib/commands/view.js index 2480a39a4a373..5da9182ddd55e 100644 --- a/test/lib/commands/view.js +++ b/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 } })