From 63997f19b4dde20c201ce9b07368b1ac6f29bec3 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. --- 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..9ba9e3542920f 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] && !node[_dedupe] + ? `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..943b23c56f8a6 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 +\`-- 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 ++-- chai@1.0.0 extraneous +| \`-- dog@1.0.0 deduped +\`-- 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 + +` 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', ],