Skip to content

fix(ls): correctly show deduped dependencies #8217

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

Open
wants to merge 3 commits into
base: latest
Choose a base branch
from

Conversation

milaninfy
Copy link
Contributor

@milaninfy milaninfy commented Apr 9, 2025

Attempt to fix npm ls output

Problem:

  • deduped nodes are cloned when encountered using previously seen nodes set but they are not explored further when we are requesting npm ls dep-name if dep-name is a child of some deduped parent then whole tree would be shown till that parent but logically dep-name is still the child/grand-child of that.
  • When deduped nodes are explored further it results in cycle when dep1 is ancestor of dep2 which again depends on dep1 creating a cycle when explored, also the algorithm used is BFS so it's hard to detect cycle.

Solution:

  • Use DFS like approach with custom cycle detection checks.
    • keep track of how any node is discovered. so each node or parent node will have a list containing all the relevant visited nodes, we check if current node is in that list then it's a cycle and we don't need to explore this node.
  • This solution attempts to explore cloned nodes of deduped nodes and use cached result so that it fixes the output and perform better. since those nodes are deduped and already explored we can use the result.

Progress:

  • still some cases which might result in Infinite loop and need to discovered those.
  • very slow and uses very large memory during run when ran on big repo, need to find out optimizations or better approach
  • Fix/add the test cases
  • Refactor

Test output

`npm ls child` - before and after
~/workarea/rep/repro $ npm ls child
repro@1.0.0 /Users/milaninfy/workarea/rep/repro
└─┬ relative-p2@1.0.0
  └─┬ grand-p1@1.0.0
    └─┬ parent-p1@1.0.0
      └── child@1.0.0

~/workarea/rep/repro $ lnpm ls child
repro@1.0.0 /Users/milaninfy/workarea/rep/repro
├─┬ relative-p1@1.0.0
│ └─┬ family@1.0.0
│   └─┬ grand-p1@1.0.0
│     └─┬ parent-p1@1.0.0
│       └── child@1.0.0
└─┬ relative-p2@1.0.0
  └─┬ grand-p1@1.0.0 deduped
    └─┬ parent-p1@1.0.0 deduped
      └── child@1.0.0 deduped

fixes: #7917

@milaninfy milaninfy force-pushed the mm/fix-ls branch 3 times, most recently from fb7ba8e to 1ff339c Compare May 8, 2025 16:45
@milaninfy milaninfy marked this pull request as ready for review June 4, 2025 17:17
@milaninfy milaninfy requested a review from a team as a code owner June 4, 2025 17:17
@milaninfy milaninfy marked this pull request as draft June 11, 2025 18:58
@milaninfy milaninfy marked this pull request as ready for review June 11, 2025 19:51
// the `tree` obj) that was just visited in the `visit` method below
// `nodeResult` is going to be the returned `item` from `visit`
const result = exploreDependencyGraph({
node: tree,
getChildren (node, nodeResult) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing in function like this and visit can be pretty confusing when trying to understand the code later. It's also easy to introduce unintended mistakes cause state can get pretty noodly.

Let's call these beforehand and pass the results as new params.

@@ -58,6 +58,7 @@ class LS extends ArboristWorkspaceCmd {
const unicode = this.npm.config.get('unicode')
const packageLockOnly = this.npm.config.get('package-lock-only')
const workspacesEnabled = this.npm.flatOptions.workspacesEnabled
const includeWorkspaceRoot = this.npm.flatOptions.includeWorkspaceRoot
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason workspacesEnabled is pulling from flatoptions is because it's derived from workspaces config. includeWorkspaceRoot has no such distinction so we should pull it from config like the rest of this command does.

We know this is less than ideal, but fixing this disparity is a future concern.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] npm ls only listing dev dependencies by default
2 participants