Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from
Apr 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/curly-actors-repeat.md
Original file line number Diff line number Diff line change
@@ -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).
1 change: 1 addition & 0 deletions cspell.json
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@
"hashbang",
"highmaps",
"hikljmi",
"hoistable",
"homepath",
"hosters",
"hyperdrive",
Expand Down
25 changes: 25 additions & 0 deletions pkg-manager/resolve-dependencies/src/hoistPeers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
49 changes: 27 additions & 22 deletions pkg-manager/resolve-dependencies/src/resolveDependencies.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'

Expand Down Expand Up @@ -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
Expand All @@ -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, {
Expand All @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this happens above on line 48 too, but I'd be a bit worried about stack overflows here with large arrays. It might be safer to use a for loop since that doesn't have the same downside?

Are we guaranteed importerResolutionResult.pkgAddresses is small? I'm guessing it will be 99% of the time, but still depends on user input.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what you mean. How does an array concatenation cause a stackoverflow?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I should have cited something. The best discussion of the issue I can find is:

nodejs/node#16870

The problem is that all arguments are placed onto the stack one by one before calling .push, which means that this code can throw depending on how deep in the stack we are already and argument size.

I tend to avoid spreading arguments into a function if the arguments aren't a fixed size so I don't have to think about this problem.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh wow, good to know

}
}
return pkgAddresses
}, pkgAddressesByImportersWithoutPeers, importers))
return { pkgAddressesByImporters, time }
Expand Down
30 changes: 29 additions & 1 deletion pkg-manager/resolve-dependencies/test/hoistPeers.test.ts
Original file line number Diff line number Diff line change
@@ -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: '*' }]], {
Expand All @@ -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',
})
})