From 23ce3af199c8a14ef16c63fc638a1ac21fd9a9b0 Mon Sep 17 00:00:00 2001 From: isaacs Date: Wed, 23 Jun 2021 10:32:48 -0700 Subject: [PATCH] feat(ls): report *why* something is invalid This is a papercut that has been driving me crazy when debugging ERESOLVE issues. It's not particularly useful to just say something is "invalid", without showing which module's dependency is not being met. With this commit, it prints all the packages that depend on that dependency and do not have their required version met. This does print multiple times for deduped deps that are invalid, but if we skip the printing of the `invalid` label for deduped deps, we end up losing information that is only detected later in the tree walk. PR-URL: https://github.com/npm/cli/pull/3460 Credit: @isaacs Close: #3460 Reviewed-by: @nlf --- lib/ls.js | 19 +++++-- tap-snapshots/test/lib/ls.js.test.cjs | 31 +++++++---- test/lib/ls.js | 79 +++++++++++++++++++++++++-- 3 files changed, 109 insertions(+), 20 deletions(-) diff --git a/lib/ls.js b/lib/ls.js index c8d84651e2113..7540692911976 100644 --- a/lib/ls.js +++ b/lib/ls.js @@ -308,6 +308,9 @@ const getHumanOutputItem = (node, { args, color, global, long }) => { const targetLocation = node.root ? relative(node.root.realpath, node.realpath) : node.targetLocation + const invalid = node[_invalid] + ? `invalid: ${node[_invalid]}` + : '' const label = ( node[_missing] @@ -321,8 +324,8 @@ const getHumanOutputItem = (node, { args, color, global, long }) => { : '' ) + ( - node[_invalid] - ? ' ' + (color ? chalk.red.bgBlack('invalid') : 'invalid') + invalid + ? ' ' + (color ? chalk.red.bgBlack(invalid) : invalid) : '' ) + ( @@ -373,7 +376,7 @@ const getJsonOutputItem = (node, { global, long }) => { item.extraneous = true if (node[_invalid]) - item.invalid = true + item.invalid = node[_invalid] if (node[_missing] && !isOptional(node)) { item.required = node[_required] @@ -432,9 +435,15 @@ const mapEdgesToNodes = ({ seenPaths }) => (edge) => { if (node.path) seenPaths.add(node.path) - node[_required] = edge.spec + node[_required] = edge.spec || '*' node[_type] = edge.type - node[_invalid] = edge.invalid + + if (edge.invalid) { + const spec = JSON.stringify(node[_required]) + const from = edge.from.location || 'the root project' + node[_invalid] = (node[_invalid] ? node[_invalid] + ', ' : '') + + (`${spec} from ${from}`) + } return node } diff --git a/tap-snapshots/test/lib/ls.js.test.cjs b/tap-snapshots/test/lib/ls.js.test.cjs index 3d56d1f432731..c3d0a87648edb 100644 --- a/tap-snapshots/test/lib/ls.js.test.cjs +++ b/tap-snapshots/test/lib/ls.js.test.cjs @@ -32,16 +32,16 @@ exports[`test/lib/ls.js TAP ignore missing optional deps human output > ls resul test-npm-ls-ignore-missing-optional@1.2.3 {project} +-- unmet optional dependency optional-missing@1 +-- optional-ok@1.2.3 -+-- optional-wrong@3.2.1 invalid ++-- optional-wrong@3.2.1 invalid: "1" from the root project +-- unmet dependency peer-missing@1 +-- peer-ok@1.2.3 +-- unmet optional dependency peer-optional-missing@1 +-- peer-optional-ok@1.2.3 -+-- peer-optional-wrong@3.2.1 invalid -+-- peer-wrong@3.2.1 invalid ++-- peer-optional-wrong@3.2.1 invalid: "1" from the root project ++-- peer-wrong@3.2.1 invalid: "1" from the root project +-- unmet dependency prod-missing@1 +-- prod-ok@1.2.3 -\`-- prod-wrong@3.2.1 invalid +\`-- prod-wrong@3.2.1 invalid: "1" from the root project ` @@ -356,7 +356,7 @@ npm-broken-resolved-field-test@1.0.0 {CWD}/tap-testdir-ls-ls-broken-resolved-fie exports[`test/lib/ls.js TAP ls colored output > should output tree containing color info 1`] = ` test-npm-ls@1.0.0 {CWD}/tap-testdir-ls-ls-colored-output +-- chai@1.0.0 extraneous -+-- foo@1.0.0 invalid ++-- foo@1.0.0 invalid: "^2.0.0" from the root project | \`-- dog@1.0.0 \`-- UNMET DEPENDENCY ipsum@^1.0.0  @@ -454,8 +454,8 @@ exports[`test/lib/ls.js TAP ls global > should print tree and not mark top-level exports[`test/lib/ls.js TAP ls invalid deduped dep > should output tree signaling mismatching peer dep in problems 1`] = ` invalid-deduped-dep@1.0.0 {CWD}/tap-testdir-ls-ls-invalid-deduped-dep +-- a@1.0.0 -| \`-- b@1.0.0 deduped invalid -\`-- b@1.0.0 invalid +| \`-- b@1.0.0 deduped invalid: "^2.0.0" from the root project, "^2.0.0" from node_modules/a +\`-- b@1.0.0 invalid: "^2.0.0" from the root project, "^2.0.0" from node_modules/a  ` @@ -466,7 +466,7 @@ test-npm-ls@1.0.0 {CWD}/tap-testdir-ls-ls-invalid-peer-dep | \`-- foo@1.0.0 | \`-- dog@1.0.0 +-- optional-dep@1.0.0 -+-- peer-dep@1.0.0 invalid ++-- peer-dep@1.0.0 invalid: "^2.0.0" from the root project \`-- prod-dep@1.0.0 \`-- dog@2.0.0 @@ -567,7 +567,7 @@ exports[`test/lib/ls.js TAP ls missing package.json > should output tree missing exports[`test/lib/ls.js TAP ls missing/invalid/extraneous > should output tree containing missing, invalid, extraneous labels 1`] = ` test-npm-ls@1.0.0 {CWD}/tap-testdir-ls-ls-missing-invalid-extraneous +-- chai@1.0.0 extraneous -+-- foo@1.0.0 invalid ++-- foo@1.0.0 invalid: "^2.0.0" from the root project | \`-- dog@1.0.0 \`-- UNMET DEPENDENCY ipsum@^1.0.0 @@ -602,7 +602,7 @@ exports[`test/lib/ls.js TAP ls unmet optional dep > should output tree with empt | \`-- foo@1.0.0 | \`-- dog@1.0.0 +-- UNMET OPTIONAL DEPENDENCY missing-optional-dep@^1.0.0 -+-- optional-dep@1.0.0 invalid ++-- optional-dep@1.0.0 invalid: "^2.0.0" from the root project +-- peer-dep@1.0.0 \`-- prod-dep@1.0.0  \`-- dog@2.0.0 @@ -691,3 +691,14 @@ dedupe-entries@1.0.0 {CWD}/tap-testdir-ls-ls-with-no-args-dedupe-entries-and-not \`-- @npmcli/c@1.0.0 ` + +exports[`test/lib/ls.js TAP show multiple invalid reasons > ls result 1`] = ` +test-npm-ls@1.0.0 {cwd}/tap-testdir-ls-show-multiple-invalid-reasons ++-- cat@1.0.0 invalid: "^2.0.0" from the root project +| \`-- dog@1.0.0 deduped invalid: "^1.2.3" from the root project, "^2.0.0" from node_modules/cat ++-- chai@1.0.0 extraneous +| \`-- dog@1.0.0 deduped invalid: "^1.2.3" from the root project, "^2.0.0" from node_modules/cat, "2.x" from node_modules/chai +\`-- dog@1.0.0 invalid: "^1.2.3" from the root project, "^2.0.0" from node_modules/cat, "2.x" from node_modules/chai + \`-- cat@1.0.0 deduped invalid: "^2.0.0" from the root project + +` diff --git a/test/lib/ls.js b/test/lib/ls.js index 4d6fef52b629e..5f196501e55d1 100644 --- a/test/lib/ls.js +++ b/test/lib/ls.js @@ -123,7 +123,23 @@ const ls = new LS(npm) const redactCwd = res => res && res.replace(/\\+/g, '/').replace(new RegExp(__dirname.replace(/\\+/g, '/'), 'gi'), '{CWD}') -const jsonParse = res => JSON.parse(redactCwd(res)) +const redactCwdObj = obj => { + if (Array.isArray(obj)) + return obj.map(o => redactCwdObj(o)) + else if (typeof obj === 'string') + return redactCwd(obj) + else if (!obj) + return obj + else if (typeof obj === 'object') { + return Object.keys(obj).reduce((o, k) => { + o[k] = redactCwdObj(obj[k]) + return o + }, {}) + } else + return obj +} + +const jsonParse = res => redactCwdObj(JSON.parse(res)) const cleanUpResult = () => result = '' @@ -3060,7 +3076,7 @@ t.test('ls --json', (t) => { dependencies: { foo: { version: '1.0.0', - invalid: true, + invalid: '"^2.0.0" from the root project', problems: [ 'invalid: foo@1.0.0 {CWD}/tap-testdir-ls-ls---json-missing-invalid-extraneous/node_modules/foo', ], @@ -3756,7 +3772,7 @@ t.test('ls --json', (t) => { dependencies: { 'peer-dep': { version: '1.0.0', - invalid: true, + invalid: '"^2.0.0" from the root project', problems: [ 'invalid: peer-dep@1.0.0 {CWD}/tap-testdir-ls-ls---json-unmet-peer-dep/node_modules/peer-dep', ], @@ -3817,7 +3833,7 @@ t.test('ls --json', (t) => { dependencies: { 'optional-dep': { version: '1.0.0', - invalid: true, + invalid: '"^2.0.0" from the root project', problems: [ 'invalid: optional-dep@1.0.0 {CWD}/tap-testdir-ls-ls---json-unmet-optional-dep/node_modules/optional-dep', ], @@ -4175,6 +4191,59 @@ t.test('ls --json', (t) => { t.end() }) +t.test('show multiple invalid reasons', (t) => { + config.json = false + config.all = true + config.depth = Infinity + npm.prefix = t.testdir({ + 'package.json': JSON.stringify({ + name: 'test-npm-ls', + version: '1.0.0', + dependencies: { + cat: '^2.0.0', + dog: '^1.2.3', + }, + }), + node_modules: { + cat: { + 'package.json': JSON.stringify({ + name: 'cat', + version: '1.0.0', + dependencies: { + dog: '^2.0.0', + }, + }), + }, + dog: { + 'package.json': JSON.stringify({ + name: 'dog', + version: '1.0.0', + dependencies: { + cat: '', + }, + }), + }, + chai: { + 'package.json': JSON.stringify({ + name: 'chai', + version: '1.0.0', + dependencies: { + dog: '2.x', + }, + }), + }, + }, + }) + + const cleanupPaths = str => + redactCwd(str).toLowerCase().replace(/\\/g, '/') + ls.exec([], (err) => { + t.match(err, { code: 'ELSPROBLEMS' }, 'should list dep problems') + t.matchSnapshot(cleanupPaths(result), 'ls result') + t.end() + }) +}) + t.test('ls --package-lock-only', (t) => { config['package-lock-only'] = true t.test('ls --package-lock-only --json', (t) => { @@ -4751,7 +4820,7 @@ t.test('ls --package-lock-only', (t) => { dependencies: { foo: { version: '1.0.0', - invalid: true, + invalid: '"^2.0.0" from the root project', problems: [ 'invalid: foo@1.0.0 {CWD}/tap-testdir-ls-ls---package-lock-only-ls---package-lock-only---json-missing-invalid-extraneous/node_modules/foo', ],