From b7820a0fe740d4bf6efb000ca487cbcf7bc3c89a Mon Sep 17 00:00:00 2001 From: Magnus Klingenberg Date: Sat, 3 Jun 2023 00:32:27 +0200 Subject: [PATCH] fix(resolvePeers): fix deduplication when version missmatch (#6606) When dedupe-peer-dependents is enabled (default), use the path to determine compatibility. When multiple dependency groups can be deduplicated, the latter ones are sorted according to number of peers to allow them to benefit from deduplication. close #6605 --------- Co-authored-by: Zoltan Kochan --- .changeset/gentle-lies-play.md | 13 +++ .../resolve-dependencies/src/resolvePeers.ts | 60 +++++++--- .../test/dedupeDepPaths.test.ts | 106 ++++++++++++++++++ 3 files changed, 164 insertions(+), 15 deletions(-) create mode 100644 .changeset/gentle-lies-play.md create mode 100644 pkg-manager/resolve-dependencies/test/dedupeDepPaths.test.ts diff --git a/.changeset/gentle-lies-play.md b/.changeset/gentle-lies-play.md new file mode 100644 index 00000000000..dda5fa546d3 --- /dev/null +++ b/.changeset/gentle-lies-play.md @@ -0,0 +1,13 @@ +--- +"@pnpm/resolve-dependencies": patch +"pnpm": patch +--- + +When `dedupe-peer-dependents` is enabled (default), use the path (not id) to +determine compatibility. + +When multiple dependency groups can be deduplicated, the +latter ones are sorted according to number of peers to allow them to +benefit from deduplication. + +Resolves: [#6605](https://github.com/pnpm/pnpm/issues/6605) diff --git a/pkg-manager/resolve-dependencies/src/resolvePeers.ts b/pkg-manager/resolve-dependencies/src/resolvePeers.ts index c291d0319fa..830510f7d9e 100644 --- a/pkg-manager/resolve-dependencies/src/resolvePeers.ts +++ b/pkg-manager/resolve-dependencies/src/resolvePeers.ts @@ -110,17 +110,15 @@ export function resolvePeers ( node.children = mapValues((childNodeId) => pathsByNodeId[childNodeId] ?? childNodeId, node.children) }) - const dependenciesByProjectId: { [id: string]: { [alias: string]: string } } = {} + const dependenciesByProjectId: { [id: string]: Record } = {} for (const { directNodeIdsByAlias, id } of opts.projects) { dependenciesByProjectId[id] = mapValues((nodeId) => pathsByNodeId[nodeId], directNodeIdsByAlias) } if (opts.dedupePeerDependents) { - const depPathsMap = deduplicateDepPaths(depPathsByPkgId, depGraph) - Object.values(depGraph).forEach((node) => { - node.children = mapValues((childDepPath) => depPathsMap[childDepPath] ?? childDepPath, node.children) - }) + const duplicates = Object.values(depPathsByPkgId).filter((item) => item.length > 1) + const allDepPathsMap = deduplicateAll(depGraph, duplicates) for (const { id } of opts.projects) { - dependenciesByProjectId[id] = mapValues((depPath) => depPathsMap[depPath] ?? depPath, dependenciesByProjectId[id]) + dependenciesByProjectId[id] = mapValues((depPath) => allDepPathsMap[depPath] ?? depPath, dependenciesByProjectId[id]) } } return { @@ -134,15 +132,38 @@ function nodeDepsCount (node: GenericDependenciesGraphNode) { return Object.keys(node.children).length + node.resolvedPeerNames.length } +function deduplicateAll ( + depGraph: GenericDependenciesGraph, + duplicates: string[][] +): Record { + const { depPathsMap, remainingDuplicates } = deduplicateDepPaths(duplicates, depGraph) + if (remainingDuplicates.length === duplicates.length) { + return depPathsMap + } + Object.values(depGraph).forEach((node) => { + node.children = mapValues((childDepPath) => depPathsMap[childDepPath] ?? childDepPath, node.children) + }) + if (Object.keys(depPathsMap).length > 0) { + return { + ...depPathsMap, + ...deduplicateAll(depGraph, remainingDuplicates), + } + } + return depPathsMap +} + function deduplicateDepPaths ( - depPathsByPkgId: Record, + duplicates: string[][], depGraph: GenericDependenciesGraph ) { + const depCountSorter = (depPath1: string, depPath2: string) => nodeDepsCount(depGraph[depPath1]) - nodeDepsCount(depGraph[depPath2]) const depPathsMap: Record = {} - for (let depPaths of Object.values(depPathsByPkgId)) { - if (depPaths.length === 1) continue - depPaths = depPaths.sort((depPath1, depPath2) => nodeDepsCount(depGraph[depPath1]) - nodeDepsCount(depGraph[depPath2])) - let currentDepPaths = depPaths + const remainingDuplicates: string[][] = [] + + for (const depPaths of duplicates) { + const unresolvedDepPaths = new Set(depPaths) + let currentDepPaths = depPaths.sort(depCountSorter) + while (currentDepPaths.length) { const depPath1 = currentDepPaths.pop()! const nextDepPaths = [] @@ -150,15 +171,24 @@ function deduplicateDepPaths ( const depPath2 = currentDepPaths.pop()! if (isCompatibleAndHasMoreDeps(depGraph, depPath1, depPath2)) { depPathsMap[depPath2] = depPath1 + unresolvedDepPaths.delete(depPath1) + unresolvedDepPaths.delete(depPath2) } else { nextDepPaths.push(depPath2) } } nextDepPaths.push(...currentDepPaths) - currentDepPaths = nextDepPaths + currentDepPaths = nextDepPaths.sort(depCountSorter) + } + + if (unresolvedDepPaths.size) { + remainingDuplicates.push([...unresolvedDepPaths]) } } - return depPathsMap + return { + depPathsMap, + remainingDuplicates, + } } function isCompatibleAndHasMoreDeps ( @@ -168,8 +198,8 @@ function isCompatibleAndHasMoreDeps ( ) { const node1 = depGraph[depPath1] const node2 = depGraph[depPath2] - const node1DepPaths = Object.keys(node1.children) - const node2DepPaths = Object.keys(node2.children) + const node1DepPaths = Object.values(node1.children) + const node2DepPaths = Object.values(node2.children) return nodeDepsCount(node1) > nodeDepsCount(node2) && node2DepPaths.every((depPath) => node1DepPaths.includes(depPath)) && node2.resolvedPeerNames.every((depPath) => node1.resolvedPeerNames.includes(depPath)) diff --git a/pkg-manager/resolve-dependencies/test/dedupeDepPaths.test.ts b/pkg-manager/resolve-dependencies/test/dedupeDepPaths.test.ts new file mode 100644 index 00000000000..60fff069049 --- /dev/null +++ b/pkg-manager/resolve-dependencies/test/dedupeDepPaths.test.ts @@ -0,0 +1,106 @@ +import { resolvePeers } from '../lib/resolvePeers' + +test('packages are not deduplicated when versions do not match', () => { + const fooPkg = { + name: 'foo', + version: '1.0.0', + depPath: 'foo/1.0.0', + peerDependencies: { + bar: '1.0.0 || 2.0.0', + baz: '1.0.0 || 2.0.0', + }, + peerDependenciesMeta: { + baz: { + optional: true, + }, + }, + } + + const peers = Object.fromEntries( + [ + ['bar', '1.0.0'], + ['bar', '2.0.0'], + ['baz', '1.0.0'], + ['baz', '2.0.0'], + ].map(([name, version]) => [ + `${name}_${version.replace(/\./g, '_')}`, + { + name, + version, + depPath: `${name}/${version}`, + peerDependencies: {}, + }, + ]) + ) + + const { dependenciesByProjectId } = resolvePeers({ + projects: [ + { + directNodeIdsByAlias: { + foo: '>project1>foo/1.0.0>', + bar: '>project1>bar/1.0.0>', + }, + topParents: [], + rootDir: '', + id: 'project1', + }, + { + directNodeIdsByAlias: { + foo: '>project2>foo/1.0.0>', + bar: '>project2>bar/1.0.0>', + baz: '>project2>baz/1.0.0>', + }, + topParents: [], + rootDir: '', + id: 'project2', + }, + { + directNodeIdsByAlias: { + foo: '>project3>foo/1.0.0>', + bar: '>project3>bar/2.0.0>', + }, + topParents: [], + rootDir: '', + id: 'project3', + }, + { + directNodeIdsByAlias: { + foo: '>project4>foo/1.0.0>', + bar: '>project4>bar/2.0.0>', + baz: '>project4>baz/2.0.0>', + }, + topParents: [], + rootDir: '', + id: 'project4', + }, + ], + dependenciesTree: Object.fromEntries([ + ['>project1>foo/1.0.0>', fooPkg], + ['>project1>bar/1.0.0>', peers.bar_1_0_0], + + ['>project2>foo/1.0.0>', fooPkg], + ['>project2>bar/1.0.0>', peers.bar_1_0_0], + ['>project2>baz/1.0.0>', peers.baz_1_0_0], + + ['>project3>foo/1.0.0>', fooPkg], + ['>project3>bar/2.0.0>', peers.bar_2_0_0], + + ['>project4>foo/1.0.0>', fooPkg], + ['>project4>bar/2.0.0>', peers.bar_2_0_0], + ['>project4>baz/2.0.0>', peers.baz_2_0_0], + + ].map(([path, resolvedPackage]) => [path, { + children: {}, + installable: {}, + resolvedPackage, + depth: 0, + }])), + dedupePeerDependents: true, + virtualStoreDir: '', + lockfileDir: '', + }) + + expect(dependenciesByProjectId.project1.foo).toEqual(dependenciesByProjectId.project2.foo) + expect(dependenciesByProjectId.project1.foo).not.toEqual(dependenciesByProjectId.project3.foo) + expect(dependenciesByProjectId.project3.foo).toEqual(dependenciesByProjectId.project4.foo) +})