fix: prefer existing tree nodes for peerOptional deps (#9249) (#9283)

When a peerOptional edge conflicts, search descendants for a satisfying
node before fetching from the registry. This prevents extraneous
packages from blocking hoisting of required deps.

This fixes 1/2 or 1/3 of #9249. Before this change a clean install would
resolve `nm/jest-util@30` when resolving the conflict at nm/jest-util
between ts-jest's jest-util@^29||^30, and expect's ^28, which had been
placed at root. `#nodeFromEdge` would create a brand new node, matching
greatest ^30. A subequent install would mark nm/jest-util@30 as
extraneous and prune it.
This tree is valid, but ts-jest's peerOptional jest-util is unsatisfied,
while compatible jest-util are installed and duplicated.

This change reduces duplication and can prevent peerOptionals from
actively installing.

1. Now during initial installs npm will prefer hoisting a dependency to
de-dupe a peerOptional conflict over creating a new extraneous edge.
2. It doesn't solve the problem if there's no compatible version in the
sub-tree. npm will still use `#nodeFromEdge` and install an extraneous
edge.
3. It doesn't fix installs from lockfiles generated before this fix. I
think this is okay, because the trees are techincally valid, just not
optimal.

 I think a better solution to all three issues would be:
* During problemEdge conflict resolution, npm would hoist
nm/jest-util@28 under expect, without replacing it with anything.
ts-jest's peerOptional jest-util would be unsatisfied. This creates the
same tree as npm's second installs that prune extraneous.
* Check for any dependencies that can be hosited. This can run during
the initial install on problemEdge conflict resoultion, and in
pruneIdealTree on any nodes that are removed.
 
I think this solves all three issues. I didn't implement it because I
couldn't find a way to resolve the conflict by leaving a hole in the
tree..

Author: everett1992

workspaces/arborist/lib/arborist/build-ideal-tree.js

@@ -898,8 +898,21 @@ This is a one-time fix-up, please be patient...
       // be forced to agree on a version of z.
       const required = new Set([edge.from])
       const parent = edge.peer ? virtualRoot : null
-      const dep = vrDep && vrDep.satisfies(edge) ? vrDep
-        : await this.#nodeFromEdge(edge, parent, null, required)
+      let dep = vrDep && vrDep.satisfies(edge) ? vrDep : null
+
+      // A peerOptional conflict can be resolved by finding an existing node in the tree that satisfies the edge, avoiding a registry fetch that may introduce an extraneous package. See npm/cli#9249.
+      // Skip the shortcut when the user has signaled an explicit re-fetch intent (npm update by name, explicit request, or audit fix), so we honor those signals rather than silently keeping the existing node.
+      const skipExistingShortcut = this[_updateNames].includes(edge.name)
+        || this.#explicitRequests.has(edge)
+        || (edge.to && this.auditReport?.isVulnerable(edge.to))
+      if (!dep && edge.type === 'peerOptional' && !skipExistingShortcut) {
+        dep = this.#findHoistableNode(
+          /* istanbul ignore next - resolveParent is always set for non-root nodes */
+          edge.from.resolveParent || edge.from, edge)
+      }
+      if (!dep) {
+        dep = await this.#nodeFromEdge(edge, parent, null, required)
+      }
 
       /* istanbul ignore next */
       debug(() => {
@@ -1029,6 +1042,24 @@ This is a one-time fix-up, please be patient...
     return this.#buildDepStep()
   }
 
+  // BFS descendants of `root` for a node satisfying `edge`.
+  // Prefers nodes closer to root.  Skips bundled nodes.
+  #findHoistableNode (root, edge) {
+    const queue = [...root.children.values()]
+    while (queue.length) {
+      const node = queue.shift()
+      if (node.name === edge.name
+        && !node.inDepBundle
+        && node.satisfies(edge)) {
+        return node
+      }
+      for (const child of node.children.values()) {
+        queue.push(child)
+      }
+    }
+    return null
+  }
+
   // loads a node from an edge, and then loads its peer deps (and their peer deps, on down the line) into a virtual root parent.
   async #nodeFromEdge (edge, parent_, secondEdge, required) {
     // create a virtual root node with the same deps as the node that is requesting this one, so that we can get all the peer deps in a context where they're likely to be resolvable.

workspaces/arborist/test/arborist/build-ideal-tree.js

@@ -4482,6 +4482,140 @@ t.test('skip invalid peerOptional edges in problemEdges when save=false (#8726)'
   t.ok(tree.children.get('util'), 'util is in the tree')
 })
 
+t.test('peerOptional prefers existing tree node over registry fetch (#9249)', async t => {
+  // Reproduction: ts-jest has peerOptional jest-util@"^29||^30".
+  // @types/jest@28 → expect@28 → jest-util@28 placed at root first.
+  // jest@29 → jest-util@29 nested (root slot taken by @28).
+  // ts-jest re-queued, peerOptional jest-util resolves to root @28 → INVALID.
+  // Without fix: #nodeFromEdge fetches jest-util@30 (latest ^29||^30), blocks @29.
+  // With fix: #findHoistableNode finds nested @29, PlaceDep hoists it to root.
+  const registry = createRegistry(t, false)
+
+  const jestPack = registry.packument({
+    name: 'jest',
+    version: '29.0.0',
+    dependencies: { 'jest-util': '^29.0.0' },
+  })
+  const jestManifest = registry.manifest({ name: 'jest', packuments: [jestPack] })
+  await registry.package({ manifest: jestManifest })
+
+  const tsJestPack = registry.packument({
+    name: 'ts-jest',
+    version: '29.0.0',
+    peerDependencies: { jest: '^29.0.0', 'jest-util': '^29.0.0 || ^30.0.0' },
+    peerDependenciesMeta: { 'jest-util': { optional: true } },
+  })
+  const tsJestManifest = registry.manifest({ name: 'ts-jest', packuments: [tsJestPack] })
+  await registry.package({ manifest: tsJestManifest })
+
+  const expectPack = registry.packument({
+    name: 'expect',
+    version: '28.0.0',
+    dependencies: { 'jest-util': '^28.0.0' },
+  })
+  const expectManifest = registry.manifest({ name: 'expect', packuments: [expectPack] })
+  await registry.package({ manifest: expectManifest })
+
+  const atTypesPack = registry.packument({
+    name: '@types/jest',
+    version: '28.0.0',
+    dependencies: { expect: '^28.0.0' },
+  })
+  const atTypesManifest = registry.manifest({ name: '@types/jest', packuments: [atTypesPack] })
+  await registry.package({ manifest: atTypesManifest })
+
+  // Only publish 28, 29, and 30.
+  const jestUtilPacks = registry.packuments(['28.0.0', '29.0.0', '30.0.0'], 'jest-util')
+  const jestUtilManifest = registry.manifest({ name: 'jest-util', packuments: jestUtilPacks })
+  await registry.package({ manifest: jestUtilManifest, times: 3 })
+
+  const path = t.testdir({
+    'package.json': JSON.stringify({
+      dependencies: {
+        jest: '^29.0.0',
+        'ts-jest': '^29.0.0',
+        '@types/jest': '^28.0.0',
+      },
+    }),
+  })
+
+  const arb = newArb(path)
+  const tree = await arb.buildIdealTree()
+
+  // jest-util@29 at root — found via #findHoistableNode, not fetched as @30
+  t.equal(tree.children.get('jest-util').version, '29.0.0',
+    'jest-util@29 hoisted to root from nested location')
+
+  // ts-jest's peerOptional resolved to @29 from the tree, not @30 from registry
+  const tsJest = tree.children.get('ts-jest')
+  const peerOptEdge = tsJest.edgesOut.get('jest-util')
+  t.equal(peerOptEdge.to.version, '29.0.0',
+    'ts-jest peerOptional jest-util resolved to @29')
+
+  // jest-util@28 nested under expect (incompatible with root @29)
+  const expectNode = [...tree.inventory.query('name', 'expect')][0]
+  t.equal(expectNode?.children?.get('jest-util')?.version, '28.0.0',
+    'jest-util@28 nested under expect')
+})
+
+t.test('peerOptional skips dedupe shortcut when update.names includes the dep', async t => {
+  // Same scenario as above, but with update: { names: ['jest-util'] }.
+  // skipExistingShortcut=true so #findHoistableNode is NOT called;
+  // #nodeFromEdge fetches from registry, getting jest-util@30 (latest matching ^29||^30).
+  const registry = createRegistry(t, false)
+
+  const jestPack = registry.packument({
+    name: 'jest',
+    version: '29.0.0',
+    dependencies: { 'jest-util': '^29.0.0' },
+  })
+  await registry.package({ manifest: registry.manifest({ name: 'jest', packuments: [jestPack] }) })
+
+  const tsJestPack = registry.packument({
+    name: 'ts-jest',
+    version: '29.0.0',
+    peerDependencies: { jest: '^29.0.0', 'jest-util': '^29.0.0 || ^30.0.0' },
+    peerDependenciesMeta: { 'jest-util': { optional: true } },
+  })
+  await registry.package({ manifest: registry.manifest({ name: 'ts-jest', packuments: [tsJestPack] }) })
+
+  const expectPack = registry.packument({
+    name: 'expect',
+    version: '28.0.0',
+    dependencies: { 'jest-util': '^28.0.0' },
+  })
+  await registry.package({ manifest: registry.manifest({ name: 'expect', packuments: [expectPack] }) })
+
+  const atTypesPack = registry.packument({
+    name: '@types/jest',
+    version: '28.0.0',
+    dependencies: { expect: '^28.0.0' },
+  })
+  await registry.package({ manifest: registry.manifest({ name: '@types/jest', packuments: [atTypesPack] }) })
+
+  const jestUtilPacks = registry.packuments(['28.0.0', '29.0.0', '30.0.0'], 'jest-util')
+  await registry.package({ manifest: registry.manifest({ name: 'jest-util', packuments: jestUtilPacks }), times: 3 })
+
+  const path = t.testdir({
+    'package.json': JSON.stringify({
+      dependencies: {
+        jest: '^29.0.0',
+        'ts-jest': '^29.0.0',
+        '@types/jest': '^28.0.0',
+      },
+    }),
+  })
+
+  const arb = newArb(path)
+  const tree = await arb.buildIdealTree({ update: { names: ['jest-util'] } })
+
+  // With skipExistingShortcut=true, #nodeFromEdge fetches from registry
+  // so jest-util@30 (latest matching ^29||^30) is used instead of deduping @29
+  const tsJest = tree.children.get('ts-jest')
+  const peerOptEdge = tsJest.edgesOut.get('jest-util')
+  t.equal(peerOptEdge.to?.version, '30.0.0', 'peerOptional jest-util refetched to @30, not deduped to @29')
+})
+
 t.test('overrides with bundledDependencies', async t => {
   t.test('does not infinite loop with bundledDependencies and overrides', async t => {
     // https://github.com/npm/cli/issues/9227