revert: resolve peer having peer correctly

ref #7583
pnpm · Feb 3, 2024 · f5eadba · f5eadba
1 parent 1a3449e
commit f5eadba
Show file tree

Hide file tree

Showing 4 changed files with 52 additions and 95 deletions.
diff --git a/.changeset/little-badgers-repair.md b/.changeset/little-badgers-repair.md
@@ -0,0 +1,5 @@
+---
+"@pnpm/resolve-dependencies": patch
+---
+
+Revert [#7583](https://github.com/pnpm/pnpm/pull/7583).
diff --git a/pkg-manager/core/test/install/peerDependencies.ts b/pkg-manager/core/test/install/peerDependencies.ts
@@ -1466,7 +1466,8 @@ test('in a subdependency, when there are several aliased dependencies of the sam
   expect(lockfile.packages['/@pnpm.e2e/abc@1.0.0(@pnpm.e2e/peer-c@2.0.0)']).toBeTruthy()
 })
 
-test('peer having peer is resolved correctly', async () => {
+// TODO: fix this test
+test.skip('peer having peer is resolved correctly', async () => {
   const manifest1 = {
     name: 'project-1',
 

diff --git a/pkg-manager/resolve-dependencies/src/resolvePeers.ts b/pkg-manager/resolve-dependencies/src/resolvePeers.ts
@@ -351,24 +351,35 @@ function resolvePeersOfNode<T extends PartialResolvedPackage> (
   }
 
   const {
-    resolvedPeers: resolvedPeersOfChildren,
+    resolvedPeers: unknownResolvedPeersOfChildren,
     missingPeers: missingPeersOfChildren,
   } = resolvePeersOfChildren(children, parentPkgs, ctx)
 
-  const { allMissingPeers, allResolvedPeers } = resolvePeersAndTheirPeers({
-    currentDepth: node.depth,
-    dependenciesTree: ctx.dependenciesTree,
-    lockfileDir: ctx.lockfileDir,
-    nodeId,
-    parentPkgs,
-    peerDependencyIssues: ctx.peerDependencyIssues,
-    resolvedPackage,
-    rootDir: ctx.rootDir,
-    resolvedPeersOfChildren,
-  })
+  const { resolvedPeers, missingPeers } = Object.keys(resolvedPackage.peerDependencies).length === 0
+    ? { resolvedPeers: new Map<string, string>(), missingPeers: new Set<string>() }
+    : _resolvePeers({
+      currentDepth: node.depth,
+      dependenciesTree: ctx.dependenciesTree,
+      lockfileDir: ctx.lockfileDir,
+      nodeId,
+      parentPkgs,
+      peerDependencyIssues: ctx.peerDependencyIssues,
+      resolvedPackage,
+      rootDir: ctx.rootDir,
+    })
 
-  for (const missingPeer of missingPeersOfChildren) {
-    allMissingPeers.add(missingPeer)
+  const allResolvedPeers = unknownResolvedPeersOfChildren
+  for (const [k, v] of resolvedPeers) {
+    allResolvedPeers.set(k, v)
+  }
+  allResolvedPeers.delete(node.resolvedPackage.name)
+
+  const allMissingPeers = new Set<string>()
+  for (const peer of missingPeersOfChildren) {
+    allMissingPeers.add(peer)
+  }
+  for (const peer of missingPeers) {
+    allMissingPeers.add(peer)
   }
 
   let depPath: string
@@ -438,7 +449,7 @@ function resolvePeersOfNode<T extends PartialResolvedPackage> (
       children: Object.assign(
         getPreviouslyResolvedChildren(nodeId, ctx.dependenciesTree),
         children,
-        Object.fromEntries(Array.from(allResolvedPeers.entries()).filter(([peerName]) => resolvedPackage.peerDependencies[peerName]))
+        Object.fromEntries(resolvedPeers.entries())
       ),
       depPath,
       depth: node.depth,
@@ -528,83 +539,21 @@ function resolvePeersOfChildren<T extends PartialResolvedPackage> (
   return { resolvedPeers: unknownResolvedPeersOfChildren, missingPeers: allMissingPeers }
 }
 
-function resolvePeersAndTheirPeers<T extends PartialResolvedPackage> (
-  ctx: Omit<ResolvePeersOptions<T>, 'directParentPkg'> & {
-    resolvedPackage: Pick<PartialResolvedPackage, 'name' | 'version' | 'peerDependencies'>
-    resolvedPeersOfChildren: Map<string, string>
-  }
-) {
-  const allMissingPeers = new Set<string>()
-  const allResolvedPeers = ctx.resolvedPeersOfChildren
-  let peerDependencies = ctx.resolvedPackage.peerDependencies
-  for (const peerNodeId of allResolvedPeers.values()) {
-    const peerNode = ctx.dependenciesTree.get(peerNodeId)
-    if (!peerNode) continue
-    const peerPkg = peerNode.resolvedPackage as T
-    peerDependencies = {
-      ...peerPkg.peerDependencies,
-      ...peerDependencies,
-    }
-  }
-  const directParentPkg = {
-    name: ctx.resolvedPackage.name,
-    version: ctx.resolvedPackage.version,
-  }
-  const _resolvePeersFn = _resolvePeers.bind(null, {
-    currentDepth: ctx.currentDepth,
-    dependenciesTree: ctx.dependenciesTree,
-    directParentPkg,
-    lockfileDir: ctx.lockfileDir,
-    nodeId: ctx.nodeId,
-    parentPkgs: ctx.parentPkgs,
-    peerDependencyIssues: ctx.peerDependencyIssues,
-    rootDir: ctx.rootDir,
-  })
-  while (Object.keys(peerDependencies).length > 0) {
-    const { resolvedPeers, missingPeers } = _resolvePeersFn(peerDependencies)
-    for (const peer of missingPeers) {
-      allMissingPeers.add(peer)
-    }
-    peerDependencies = {}
-    for (const peerNodeId of resolvedPeers.values()) {
-      const peerNode = ctx.dependenciesTree.get(peerNodeId)
-      if (!peerNode) continue
-      const peerPkg = peerNode.resolvedPackage as T
-      for (const [peerName, peer] of Object.entries(peerPkg.peerDependencies ?? {})) {
-        if (!allResolvedPeers.has(peerName)) {
-          // It might happen that there are multiple peers depending on the same peers.
-          // In this case we pick the peer information only from one dependency.
-          // This will not break anything except possibly peer dependency warnings.
-          peerDependencies[peerName] = peer
-        }
-      }
-    }
-    for (const [alias, nodeId] of resolvedPeers) {
-      allResolvedPeers.set(alias, nodeId)
-    }
-  }
-  allResolvedPeers.delete(ctx.resolvedPackage.name)
-  return { allMissingPeers, allResolvedPeers }
-}
-
-interface ResolvePeersOptions<T> {
-  currentDepth: number
-  dependenciesTree: DependenciesTree<T>
-  directParentPkg: Pick<PartialResolvedPackage, 'name' | 'version'>
-  lockfileDir: string
-  nodeId: string
-  parentPkgs: ParentRefs
-  peerDependencyIssues: Pick<PeerDependencyIssues, 'bad' | 'missing'>
-  rootDir: string
-}
-
 function _resolvePeers<T extends PartialResolvedPackage> (
-  ctx: ResolvePeersOptions<T>,
-  peerDependencies: PeerDependencies
+  ctx: {
+    currentDepth: number
+    lockfileDir: string
+    nodeId: string
+    parentPkgs: ParentRefs
+    resolvedPackage: T
+    dependenciesTree: DependenciesTree<T>
+    rootDir: string
+    peerDependencyIssues: Pick<PeerDependencyIssues, 'bad' | 'missing'>
+  }
 ): PeersResolution {
   const resolvedPeers = new Map<string, string>()
   const missingPeers = new Set<string>()
-  for (const [peerName, { version, optional }] of Object.entries(peerDependencies)) {
+  for (const [peerName, { version, optional }] of Object.entries(ctx.resolvedPackage.peerDependencies)) {
     const peerVersionRange = version.replace(/^workspace:/, '')
 
     const resolved = ctx.parentPkgs[peerName]
@@ -615,7 +564,7 @@ function _resolvePeers<T extends PartialResolvedPackage> (
       const location = getLocationFromNodeIdAndPkg({
         dependenciesTree: ctx.dependenciesTree,
         nodeId: ctx.nodeId,
-        pkg: ctx.directParentPkg,
+        pkg: ctx.resolvedPackage,
       })
       if (!ctx.peerDependencyIssues.missing[peerName]) {
         ctx.peerDependencyIssues.missing[peerName] = []
@@ -632,7 +581,7 @@ function _resolvePeers<T extends PartialResolvedPackage> (
       const location = getLocationFromNodeIdAndPkg({
         dependenciesTree: ctx.dependenciesTree,
         nodeId: ctx.nodeId,
-        pkg: ctx.directParentPkg,
+        pkg: ctx.resolvedPackage,
       })
       if (!ctx.peerDependencyIssues.bad[peerName]) {
         ctx.peerDependencyIssues.bad[peerName] = []

diff --git a/pkg-manager/resolve-dependencies/test/resolvePeers.ts b/pkg-manager/resolve-dependencies/test/resolvePeers.ts
@@ -103,10 +103,12 @@ test('resolve peer dependencies of cyclic dependencies', () => {
     lockfileDir: '',
   })
   expect(Object.keys(dependenciesGraph)).toStrictEqual([
-    'foo/1.0.0(bar@1.0.0)(qar@1.0.0)(zoo@1.0.0)',
-    'bar/1.0.0(foo@1.0.0)(qar@1.0.0)(zoo@1.0.0)',
-    'zoo/1.0.0(bar@1.0.0)(foo@1.0.0)(qar@1.0.0)',
-    'qar/1.0.0(bar@1.0.0)(foo@1.0.0)(zoo@1.0.0)',
+    'foo/1.0.0(qar@1.0.0)(zoo@1.0.0)',
+    'bar/1.0.0(foo@1.0.0)(zoo@1.0.0)',
+    'zoo/1.0.0(qar@1.0.0)',
+    'qar/1.0.0(bar@1.0.0)(foo@1.0.0)',
+    'bar/1.0.0(foo@1.0.0)',
+    'foo/1.0.0',
   ])
 })