From 83590d40f94347f21714dbd158b9ddcad9c82de9 Mon Sep 17 00:00:00 2001 From: isaacs Date: Thu, 20 May 2021 10:53:10 -0700 Subject: [PATCH] fix(ls): show relative paths from root 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: https://github.com/npm/cli/pull/3272 Credit: @isaacs Close: #3272 Reviewed-by: @darcyclarke --- lib/ls.js | 12 +++++-- tap-snapshots/test/lib/ls.js.test.cjs | 46 +++++++++++++-------------- test/lib/ls.js | 7 +++- 3 files changed, 39 insertions(+), 26 deletions(-) diff --git a/lib/ls.js b/lib/ls.js index 4e504912a0175..d92b73ddfcdbb 100644 --- a/lib/ls.js +++ b/lib/ls.js @@ -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') @@ -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] @@ -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: [] }) @@ -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, @@ -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], diff --git a/tap-snapshots/test/lib/ls.js.test.cjs b/tap-snapshots/test/lib/ls.js.test.cjs index f443d9caba895..2ed0b4b001376 100644 --- a/tap-snapshots/test/lib/ls.js.test.cjs +++ b/tap-snapshots/test/lib/ls.js.test.cjs @@ -70,7 +70,7 @@ test-npm-ls@1.0.0 {CWD}/tap-testdir-ls-ls---dev exports[`test/lib/ls.js TAP ls --link > should output tree containing linked deps 1`] = ` test-npm-ls@1.0.0 {CWD}/tap-testdir-ls-ls---link -\`-- linked-dep@1.0.0 -> {CWD}/tap-testdir-ls-ls---link/linked-dep +\`-- linked-dep@1.0.0 -> ./linked-dep ` @@ -480,24 +480,24 @@ 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`] = ` workspaces-tree@1.0.0 {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces -+-- e@1.0.0 -> {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces/group/e -\`-- f@1.0.0 -> {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces/group/f ++-- e@1.0.0 -> ./group/e +\`-- f@1.0.0 -> ./group/f ` exports[`test/lib/ls.js TAP ls loading a tree containing workspaces > should filter single workspace 1`] = ` workspaces-tree@1.0.0 {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces -+-- a@1.0.0 -> {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces/a -| \`-- d@1.0.0 deduped -> {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces/d -\`-- d@1.0.0 -> {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces/d ++-- a@1.0.0 -> ./a +| \`-- d@1.0.0 deduped -> ./d +\`-- d@1.0.0 -> ./d ` exports[`test/lib/ls.js TAP ls loading a tree containing workspaces > should filter using workspace config 1`] = ` workspaces-tree@1.0.0 {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces -\`-- a@1.0.0 -> {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces/a +\`-- a@1.0.0 -> ./a +-- c@1.0.0 - \`-- d@1.0.0 -> {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces/d + \`-- d@1.0.0 -> ./d \`-- foo@1.1.1 \`-- bar@1.0.0 @@ -505,34 +505,34 @@ workspaces-tree@1.0.0 {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspac exports[`test/lib/ls.js TAP ls loading a tree containing workspaces > should list --all workspaces properly 1`] = ` workspaces-tree@1.0.0 {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces -+-- a@1.0.0 -> {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces/a ++-- a@1.0.0 -> ./a | +-- c@1.0.0 -| \`-- d@1.0.0 deduped -> {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces/d -+-- b@1.0.0 -> {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces/b -+-- d@1.0.0 -> {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces/d +| \`-- d@1.0.0 deduped -> ./d ++-- b@1.0.0 -> ./b ++-- d@1.0.0 -> ./d | \`-- foo@1.1.1 | \`-- bar@1.0.0 -+-- e@1.0.0 -> {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces/group/e -\`-- f@1.0.0 -> {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces/group/f ++-- e@1.0.0 -> ./group/e +\`-- f@1.0.0 -> ./group/f ` exports[`test/lib/ls.js TAP ls loading a tree containing workspaces > should list workspaces properly with default configs 1`] = ` workspaces-tree@1.0.0 {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces -+-- a@1.0.0 -> {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces/a ++-- a@1.0.0 -> ./a | +-- c@1.0.0 -| \`-- d@1.0.0 deduped -> {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces/d -+-- b@1.0.0 -> {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces/b -+-- d@1.0.0 -> {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces/d +| \`-- d@1.0.0 deduped -> ./d ++-- b@1.0.0 -> ./b ++-- d@1.0.0 -> ./d | \`-- foo@1.1.1 -+-- e@1.0.0 -> {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces/group/e -\`-- f@1.0.0 -> {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces/group/f ++-- e@1.0.0 -> ./group/e +\`-- f@1.0.0 -> ./group/f  ` 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`] = ` workspaces-tree@1.0.0 {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces -\`-- d@1.0.0 -> {CWD}/tap-testdir-ls-ls-loading-a-tree-containing-workspaces/d +\`-- d@1.0.0 -> ./d \`-- foo@1.1.1 \`-- bar@1.0.0 @@ -567,8 +567,8 @@ test-npm-ls@1.0.0 {CWD}/tap-testdir-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}/tap-testdir-ls-ls-print-deduped-symlinks +-- a@1.0.0 -| \`-- b@1.0.0 deduped -> {CWD}/tap-testdir-ls-ls-print-deduped-symlinks/b -\`-- b@1.0.0 -> {CWD}/tap-testdir-ls-ls-print-deduped-symlinks/b +| \`-- b@1.0.0 deduped -> ./b +\`-- b@1.0.0 -> ./b ` diff --git a/test/lib/ls.js b/test/lib/ls.js index 2cde319463a9e..ecdede809df20 100644 --- a/test/lib/ls.js +++ b/test/lib/ls.js @@ -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,