From f5eadba94394114a303d6bedbc78cf8f4de75ad4 Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Sat, 3 Feb 2024 21:57:55 +0100 Subject: [PATCH] revert: resolve peer having peer correctly ref #7583 --- .changeset/little-badgers-repair.md | 5 + .../core/test/install/peerDependencies.ts | 3 +- .../resolve-dependencies/src/resolvePeers.ts | 129 ++++++------------ .../resolve-dependencies/test/resolvePeers.ts | 10 +- 4 files changed, 52 insertions(+), 95 deletions(-) create mode 100644 .changeset/little-badgers-repair.md diff --git a/.changeset/little-badgers-repair.md b/.changeset/little-badgers-repair.md new file mode 100644 index 0000000000..5318e9fefd --- /dev/null +++ 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 index 3e5d10ab3d..ee047d7ca3 100644 --- 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 index 3387f5f3e3..5efddb332e 100644 --- a/pkg-manager/resolve-dependencies/src/resolvePeers.ts +++ b/pkg-manager/resolve-dependencies/src/resolvePeers.ts @@ -351,24 +351,35 @@ function resolvePeersOfNode ( } 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(), missingPeers: new Set() } + : _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() + 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 ( 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 ( return { resolvedPeers: unknownResolvedPeersOfChildren, missingPeers: allMissingPeers } } -function resolvePeersAndTheirPeers ( - ctx: Omit, 'directParentPkg'> & { - resolvedPackage: Pick - resolvedPeersOfChildren: Map - } -) { - const allMissingPeers = new Set() - 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 { - currentDepth: number - dependenciesTree: DependenciesTree - directParentPkg: Pick - lockfileDir: string - nodeId: string - parentPkgs: ParentRefs - peerDependencyIssues: Pick - rootDir: string -} - function _resolvePeers ( - ctx: ResolvePeersOptions, - peerDependencies: PeerDependencies + ctx: { + currentDepth: number + lockfileDir: string + nodeId: string + parentPkgs: ParentRefs + resolvedPackage: T + dependenciesTree: DependenciesTree + rootDir: string + peerDependencyIssues: Pick + } ): PeersResolution { const resolvedPeers = new Map() const missingPeers = new Set() - 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 ( 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 ( 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 index 6970747bb6..0797e8c0bc 100644 --- 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', ]) })