fix: only automatically resolve optional peers with versions that satisfy the ranges (#8028)

close #7985

.changeset/curly-actors-repeat.md

@@ -0,0 +1,6 @@
+---
+"@pnpm/resolve-dependencies": patch
+"pnpm": patch
+---
+
+A dependency is hoisted to resolve an optional peer dependency only if it satisfies the range provided for the optional peer dependency [#8028](https://github.com/pnpm/pnpm/pull/8028).

cspell.json

@@ -87,6 +87,7 @@
     "hashbang",
     "highmaps",
     "hikljmi",
+    "hoistable",
     "homepath",
     "hosters",
     "hyperdrive",

pkg-manager/resolve-dependencies/src/hoistPeers.ts

@@ -27,3 +27,28 @@ export function hoistPeers (
   }
   return dependencies
 }
+
+export function getHoistableOptionalPeers (
+  allMissingOptionalPeers: Record<string, string[]>,
+  allPreferredVersions: PreferredVersions
+): Record<string, string> {
+  const optionalDependencies: Record<string, string> = {}
+  for (const [missingOptionalPeerName, ranges] of Object.entries(allMissingOptionalPeers)) {
+    if (!allPreferredVersions[missingOptionalPeerName]) continue
+
+    let maxSatisfyingVersion: string | undefined
+    for (const [version, specType] of Object.entries(allPreferredVersions[missingOptionalPeerName])) {
+      if (
+        specType === 'version' &&
+        ranges.every(range => semver.satisfies(version, range)) &&
+        (!maxSatisfyingVersion || semver.gt(version, maxSatisfyingVersion))
+      ) {
+        maxSatisfyingVersion = version
+      }
+    }
+    if (maxSatisfyingVersion) {
+      optionalDependencies[missingOptionalPeerName] = maxSatisfyingVersion
+    }
+  }
+  return optionalDependencies
+}

pkg-manager/resolve-dependencies/src/resolveDependencies.ts

@@ -57,7 +57,7 @@ import {
   nodeIdContains,
   splitNodeId,
 } from './nodeIdUtils'
-import { hoistPeers } from './hoistPeers'
+import { hoistPeers, getHoistableOptionalPeers } from './hoistPeers'
 import { wantedDepIsLocallyAvailable } from './wantedDepIsLocallyAvailable'
 import { replaceVersionInPref } from './replaceVersionInPref'
 
@@ -303,7 +303,7 @@ export async function resolveRootDependencies (
   const pkgAddressesByImporters = await Promise.all(zipWith(async (importerResolutionResult, { parentPkgAliases, preferredVersions, options }) => {
     const pkgAddresses = importerResolutionResult.pkgAddresses
     if (!ctx.hoistPeers) return pkgAddresses
-    let prevMissingOptionalPeers: string[] = []
+    const allMissingOptionalPeers: Record<string, string[]> = {}
     while (true) {
       for (const pkgAddress of importerResolutionResult.pkgAddresses) {
         parentPkgAliases[pkgAddress.alias] = true
@@ -322,28 +322,17 @@ export async function resolveRootDependencies (
           }
         }
       }
-      const missingOptionalPeerNames = Array.from(
-        new Set(
-          [
-            ...missingOptionalPeers.map(([peerName]) => peerName),
-            ...prevMissingOptionalPeers,
-          ]
-        )
-      )
-      if (!missingRequiredPeers.length && !missingOptionalPeerNames.length) break
-      const dependencies = hoistPeers(missingRequiredPeers, ctx)
-      const nextMissingOptionalPeers: string[] = []
-      const optionalDependencies: Record<string, string> = {}
-      for (const missingOptionalPeerName of missingOptionalPeerNames) {
-        if (ctx.allPreferredVersions![missingOptionalPeerName]) {
-          optionalDependencies[missingOptionalPeerName] = Object.keys(ctx.allPreferredVersions![missingOptionalPeerName]).join(' || ')
-        } else {
-          nextMissingOptionalPeers.push(missingOptionalPeerName)
+      for (const [missingOptionalPeerName, { range: missingOptionalPeerRange }] of missingOptionalPeers) {
+        if (!allMissingOptionalPeers[missingOptionalPeerName]) {
+          allMissingOptionalPeers[missingOptionalPeerName] = [missingOptionalPeerRange]
+        } else if (!allMissingOptionalPeers[missingOptionalPeerName].includes(missingOptionalPeerRange)) {
+          allMissingOptionalPeers[missingOptionalPeerName].push(missingOptionalPeerRange)
         }
       }
-      prevMissingOptionalPeers = nextMissingOptionalPeers
-      if (!Object.keys(dependencies).length && !Object.keys(optionalDependencies).length) break
-      const wantedDependencies = getNonDevWantedDependencies({ dependencies, optionalDependencies })
+      if (!missingRequiredPeers.length) break
+      const dependencies = hoistPeers(missingRequiredPeers, ctx)
+      if (!Object.keys(dependencies).length) break
+      const wantedDependencies = getNonDevWantedDependencies({ dependencies })
 
       // eslint-disable-next-line no-await-in-loop
       const resolveDependenciesResult = await resolveDependencies(ctx, preferredVersions, wantedDependencies, {
@@ -358,6 +347,22 @@ export async function resolveRootDependencies (
       }
       pkgAddresses.push(...importerResolutionResult.pkgAddresses)
     }
+    if (Object.keys(allMissingOptionalPeers).length && ctx.allPreferredVersions) {
+      const optionalDependencies = getHoistableOptionalPeers(allMissingOptionalPeers, ctx.allPreferredVersions)
+      if (Object.keys(optionalDependencies).length) {
+        const wantedDependencies = getNonDevWantedDependencies({ optionalDependencies })
+        const resolveDependenciesResult = await resolveDependencies(ctx, preferredVersions, wantedDependencies, {
+          ...options,
+          parentPkgAliases,
+          publishedBy,
+        })
+        importerResolutionResult = {
+          pkgAddresses: resolveDependenciesResult.pkgAddresses,
+          ...filterMissingPeers(await resolveDependenciesResult.resolvingPeers, parentPkgAliases),
+        }
+        pkgAddresses.push(...importerResolutionResult.pkgAddresses)
+      }
+    }
     return pkgAddresses
   }, pkgAddressesByImportersWithoutPeers, importers))
   return { pkgAddressesByImporters, time }

pkg-manager/resolve-dependencies/test/hoistPeers.test.ts

@@ -1,4 +1,4 @@
-import { hoistPeers } from '../lib/hoistPeers'
+import { hoistPeers, getHoistableOptionalPeers } from '../lib/hoistPeers'
 
 test('hoistPeers picks an already available prerelease version', () => {
   expect(hoistPeers([['foo', { range: '*' }]], {
@@ -12,3 +12,31 @@ test('hoistPeers picks an already available prerelease version', () => {
     foo: '1.0.0-beta.0',
   })
 })
+
+test('getHoistableOptionalPeers only picks a version that satisfies all optional ranges', () => {
+  expect(getHoistableOptionalPeers({
+    foo: ['2', '2.1'],
+  }, {
+    foo: {
+      '1.0.0': 'version',
+      '2.0.0': 'version',
+      '2.1.0': 'version',
+      '3.0.0': 'version',
+    },
+  })).toStrictEqual({
+    foo: '2.1.0',
+  })
+})
+
+test('getHoistableOptionalPeers picks the highest version that satisfies all the optional ranges', () => {
+  expect(getHoistableOptionalPeers({
+    foo: ['2', '2.1'],
+  }, {
+    foo: {
+      '2.1.0': 'version',
+      '2.1.1': 'version',
+    },
+  })).toStrictEqual({
+    foo: '2.1.1',
+  })
+})