Skip to content

npm ls arborist rewrite #1545

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 9 commits into from
Closed

Conversation

ruyadorno
Copy link
Contributor

  • Rewrites lib/ls.js command to use @npmcli/arborist
  • Updates unit tests

Breaking changes:

  • npm ls now defaults to only printing root children
  • added --all flag that is same as --depth=Infinity
  • --depth now defaults to 0 and can still be used if --all is not truthy
  • added error codes: ELSPROBLEMS, EJSONPARSE to callback errors
  • extraneous deps depth will match current location in nm folder
  • mark top-level deps as extraneous when missing root package.json
  • don't mark deps as extraneous if they're valid deps of invalid deps
  • peer deps are now listed as regular deps, removed oddities such as
    peerinvalid label and stops labeling peer deps extraneous
  • might print diff git resolved values, see: https://github.com/npm/hosted-git-info
  • Parseable (--parseable) output:
    • possible order of printed elements changed
    • fixed consistency problems in which it would print root folder
      name if using a filter argument that could not match against
      any of the deps in the current installed tree
    • fixed printing non-existing paths for missing dependencies
    • fixed undefined symlink output when using --long output
  • JSON (--json) output:
    • removed: from property from --json output
    • removed: "[Circular]" references
    • added "missing" to list of peer-dep problems listed
    • added peerDependencies ref when using --long output
    • removed readme properties using --long output
    • Renamed error msg: Failed to parse json -> Failed to parse root package.json

- Rewrites lib/ls.js command to use @npmcli/arborist
- Updates unit tests
- Breaking changes:
  - added error codes: ELSPROBLEMS, EJSONPARSE to callback errors
  - extraneous deps depth will match current location in nm folder
  - mark top-level deps as extraneous when missing root package.json
  - don't mark deps as extraneous if they're valid deps of invalid deps
  - peer deps are now listed as regular deps, removed oddities such as
  peerinvalid label and stops labeling peer deps extraneous
  - might print diff git resolved values,
  see: https://github.com/npm/hosted-git-info
  - Parseable (--parseable) output:
    - possible order of printed elements changed
    - fixed consistency problems in which it would print root folder
    name if using a filter argument that could not match against
    any of the deps in the current installed tree
    - fixed printing non-existing paths for missing dependencies
    - fixed undefined symlink output when using --long output
  - JSON (--json) output:
    - removed: `from` property from --json output
    - removed: "[Circular]" references
    - added "missing" to list of peer-dep problems listed
    - added peerDependencies ref when using --long output
    - removed readme properties using --long output
    - Renamed error msg:
    `Failed to parse json` -> `Failed to parse root package.json`

refs:
  - npm/statusboard#99
  - npm/statusboard#103
- npm ls now defaults to only printing root children
- added --all flag that is same as --depth=Infinity
- --depth now defaults to 0 and can still be used if --all is not truthy
@ruyadorno ruyadorno requested a review from a team as a code owner July 21, 2020 04:20
@ruyadorno ruyadorno added this to the OSS - Sprint 11 milestone Jul 21, 2020
@ruyadorno ruyadorno self-assigned this Jul 21, 2020
@ruyadorno ruyadorno added Release 7.x work is associated with a specific npm 7 release semver:major backwards-incompatible breaking changes labels Jul 21, 2020
@isaacs
Copy link
Contributor

isaacs commented Jul 21, 2020

It looks like this patch should be landed for depth to actually default to 0:

diff --git a/docs/content/using-npm/config.md b/docs/content/using-npm/config.md
index e444d5fb8..b1df6cb41 100644
--- a/docs/content/using-npm/config.md
+++ b/docs/content/using-npm/config.md
@@ -148,6 +148,15 @@ you want your scoped package to be publicly viewable (and installable) set
 `--access=public`. The only valid values for `access` are `public` and
 `restricted`. Unscoped packages _always_ have an access level of `public`.
 
+#### all
+
+* Default: `false`
+* Type: Boolean
+
+When running `npm outdated` and `npm ls`, setting `--all` will show all
+outdated or installed packages, rather than only those directly depended
+upon by the current project.
+
 #### allow-same-version
 
 * Default: false
@@ -350,16 +359,12 @@ disabled when the environment variable `NO_COLOR` is set to any value.
 
 #### depth
 
-* Default: Infinity
+* Default: 0
 * Type: Number
 
-The depth to go when recursing directories for `npm ls`,
-`npm cache ls`, and `npm outdated`.
+The depth to go when recursing packages for `npm ls`.
 
-For `npm outdated`, a setting of `Infinity` will be treated as `0`
-since that gives more useful information.  To show the outdated status
-of all packages and dependents, use a large integer value,
-e.g., `npm outdated --depth 9999`
+To make this default to `Infinity` instead of `0`, set `--all`.
 
 #### description
 
diff --git a/lib/config/defaults.js b/lib/config/defaults.js
index b47adf673..4a78144a2 100644
--- a/lib/config/defaults.js
+++ b/lib/config/defaults.js
@@ -131,7 +131,7 @@ Object.defineProperty(exports, 'defaults', {get: function () {
     cidr: null,
 
     color: process.env.NO_COLOR == null,
-    depth: Infinity,
+    depth: 0,
     description: true,
     dev: false,
     'dry-run': false,

@ruyadorno
Copy link
Contributor Author

@isaacs applied the patch + paired with @darcyclarke on adding support to global 😅 oops

@isaacs
Copy link
Contributor

isaacs commented Jul 21, 2020

Hmm, I'm not seeing the deduped packages?

$ node . ls pacote
[email protected] /Users/isaacs/dev/npm/cli
└── [email protected]


$ npm ls pacote
[email protected] /Users/isaacs/dev/npm/cli
├─┬ @npmcli/[email protected]
│ └── [email protected]  deduped
├─┬ [email protected]
│ └── [email protected]  deduped
└── [email protected]

I thought it might be getting cut off by the default --depth=0, but this is super weird, too:

$ node . ls pacote --all
[email protected] /Users/isaacs/dev/npm/cli
├── (empty)
└─┬ [email protected]
  └── [email protected]

@isaacs
Copy link
Contributor

isaacs commented Jul 21, 2020

After running node . install (wondering if it might've been an issue with outdated metadata from npm v6), I got this interesting issue:

$ node . ls pacote
[email protected] /Users/isaacs/dev/npm/cli extraneous
└── [email protected]

It's reading the root package as extraneous for some reason, that seems like a bug.

@ruyadorno
Copy link
Contributor Author

@isaacs thanks! It's def a legit problem since I can easily reproduce it too 😞 I'll add some more test cases to cover those! I think specially dedupe was missing a more robust test case

@ruyadorno
Copy link
Contributor Author

also, I think we should default to all: true when using args!

- When using args to filter out items deduped pkgs were broken
- Fixed highlight color when using filter args
- Changed default behavior to show all packages when using args filter

Paired-with: @isaacs
- Only assign problems if the related node is included in output
- Avoid breaking if using dummy node on node.satisfies
- Added test case for `npm ls .` edge case
@ruyadorno
Copy link
Contributor Author

ruyadorno commented Jul 23, 2020

Going to fix a couple more things and it should be good to go 😊

isaacs pushed a commit that referenced this pull request Jul 23, 2020
PR-URL: #1545
Credit: @ruyadorno
Close: #1545
Reviewed-by: @isaacs
@isaacs
Copy link
Contributor

isaacs commented Jul 23, 2020

Landed in release/v7.0.0-beta. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release 7.x work is associated with a specific npm 7 release semver:major backwards-incompatible breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants