Skip to content

Conversation

liamcmitchell
Copy link
Contributor

@liamcmitchell liamcmitchell commented Aug 28, 2025

Discovered while investigating #8535 (comment).
I noticed that not all deps of a failed optional dep were added to the trash list.

I modified the tests to expose this and changed the implementation to fix it.

Best illustrated with this graph showing the results of the test case const setO = optionalSet(nodeO):

image

Yellow nodes were returned by the prev implementation. Yellow + green nodes are returned by the fixed implementation.

Code to produce graph
const oldS = gatherDepSet(set, edge => !edge.optional)
const newS = gatherDepSet(set, edge => !set.has(edge.to))
if (!oldS.size !== newS.size) {
  graph(oldS, newS)
}
function graph (...sets) {
  const safe = (id) => id.replaceAll('@', '_')
  let code = 'flowchart\n'
  for (const node of sets[0].values().next().value.root.inventory.values()) {
    const rgb = sets.map(s => s.has(node) ? 'c' : '6').concat(...'666').slice(0, 3).join('')
    code += `style ${safe(node.pkgid)} fill:#${rgb},color:#000\n`
    for (const edge of node.edgesOut.values()) {
      code += `${safe(node.pkgid)}-${edge.optional ? '.' : ''}->|${edge.type} ${edge.error || ''}|${safe(edge.to?.pkgid || `${edge.name}@${edge.spec}`)}\n`
    }
  }
  const mermaid = JSON.stringify({ theme: 'neutral' })
  console.log(`https://mermaid.live/edit#pako:${require('zlib').deflateSync(JSON.stringify({ code, mermaid })).toString('base64')}`)
}

@liamcmitchell
Copy link
Contributor Author

Test failures were from missing snapshots that were deleted because of skipped tests. I restored those snapshots and rebased.

@liamcmitchell liamcmitchell changed the title fix: only filter inbound dep set edges fix: optional set calculation Aug 31, 2025
@liamcmitchell
Copy link
Contributor Author

liamcmitchell commented Aug 31, 2025

I moved my fix from inside gatherDepSet to the filter function passed into it.

I will spend some more time looking at gatherDepSet because I think it could be improved but at least this can be of use on it's own.

Edit: no longer looking at gatherDepSet

owlstronaut pushed a commit that referenced this pull request Sep 3, 2025
While investigating CI failure in #8537,
I found unrelated test failures when running locally on windows.

`prune with lockfile with implicit optional peer dependencies` fixed by
adding files to fixture `node_modules`, avoiding network, also reduced
package lock to minimum and removed unhelpful snapshots

`move aside symlink clutter` fixed by creating problematic symlink in
`t.testdir()` instead of in `reifyPackages` hook

---------

Co-authored-by: Liam Mitchell <[email protected]>
@wraithgar wraithgar self-assigned this Sep 29, 2025
@wraithgar
Copy link
Member

Things have moved a bit in arborist since this PR was made. I rebased it against latest just to be sure nothing surprising surfaced. Tests still pass w/ 100% coverage.

@wraithgar wraithgar merged commit 13d8df6 into npm:latest Sep 29, 2025
18 checks passed
@github-actions github-actions bot mentioned this pull request Sep 29, 2025
wraithgar pushed a commit that referenced this pull request Sep 30, 2025
Discovered while investigating
#8535 (comment)

Similar to #8566, relates to
#8184

Moves `inert` (uninstallable optional) calculation into `buildIdealTree`
so it can be used in diff.

Also removes most of #8184 in favor of a
[simpler
fix](https://github.com/npm/cli/pull/8602/files#diff-02626074e1a4a170693607e4a3a69dfc08ee52067734717833b22cf162923e07R354),
see PR comments for the journey.

Improvements:

* we don't see uninstallable packages as "installed" in CLI output
* `createSparseTree` no longer creates dirs that will only be cleaned
later

For the example in the linked issue, it changes output from `added 156
packages` to `added 12 packages` and combined with
#8537 it changes to `added 6 packages`,
the expected result.
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.

2 participants