fix: improve deduplication (#6026)

.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.

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,

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<string, string> = {}
@@ -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<DependencyManifest | ProjectManifest>
+): 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'
+    }
+  }
 }

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 ||

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'])
+})

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<string, number> = {}
+
+  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<number, string[]>)
+    return Object.keys(versionsByOccurrences)
+      .sort((a, b) => parseInt(b, 10) - parseInt(a, 10))
+      .map((occurrences) => versionsByOccurrences[parseInt(occurrences, 10)])
+  }
+}

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({