From 74b535f192e7bf36eddcb4855c2a67c6559d023e Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Sat, 4 Feb 2023 01:17:22 +0200 Subject: [PATCH] fix: improve deduplication (#6026) --- .changeset/late-books-compete.md | 9 ++ .../core/src/getPeerDependencyIssues.ts | 7 +- .../core/src/install/getPreferredVersions.ts | 42 +++++-- pkg-manager/core/src/install/index.ts | 10 +- .../core/test/install/dedupeInWorkspace.ts | 63 +++++++++++ .../npm-resolver/src/pickPackageFromMeta.ts | 107 ++++++++++++------ resolving/npm-resolver/test/index.ts | 21 ++++ 7 files changed, 201 insertions(+), 58 deletions(-) create mode 100644 .changeset/late-books-compete.md create mode 100644 pkg-manager/core/test/install/dedupeInWorkspace.ts diff --git a/.changeset/late-books-compete.md b/.changeset/late-books-compete.md new file mode 100644 index 0000000000..d32c7f1aa1 --- /dev/null +++ b/.changeset/late-books-compete.md @@ -0,0 +1,9 @@ +--- +"@pnpm/npm-resolver": patch +"@pnpm/core": patch +"pnpm": patch +--- + +Deduplicate direct dependencies. + +Let's say there are two projects in the workspace that dependend on `foo`. One project has `foo@1.0.0` in the dependencies while another one has `foo@^1.0.0` in the dependencies. In this case, `foo@1.0.0` should be installed to both projects as satisfies the version specs of both projects. diff --git a/pkg-manager/core/src/getPeerDependencyIssues.ts b/pkg-manager/core/src/getPeerDependencyIssues.ts index 31589f8442..5e3a3323da 100644 --- a/pkg-manager/core/src/getPeerDependencyIssues.ts +++ b/pkg-manager/core/src/getPeerDependencyIssues.ts @@ -2,7 +2,7 @@ import { resolveDependencies, getWantedDependencies } from '@pnpm/resolve-depend import { PeerDependencyIssuesByProjects } from '@pnpm/types' import { getContext, GetContextOptions, ProjectOptions } from '@pnpm/get-context' import { createReadPackageHook } from '@pnpm/hooks.read-package-hook' -import { getPreferredVersionsFromLockfile } from './install/getPreferredVersions' +import { getPreferredVersionsFromLockfileAndManifests } from './install/getPreferredVersions' import { InstallOptions } from './install/extendInstallOptions' import { DEFAULT_REGISTRIES } from '@pnpm/normalize-registries' @@ -43,7 +43,10 @@ export async function getPeerDependencyIssues ( updatePackageManifest: false, wantedDependencies: getWantedDependencies(project.manifest), })) - const preferredVersions = ctx.wantedLockfile.packages ? getPreferredVersionsFromLockfile(ctx.wantedLockfile.packages) : undefined + const preferredVersions = getPreferredVersionsFromLockfileAndManifests( + ctx.wantedLockfile.packages, + Object.values(ctx.projects).map(({ manifest }) => manifest) + ) const { peerDependencyIssuesByProjects, waitTillAllFetchingsFinish, diff --git a/pkg-manager/core/src/install/getPreferredVersions.ts b/pkg-manager/core/src/install/getPreferredVersions.ts index 49ac775d6a..8168019f2b 100644 --- a/pkg-manager/core/src/install/getPreferredVersions.ts +++ b/pkg-manager/core/src/install/getPreferredVersions.ts @@ -1,7 +1,8 @@ import { nameVerFromPkgSnapshot, PackageSnapshots } from '@pnpm/lockfile-utils' import { getAllDependenciesFromManifest } from '@pnpm/manifest-utils' import { PreferredVersions } from '@pnpm/resolver-base' -import { DependencyManifest } from '@pnpm/types' +import { DependencyManifest, ProjectManifest } from '@pnpm/types' +import getVersionSelectorType from 'version-selector-type' export function getAllUniqueSpecs (manifests: DependencyManifest[]) { const allSpecs: Record = {} @@ -21,15 +22,32 @@ export function getAllUniqueSpecs (manifests: DependencyManifest[]) { return allSpecs } -export function getPreferredVersionsFromLockfile (snapshots: PackageSnapshots): PreferredVersions { - return Object.entries(snapshots) - .map(([depPath, snapshot]) => nameVerFromPkgSnapshot(depPath, snapshot)) - .reduce((preferredVersions, { name, version }) => { - if (!preferredVersions[name]) { - preferredVersions[name] = { [version]: 'version' } - } else { - preferredVersions[name][version] = 'version' - } - return preferredVersions - }, {} as PreferredVersions) +export function getPreferredVersionsFromLockfileAndManifests ( + snapshots: PackageSnapshots | undefined, + manifests: Array +): PreferredVersions { + const preferredVersions: PreferredVersions = {} + for (const manifest of manifests) { + const specs = getAllDependenciesFromManifest(manifest) + for (const [name, spec] of Object.entries(specs)) { + const selector = getVersionSelectorType(spec) + if (!selector) continue + preferredVersions[name] = preferredVersions[name] ?? {} + preferredVersions[name][spec] = selector.type + } + } + if (!snapshots) return preferredVersions + addPreferredVersionsFromLockfile(snapshots, preferredVersions) + return preferredVersions +} + +function addPreferredVersionsFromLockfile (snapshots: PackageSnapshots, preferredVersions: PreferredVersions) { + for (const [depPath, snapshot] of Object.entries(snapshots)) { + const { name, version } = nameVerFromPkgSnapshot(depPath, snapshot) + if (!preferredVersions[name]) { + preferredVersions[name] = { [version]: 'version' } + } else { + preferredVersions[name][version] = 'version' + } + } } diff --git a/pkg-manager/core/src/install/index.ts b/pkg-manager/core/src/install/index.ts index 1020f6be45..918932485f 100644 --- a/pkg-manager/core/src/install/index.ts +++ b/pkg-manager/core/src/install/index.ts @@ -78,7 +78,7 @@ import { InstallOptions, ProcessedInstallOptions as StrictInstallOptions, } from './extendInstallOptions' -import { getPreferredVersionsFromLockfile, getAllUniqueSpecs } from './getPreferredVersions' +import { getAllUniqueSpecs, getPreferredVersionsFromLockfileAndManifests } from './getPreferredVersions' import { linkPackages } from './link' import { reportPeerDependencyIssues } from './reportPeerDependencyIssues' @@ -793,12 +793,8 @@ const _installInContext: InstallFunction = async (projects, ctx, opts) => { }) const preferredVersions = opts.preferredVersions ?? ( - ( - !opts.update && - (ctx.wantedLockfile.packages != null) && - !isEmpty(ctx.wantedLockfile.packages) - ) - ? getPreferredVersionsFromLockfile(ctx.wantedLockfile.packages) + !opts.update + ? getPreferredVersionsFromLockfileAndManifests(ctx.wantedLockfile.packages, Object.values(ctx.projects).map(({ manifest }) => manifest)) : undefined ) const forceFullResolution = ctx.wantedLockfile.lockfileVersion !== LOCKFILE_VERSION || diff --git a/pkg-manager/core/test/install/dedupeInWorkspace.ts b/pkg-manager/core/test/install/dedupeInWorkspace.ts new file mode 100644 index 0000000000..6c2b3176b4 --- /dev/null +++ b/pkg-manager/core/test/install/dedupeInWorkspace.ts @@ -0,0 +1,63 @@ +import path from 'path' +import { assertProject } from '@pnpm/assert-project' +import { preparePackages } from '@pnpm/prepare' +import { mutateModules, MutatedProject } from '@pnpm/core' +import { addDistTag } from '@pnpm/registry-mock' +import { testDefaults } from '../utils' + +test('pick common range for a dependency used in two workspace projects', async () => { + await addDistTag({ package: '@pnpm.e2e/dep-of-pkg-with-1-dep', version: '100.1.0', distTag: 'latest' }) + preparePackages([ + { + location: 'project-1', + package: { name: 'project-1' }, + }, + { + location: 'project-2', + package: { name: 'project-2' }, + }, + ]) + + const importers: MutatedProject[] = [ + { + mutation: 'install', + rootDir: path.resolve('project-1'), + }, + { + mutation: 'install', + rootDir: path.resolve('project-2'), + }, + ] + const allProjects = [ + { + buildIndex: 0, + manifest: { + name: 'project-1', + version: '1.0.0', + + dependencies: { + '@pnpm.e2e/dep-of-pkg-with-1-dep': '100.0.0', + }, + }, + rootDir: path.resolve('project-1'), + }, + { + buildIndex: 0, + manifest: { + name: 'project-2', + version: '1.0.0', + + dependencies: { + '@pnpm.e2e/dep-of-pkg-with-1-dep': '^100.0.0', + }, + }, + rootDir: path.resolve('project-2'), + }, + ] + await mutateModules(importers, await testDefaults({ allProjects, lockfileOnly: true })) + + const project = assertProject(process.cwd()) + const lockfile = await project.readLockfile() + expect(lockfile.packages).toHaveProperty(['/@pnpm.e2e/dep-of-pkg-with-1-dep/100.0.0']) + expect(lockfile.packages).not.toHaveProperty(['/@pnpm.e2e/dep-of-pkg-with-1-dep/100.1.0']) +}) diff --git a/resolving/npm-resolver/src/pickPackageFromMeta.ts b/resolving/npm-resolver/src/pickPackageFromMeta.ts index b7f0fdd27c..d4c8b12a89 100644 --- a/resolving/npm-resolver/src/pickPackageFromMeta.ts +++ b/resolving/npm-resolver/src/pickPackageFromMeta.ts @@ -71,50 +71,22 @@ export function pickVersionByVersionRange ( preferredVerSels?: VersionSelectors, publishedBy?: Date ) { - let versions: string[] | undefined let latest: string | undefined = meta['dist-tags'].latest - const preferredVerSelsArr = Object.entries(preferredVerSels ?? {}) - if (preferredVerSelsArr.length > 0) { - const preferredVersions: string[] = [] - for (const [preferredSelector, preferredSelectorType] of preferredVerSelsArr) { - if (preferredSelector === versionRange) continue - switch (preferredSelectorType) { - case 'tag': { - preferredVersions.push(meta['dist-tags'][preferredSelector]) - break + if (preferredVerSels != null && Object.keys(preferredVerSels).length > 0) { + const prioritizedPreferredVersions = prioritizePreferredVersions(meta, versionRange, preferredVerSels) + for (const preferredVersions of prioritizedPreferredVersions) { + if (preferredVersions.includes(latest) && semver.satisfies(latest, versionRange, true)) { + return latest } - case 'range': { - // This might be slow if there are many versions - // and the package is an indirect dependency many times in the project. - // If it will create noticeable slowdown, then might be a good idea to add some caching - versions = Object.keys(meta.versions) - for (const version of versions) { - if (semver.satisfies(version, preferredSelector, true)) { - preferredVersions.push(version) - } - } - break + const preferredVersion = semver.maxSatisfying(preferredVersions, versionRange, true) + if (preferredVersion) { + return preferredVersion } - case 'version': { - if (meta.versions[preferredSelector]) { - preferredVersions.push(preferredSelector) - } - break - } - } - } - - if (preferredVersions.includes(latest) && semver.satisfies(latest, versionRange, true)) { - return latest - } - const preferredVersion = semver.maxSatisfying(preferredVersions, versionRange, true) - if (preferredVersion) { - return preferredVersion } } - versions = versions ?? Object.keys(meta.versions) + let versions = Object.keys(meta.versions) if (publishedBy) { versions = versions.filter(version => new Date(meta.time![version]) <= publishedBy) if (!versions.includes(latest)) { @@ -140,3 +112,64 @@ export function pickVersionByVersionRange ( } return maxVersion } + +function prioritizePreferredVersions ( + meta: PackageMeta, + versionRange: string, + preferredVerSels?: VersionSelectors +): string[][] { + const preferredVerSelsArr = Object.entries(preferredVerSels ?? {}) + const versionsPrioritizer = new PreferredVersionsPrioritizer() + for (const [preferredSelector, preferredSelectorType] of preferredVerSelsArr) { + if (preferredSelector === versionRange) continue + switch (preferredSelectorType) { + case 'tag': { + versionsPrioritizer.add(meta['dist-tags'][preferredSelector]) + break + } + case 'range': { + // This might be slow if there are many versions + // and the package is an indirect dependency many times in the project. + // If it will create noticeable slowdown, then might be a good idea to add some caching + const versions = Object.keys(meta.versions) + for (const version of versions) { + if (semver.satisfies(version, preferredSelector, true)) { + versionsPrioritizer.add(version) + } + } + break + } + case 'version': { + if (meta.versions[preferredSelector]) { + versionsPrioritizer.add(preferredSelector) + } + break + } + } + } + return versionsPrioritizer.versionsByPriority() +} + +class PreferredVersionsPrioritizer { + private preferredVersions: Record = {} + + add (version: string) { + if (!this.preferredVersions[version]) { + this.preferredVersions[version] = 1 + } else { + this.preferredVersions[version]++ + } + } + + versionsByPriority () { + const versionsByOccurrences = Object.entries(this.preferredVersions) + .reduce((acc, [version, occurrences]) => { + acc[occurrences] = acc[occurrences] ?? [] + acc[occurrences].push(version) + return acc + }, {} as Record) + return Object.keys(versionsByOccurrences) + .sort((a, b) => parseInt(b, 10) - parseInt(a, 10)) + .map((occurrences) => versionsByOccurrences[parseInt(occurrences, 10)]) + } +} diff --git a/resolving/npm-resolver/test/index.ts b/resolving/npm-resolver/test/index.ts index 2f24bf7141..22513d75bd 100644 --- a/resolving/npm-resolver/test/index.ts +++ b/resolving/npm-resolver/test/index.ts @@ -586,6 +586,27 @@ test("prefer a version that is both inside the wanted and preferred ranges. Even expect(resolveResult!.id).toBe('registry.npmjs.org/is-positive/1.0.0') }) +test('prefer the version that is matched by more preferred selectors', async () => { + nock(registry) + .get('/is-positive') + .reply(200, isPositiveMeta) + + const resolveFromNpm = createResolveFromNpm({ + cacheDir: tempy.directory(), + }) + const resolveResult = await resolveFromNpm({ + alias: 'is-positive', + pref: '^3.0.0', + }, { + preferredVersions: { + 'is-positive': { '^3.0.0': 'range', '3.0.0': 'version' }, + }, + registry, + }) + + expect(resolveResult!.id).toBe('registry.npmjs.org/is-positive/3.0.0') +}) + test('offline resolution fails when package meta not found in the store', async () => { const cacheDir = tempy.directory() const resolve = createResolveFromNpm({