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
[email protected] {project}
+-- unmet optional dependency optional-missing@1
+-- [email protected]
+-- [email protected] invalid
+-- [email protected] invalid: "1" from the root project
+-- unmet dependency peer-missing@1
+-- [email protected]
+-- unmet optional dependency peer-optional-missing@1
+-- [email protected]
+-- [email protected] invalid
+-- [email protected] invalid
+-- [email protected] invalid: "1" from the root project
+-- [email protected] invalid: "1" from the root project
+-- unmet dependency prod-missing@1
+-- [email protected]
\`-- [email protected] invalid
\`-- [email protected] invalid: "1" from the root project
`

Expand Down Expand Up @@ -356,7 +356,7 @@ [email protected] {CWD}/tap-testdir-ls-ls-broken-resolved-fie
exports[`test/lib/ls.js TAP ls colored output > should output tree containing color info 1`] = `
[[email protected] {CWD}/tap-testdir-ls-ls-colored-output
+-- [email protected] extraneous
[0m+-- [email protected] [31m[40minvalid[49m[39m[0m
[0m+-- [email protected] [31m[40minvalid: "^2.0.0" from the root project[49m[39m[0m
| \`-- [email protected]
\`-- 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`] = `
[[email protected] {CWD}/tap-testdir-ls-ls-invalid-deduped-dep
+-- [email protected]
[0m| \`-- [email protected] [90mdeduped[39m [31m[40minvalid[49m[39m[0m
[0m\`-- [email protected] [31m[40minvalid[49m[39m[0m
[0m| \`-- [email protected] [90mdeduped[39m [31m[40minvalid: "^2.0.0" from the root project, "^2.0.0" from node_modules/a[49m[39m[0m
[0m\`-- [email protected] [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 @@ [email protected] {CWD}/tap-testdir-ls-ls-invalid-peer-dep
| \`-- [email protected]
| \`-- [email protected]
+-- [email protected]
+-- [email protected] invalid
+-- [email protected] invalid: "^2.0.0" from the root project
\`-- [email protected]
\`-- [email protected]
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`] = `
[email protected] {CWD}/tap-testdir-ls-ls-missing-invalid-extraneous
+-- [email protected] extraneous
+-- [email protected] invalid
+-- [email protected] invalid: "^2.0.0" from the root project
| \`-- [email protected]
\`-- 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
| \`-- [email protected]
| \`-- [email protected]
+-- UNMET OPTIONAL DEPENDENCY missing-optional-dep@^1.0.0
[0m+-- [email protected] [31m[40minvalid[49m[39m[0m
[0m+-- [email protected] [31m[40minvalid: "^2.0.0" from the root project[49m[39m[0m
+-- [email protected]
\`-- [email protected]
 \`-- [email protected]
Expand Down Expand Up @@ -691,3 +691,14 @@ [email protected] {CWD}/tap-testdir-ls-ls-with-no-args-dedupe-entries-and-not
\`-- @npmcli/[email protected]
`

exports[`test/lib/ls.js TAP show multiple invalid reasons > ls result 1`] = `
[email protected] {cwd}/tap-testdir-ls-show-multiple-invalid-reasons
+-- [email protected] invalid: "^2.0.0" from the root project
| \`-- [email protected] deduped invalid: "^1.2.3" from the root project, "^2.0.0" from node_modules/cat
+-- [email protected] extraneous
| \`-- [email protected] deduped invalid: "^1.2.3" from the root project, "^2.0.0" from node_modules/cat, "2.x" from node_modules/chai
\`-- [email protected] invalid: "^1.2.3" from the root project, "^2.0.0" from node_modules/cat, "2.x" from node_modules/chai
\`-- [email protected] 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: [email protected] {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: [email protected] {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: [email protected] {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: [email protected] {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.