fix: more dedupe edge cases

PR-URL: https://github.com/npm/cli/pull/1545
Credit: @ruyadorno
Close: #1545
Reviewed-by: @isaacs

3 files changed, 150 insertions, 12 deletions

diff --git a/lib/ls.js b/lib/ls.js
index d8f0554ad..2c6171b3c 100644
--- a/lib/ls.js
+++ b/lib/ls.js
@@ -115,27 +115,27 @@ const getHumanOutputItem = (node, { args, color, global, long }) => {
   const missingColor = isOptional(node)
     ? chalk.yellow.bgBlack
     : chalk.red.bgBlack
-  const missingMsg = `UNMET ${isOptional(node) ? 'OPTIONAL ' : ''}DEPENDENCY `
+  const missingMsg = `UNMET ${isOptional(node) ? 'OPTIONAL ' : ''}DEPENDENCY`
   const label =
     (
       node[_missing]
-        ? (color ? missingColor(missingMsg) : missingMsg)
+        ? (color ? missingColor(missingMsg) : missingMsg) + ' '
         : ''
     ) +
     `${highlightDepName ? chalk.yellow.bgBlack(printable) : printable}` +
     (
       node[_dedupe]
-        ? (color ? chalk.gray(' deduped') : ' deduped')
+        ? ' ' + (color ? chalk.gray('deduped') : 'deduped')
         : ''
     ) +
     (
       node[_invalid]
-        ? (color ? chalk.red.bgBlack(' invalid') : ' invalid')
+        ? ' ' + (color ? chalk.red.bgBlack('invalid') : 'invalid')
         : ''
     ) +
     (
       isExtraneous(node, { global })
-        ? (color ? chalk.green.bgBlack(' extraneous') : ' extraneous')
+        ? ' ' + (color ? chalk.green.bgBlack('extraneous') : 'extraneous')
         : ''
     ) +
     (isGitNode(node) ? ` (${node.resolved})` : '') +
@@ -263,6 +263,9 @@ const augmentNodesWithMetadata = ({
       pkgid: node.pkgid,
       package: node.package,
       path: node.path,
+      isLink: node.isLink,
+      realpath: node.realpath,
+      [_invalid]: node[_invalid],
       [_missing]: node[_missing],
       [_dedupe]: true
     }
diff --git a/tap-snapshots/test-lib-ls.js-TAP.test.js b/tap-snapshots/test-lib-ls.js-TAP.test.js
index 274c6ef97..1363eec70 100644
--- a/tap-snapshots/test-lib-ls.js-TAP.test.js
+++ b/tap-snapshots/test-lib-ls.js-TAP.test.js
@@ -321,10 +321,10 @@ npm-broken-resolved-field-test@1.0.0 {CWD}/ls-ls-broken-resolved-field
 
 exports[`test/lib/ls.js TAP ls coloured output > should output tree containing color info 1`] = `
 test-npm-ls@1.0.0 {CWD}/ls-ls-coloured-output
-+-- foo@1.0.0 invalid
++-- foo@1.0.0 invalid
 | \`-- bar@1.0.0
-+-- UNMET DEPENDENCY ipsum@^1.0.0
-\`-- lorem@1.0.0 extraneous
++-- UNMET DEPENDENCY ipsum@^1.0.0
+\`-- lorem@1.0.0 extraneous
 
 `
 
@@ -336,6 +336,14 @@ test-npm-ls@1.0.0 {CWD}/ls-ls-cycle-deps
 
 `
 
+exports[`test/lib/ls.js TAP ls cycle deps with filter args > should print tree output containing deduped ref 1`] = `
+test-npm-ls@1.0.0 {CWD}/ls-ls-cycle-deps-with-filter-args
+\`-- a@1.0.0
+  \`-- b@1.0.0
+    \`-- a@1.0.0 deduped
+
+`
+
 exports[`test/lib/ls.js TAP ls deduped missing dep > should output parseable signaling missing peer dep in problems 1`] = `
 test-npm-ls@1.0.0 {CWD}/ls-ls-deduped-missing-dep
 +-- a@1.0.0
@@ -379,6 +387,14 @@ 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}/ls-ls-invalid-deduped-dep
++-- a@1.0.0
+| \`-- b@1.0.0 deduped invalid
+\`-- b@1.0.0 invalid
+
+`
+
 exports[`test/lib/ls.js TAP ls invalid peer dep > should output tree signaling mismatching peer dep in problems 1`] = `
 test-npm-ls@1.0.0 {CWD}/ls-ls-invalid-peer-dep
 +-- dev-dep@1.0.0
@@ -430,6 +446,14 @@ test-npm-ls@1.0.0 {CWD}/ls-ls-no-args
 
 `
 
+exports[`test/lib/ls.js TAP ls print deduped symlinks > should output tree containing linked deps 1`] = `
+print-deduped-symlinks@1.0.0 {CWD}/ls-ls-print-deduped-symlinks
++-- a@1.0.0
+| \`-- b@1.0.0 deduped -> {CWD}/ls-ls-print-deduped-symlinks/b
+\`-- b@1.0.0 -> {CWD}/ls-ls-print-deduped-symlinks/b
+
+`
+
 exports[`test/lib/ls.js TAP ls resolved points to git ref > should output tree containing git refs 1`] = `
 test-npm-ls@1.0.0 {CWD}/ls-ls-resolved-points-to-git-ref
 \`-- abbrev@1.1.1 (git+ssh://git@github.com/isaacs/abbrev-js.git#b8f3a2fc0c3bb8ffd8b0d0072cc6b5a3667e963c)
@@ -442,8 +466,8 @@ exports[`test/lib/ls.js TAP ls unmet optional dep > should output tree with empt
 | \`-- foo@1.0.0
 |   \`-- bar@1.0.0
 +-- lorem@1.0.0
-+-- UNMET OPTIONAL DEPENDENCY missing-optional-dep@^1.0.0
-+-- optional-dep@1.0.0 invalid
++-- UNMET OPTIONAL DEPENDENCY missing-optional-dep@^1.0.0
++-- optional-dep@1.0.0 invalid
 +-- peer-dep@1.0.0
 \`-- prod-dep@1.0.0
   \`-- bar@2.0.0
@@ -465,10 +489,10 @@ test-npm-ls@1.0.0 {CWD}/ls-ls-using-aliases
 exports[`test/lib/ls.js TAP ls with args and dedupe entries > should print tree output containing deduped ref 1`] = `
 dedupe-entries@1.0.0 {CWD}/ls-ls-with-args-and-dedupe-entries
 +-- @npmcli/a@1.0.0
-| \`-- @npmcli/b@1.1.2 deduped
+| \`-- @npmcli/b@1.1.2 deduped
 +-- @npmcli/b@1.1.2
 \`-- @npmcli/c@1.0.0
-  \`-- @npmcli/b@1.1.2 deduped
+  \`-- @npmcli/b@1.1.2 deduped
 
 `
 
diff --git a/test/lib/ls.js b/test/lib/ls.js
index 8446fba79..83a87513b 100644
--- a/test/lib/ls.js
+++ b/test/lib/ls.js
@@ -552,6 +552,42 @@ test('ls', (t) => {
     })
   })
 
+  t.test('print deduped symlinks', (t) => {
+    prefix = t.testdir({
+      'package.json': JSON.stringify({
+        name: 'print-deduped-symlinks',
+        version: '1.0.0',
+        dependencies: {
+          'a': '^1.0.0',
+          'b': '^1.0.0'
+        }
+      }),
+      'b': {
+        'package.json': JSON.stringify({
+          name: 'b',
+          version: '1.0.0'
+        })
+      },
+      node_modules: {
+        a: {
+          'package.json': JSON.stringify({
+            name: 'a',
+            version: '1.0.0',
+            dependencies: {
+              b: '^1.0.0'
+            }
+          })
+        },
+        'b': t.fixture('symlink', '../b')
+      }
+    })
+    ls([], () => {
+      t.matchSnapshot(redactCwd(result), 'should output tree containing linked deps')
+      _flatOptions.link = false
+      t.end()
+    })
+  })
+
   t.test('--production', (t) => {
     _flatOptions.production = true
     prefix = t.testdir({
@@ -719,6 +755,42 @@ test('ls', (t) => {
     })
   })
 
+  t.test('invalid deduped dep', (t) => {
+    _flatOptions.color = true
+    prefix = t.testdir({
+      'package.json': JSON.stringify({
+        name: 'invalid-deduped-dep',
+        version: '1.0.0',
+        dependencies: {
+          'a': '^1.0.0',
+          'b': '^2.0.0'
+        }
+      }),
+      node_modules: {
+        a: {
+          'package.json': JSON.stringify({
+            name: 'a',
+            version: '1.0.0',
+            dependencies: {
+              b: '^2.0.0'
+            }
+          })
+        },
+        b: {
+          'package.json': JSON.stringify({
+            name: 'b',
+            version: '1.0.0'
+          })
+        }
+      }
+    })
+    ls([], () => {
+      t.matchSnapshot(redactCwd(result), 'should output tree signaling mismatching peer dep in problems')
+      _flatOptions.color = false
+      t.end()
+    })
+  })
+
   t.test('deduped missing dep', (t) => {
     prefix = t.testdir({
       'package.json': JSON.stringify({
@@ -836,6 +908,45 @@ test('ls', (t) => {
     })
   })
 
+  t.test('cycle deps with filter args', (t) => {
+    _flatOptions.color = true
+    prefix = t.testdir({
+      'package.json': JSON.stringify({
+        name: 'test-npm-ls',
+        version: '1.0.0',
+        dependencies: {
+          'a': '^1.0.0'
+        }
+      }),
+      node_modules: {
+        'a': {
+          'package.json': JSON.stringify({
+            name: 'a',
+            version: '1.0.0',
+            dependencies: {
+              b: '^1.0.0'
+            }
+          })
+        },
+        'b': {
+          'package.json': JSON.stringify({
+            name: 'b',
+            version: '1.0.0',
+            dependencies: {
+              a: '^1.0.0'
+            }
+          })
+        }
+      }
+    })
+    ls(['a'], (err) => {
+      t.ifError(err, 'npm ls')
+      t.matchSnapshot(redactCwd(result), 'should print tree output containing deduped ref')
+      _flatOptions.color = false
+      t.end()
+    })
+  })
+
   t.test('with no args dedupe entries', (t) => {
     prefix = t.testdir({
       'package.json': JSON.stringify({