Skip to content

Commit

Permalink
feat(ls): report *why* something is invalid
Browse files Browse the repository at this point in the history 
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: #3460
Credit: @isaacs
Close: #3460
Reviewed-by: @nlf
  • Loading branch information
@isaacs @wraithgar
isaacs authored and wraithgar committed Jun 23, 2021
1 parent 6254b6f commit 23ce3af
Show file tree
Hide file tree
Showing 3 changed files with 109 additions and 20 deletions.
19 changes: 14 additions & 5 deletions lib/ls.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand All @@ -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)
: ''
) +
(
Expand Down Expand Up @@ -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]
Expand Down Expand Up @@ -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
}
Expand Down
31 changes: 21 additions & 10 deletions tap-snapshots/test/lib/ls.js.test.cjs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
`

Expand Down Expand Up @@ -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
[0m+-- foo@1.0.0 [31m[40minvalid[49m[39m[0m
[0m+-- foo@1.0.0 [31m[40minvalid: "^2.0.0" from the root project[49m[39m[0m
| \`-- dog@1.0.0
\`-- UNMET DEPENDENCY ipsum@^1.0.0

Expand Down Expand Up @@ -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
[0m| \`-- b@1.0.0 [90mdeduped[39m [31m[40minvalid[49m[39m[0m
[0m\`-- b@1.0.0 [31m[40minvalid[49m[39m[0m
[0m| \`-- b@1.0.0 [90mdeduped[39m [31m[40minvalid: "^2.0.0" from the root project, "^2.0.0" from node_modules/a[49m[39m[0m
[0m\`-- b@1.0.0 [31m[40minvalid: "^2.0.0" from the root project, "^2.0.0" from node_modules/a[49m[39m[0m

`

Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
[0m+-- optional-dep@1.0.0 [31m[40minvalid[49m[39m[0m
[0m+-- optional-dep@1.0.0 [31m[40minvalid: "^2.0.0" from the root project[49m[39m[0m
+-- peer-dep@1.0.0
\`-- prod-dep@1.0.0
 \`-- dog@2.0.0
Expand Down Expand Up @@ -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
`
79 changes: 74 additions & 5 deletions test/lib/ls.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 = ''

Expand Down Expand Up @@ -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',
],
Expand Down Expand Up @@ -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',
],
Expand Down Expand Up @@ -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',
],
Expand Down Expand Up @@ -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) => {
Expand Down Expand Up @@ -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',
],
Expand Down

0 comments on commit 23ce3af

Please sign in to comment.