Skip to content

Commit

Permalink
fix(ls): show relative paths from root
Browse files Browse the repository at this point in the history
Change the output in `npm ls` for symlink dependencies (including
workspaces) to show the relative path from the project root, rather than
the absolute path of the link target.

This makes the output much less noisy and more ergonomic when many
workspaces and link dependencies are in use, especially when paths are
long.

It is arguable that this output might be slightly misleading, since the
_actual_ workspace symlink from `${root}/node_modules/b` to
`${root}/packages/b` has a link value of `../packages/b`, not just
`packages/b`.  (Or on Windows, always the full absolute path, because
junctions.)  Thus, `npm ls b` will not show the same output as `ls -l
node_modules/b`.

However, `npm ls` shows the logical tree, not the physical tree, so
presenting the user with a path that they can use and interpret is more
important than presenting them with the strictly accurate details of
their filesystem.

PR-URL: #3272
Credit: @isaacs
Close: #3272
Reviewed-by: @darcyclarke
  • Loading branch information
isaacs authored and wraithgar committed May 24, 2021
1 parent 46a9bcb commit 83590d4
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 26 deletions.
12 changes: 10 additions & 2 deletions lib/ls.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
const { resolve } = require('path')
const { resolve, relative, sep } = require('path')
const relativePrefix = `.${sep}`
const { EOL } = require('os')

const archy = require('archy')
Expand Down Expand Up @@ -298,6 +299,9 @@ const getHumanOutputItem = (node, { args, color, global, long }) => {
? chalk.yellow.bgBlack
: chalk.red.bgBlack
const missingMsg = `UNMET ${isOptional(node) ? 'OPTIONAL ' : ''}DEPENDENCY`
const targetLocation = node.root
? relative(node.root.realpath, node.realpath)
: node.targetLocation
const label =
(
node[_missing]
Expand All @@ -321,7 +325,7 @@ const getHumanOutputItem = (node, { args, color, global, long }) => {
: ''
) +
(isGitNode(node) ? ` (${node.resolved})` : '') +
(node.isLink ? ` -> ${node.realpath}` : '') +
(node.isLink ? ` -> ${relativePrefix}${targetLocation}` : '') +
(long ? `${EOL}${node.package.description || ''}` : '')

return augmentItemWithIncludeMetadata(node, { label, nodes: [] })
Expand Down Expand Up @@ -445,6 +449,9 @@ const augmentNodesWithMetadata = ({
// revisit that node in tree traversal logic, so we make it so that
// we have a diff obj for deduped nodes:
if (seenNodes.has(node.path)) {
const { realpath, root } = node
const targetLocation = root ? relative(root.realpath, realpath)
: node.targetLocation
node = {
name: node.name,
version: node.version,
Expand All @@ -453,6 +460,7 @@ const augmentNodesWithMetadata = ({
path: node.path,
isLink: node.isLink,
realpath: node.realpath,
targetLocation,
[_type]: node[_type],
[_invalid]: node[_invalid],
[_missing]: node[_missing],
Expand Down
46 changes: 23 additions & 23 deletions tap-snapshots/test/lib/ls.js.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ [email protected] {CWD}/tap-testdir-ls-ls---dev

exports[`test/lib/ls.js TAP ls --link > should output tree containing linked deps 1`] = `
[email protected] {CWD}/tap-testdir-ls-ls---link
\`-- [email protected] -> {CWD}/tap-testdir-ls-ls---link/linked-dep
\`-- [email protected] -> ./linked-dep
`

Expand Down Expand Up @@ -480,59 +480,59 @@ exports[`test/lib/ls.js TAP ls json read problems > should print empty result 1`

exports[`test/lib/ls.js TAP ls loading a tree containing workspaces > should filter by parent folder workspace config 1`] = `
[email protected] {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces
+-- [email protected] -> {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces/group/e
\`-- [email protected] -> {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces/group/f
+-- [email protected] -> ./group/e
\`-- [email protected] -> ./group/f
`

exports[`test/lib/ls.js TAP ls loading a tree containing workspaces > should filter single workspace 1`] = `
[email protected] {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces
+-- [email protected] -> {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces/a
| \`-- [email protected] deduped -> {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces/d
\`-- [email protected] -> {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces/d
+-- [email protected] -> ./a
| \`-- [email protected] deduped -> ./d
\`-- [email protected] -> ./d
`

exports[`test/lib/ls.js TAP ls loading a tree containing workspaces > should filter using workspace config 1`] = `
[email protected] {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces
\`-- [email protected] -> {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces/a
\`-- [email protected] -> ./a
+-- [email protected]
\`-- [email protected] -> {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces/d
\`-- [email protected] -> ./d
\`-- [email protected]
\`-- [email protected]
`

exports[`test/lib/ls.js TAP ls loading a tree containing workspaces > should list --all workspaces properly 1`] = `
[email protected] {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces
+-- [email protected] -> {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces/a
+-- [email protected] -> ./a
| +-- [email protected]
| \`-- [email protected] deduped -> {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces/d
+-- [email protected] -> {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces/b
+-- [email protected] -> {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces/d
| \`-- [email protected] deduped -> ./d
+-- [email protected] -> ./b
+-- [email protected] -> ./d
| \`-- [email protected]
| \`-- [email protected]
+-- [email protected] -> {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces/group/e
\`-- [email protected] -> {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces/group/f
+-- [email protected] -> ./group/e
\`-- [email protected] -> ./group/f
`

exports[`test/lib/ls.js TAP ls loading a tree containing workspaces > should list workspaces properly with default configs 1`] = `
[[email protected] {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces
[0m+-- [[email protected][39m -> {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces/a[0m
[0m+-- [[email protected][39m -> ./a[0m
| +-- [email protected]
[0m| \`-- [email protected] [90mdeduped[39m -> {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces/d[0m
[0m+-- [[email protected][39m -> {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces/b[0m
[0m+-- [[email protected][39m -> {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces/d[0m
[0m| \`-- [email protected] [90mdeduped[39m -> ./d[0m
[0m+-- [[email protected][39m -> ./b[0m
[0m+-- [[email protected][39m -> ./d[0m
| \`-- [email protected]
[0m+-- [[email protected][39m -> {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces/group/e[0m
[0m\`-- [[email protected][39m -> {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces/group/f[0m
[0m+-- [[email protected][39m -> ./group/e[0m
[0m\`-- [[email protected][39m -> ./group/f[0m

`

exports[`test/lib/ls.js TAP ls loading a tree containing workspaces > should print all tree and filter by dep within only the ws subtree 1`] = `
[email protected] {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces
\`-- [email protected] -> {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces/d
\`-- [email protected] -> ./d
\`-- [email protected]
\`-- [email protected]
Expand Down Expand Up @@ -567,8 +567,8 @@ [email protected] {CWD}/tap-testdir-ls-ls-no-args
exports[`test/lib/ls.js TAP ls print deduped symlinks > should output tree containing linked deps 1`] = `
[email protected] {CWD}/tap-testdir-ls-ls-print-deduped-symlinks
+-- [email protected]
| \`-- [email protected] deduped -> {CWD}/tap-testdir-ls-ls-print-deduped-symlinks/b
\`-- [email protected] -> {CWD}/tap-testdir-ls-ls-print-deduped-symlinks/b
| \`-- [email protected] deduped -> ./b
\`-- [email protected] -> ./b
`

Expand Down
7 changes: 6 additions & 1 deletion test/lib/ls.js
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,12 @@ const diffDepTypesNmFixture = {
}

let result = ''
const LS = require('../../lib/ls.js')
const LS = t.mock('../../lib/ls.js', {
path: {
...require('path'),
sep: '/',
},
})
const config = {
all: true,
color: false,
Expand Down

0 comments on commit 83590d4

Please sign in to comment.