Skip to content

Commit 817f99d

Browse files
aqeelatzkochan
aqeelat
and
zkochan
authored
fix(resolver): stabilize transitivePeerDependencies in dependency cycles (#12286)
## Summary Fixes lockfile churn where a package's `transitivePeerDependencies` (e.g. `supports-color` via `debug`) could be dropped — and shift between packages — when the package participates in a dependency cycle. Which packages carry a given transitive peer depended on resolution order, so upgrading an unrelated dependency churned the lockfile. ## Root cause When peer resolution walks into a cycle, the cycle is broken by dropping the repeated package's subtree, so the re-entry occurrence resolves against truncated children and looks peer-free. That occurrence was then recorded as "pure" in `purePkgs` — a verdict keyed by package id, not by context. A later occurrence of the same package, reached through a different parent that *can* see the full subtree, hit the `purePkgs` short-circuit and returned an empty peer set, dropping the transitive peers it should have surfaced. Because the outcome depends on which occurrence is walked first, it was order-dependent. ## Fix Don't record a cycle re-entry's resolution in `purePkgs` / `peersCache` (a re-entry is detected when the package id already appears in the ancestor chain). Its truncated peer sets aren't authoritative for the package as a whole, so leaving the caches untouched lets later occurrences resolve correctly — or reuse the package's authoritative, non-truncated entry via `findHit`. This is a minimal guard at the cache-population site: it adds no post-resolution pass and does not change `transitivePeerDependencies` for packages that aren't in cycles. This PR also includes an independent fix: when collecting peer providers from a node's children, match each child's resolved package name in addition to its alias, so `pnpm add my-alias@npm:real-pkg` is visible to peer resolution when `real-pkg` is a peer dependency name. Both the TypeScript pnpm CLI and the Rust (pacquet) port are updated in parity. Fixes #5108 Related `transitivePeerDependencies`-instability reports: #5552, #5794, and the `transitivePeerDependencies` aspect of #9992 (the out-of-scope version drift in #9992 is a separate problem and is not addressed here). --------- Co-authored-by: Zoltan Kochan <z@kochan.io>
1 parent 8cec791 commit 817f99d

12 files changed

Lines changed: 529 additions & 7 deletions

File tree

‎.changeset/tpd-propagation-fix.md‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
"@pnpm/installing.deps-resolver": patch
3+
"pnpm": patch
4+
---
5+
6+
Fixed lockfile churn where a package's `transitivePeerDependencies` could be dropped (and shift between packages) when the package participates in a dependency cycle. A cycle re-entry resolves against truncated children, so it must not be cached as "pure"; otherwise sibling occurrences of the same package short-circuit and lose transitive peers depending on traversal order [#5108](https://github.com/pnpm/pnpm/issues/5108).

‎installing/deps-installer/test/install/cyclicPeerDeterminism.ts‎

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -219,3 +219,103 @@ test('aliased install with a transitive mutual-peer cycle should not hang', asyn
219219
const lockfile = rootProject.readLockfile()
220220
expect(lockfile.importers?.['.']?.dependencies?.a?.version).toContain(`${aRealName}@1.0.0`)
221221
})
222+
223+
test('transitivePeerDependencies propagate through regular dep cycles', async () => {
224+
const rootProject = prepareEmpty()
225+
const lockfileDir = rootProject.dir()
226+
227+
const parentName = '@pnpm.e2e/tpd-cycle-parent'
228+
const aName = '@pnpm.e2e/tpd-cycle-a'
229+
const bName = '@pnpm.e2e/tpd-cycle-b'
230+
const cName = '@pnpm.e2e/tpd-cycle-c'
231+
const dName = '@pnpm.e2e/tpd-cycle-d'
232+
const hName = '@pnpm.e2e/tpd-cycle-h'
233+
234+
const manifest: ProjectManifest = {
235+
name: 'root',
236+
dependencies: {
237+
[parentName]: '1.0.0',
238+
},
239+
}
240+
const allProjects: ProjectOptions[] = [{
241+
buildIndex: 0,
242+
manifest,
243+
rootDir: lockfileDir as ProjectRootDir,
244+
}]
245+
const options = {
246+
...testDefaults(
247+
{ allProjects, forceFullResolution: true },
248+
{ retry: { retries: 0 } }
249+
),
250+
lockfileDir,
251+
lockfileOnly: true,
252+
} satisfies MutateModulesOptions
253+
254+
const installProjects: MutatedProject[] = [{
255+
mutation: 'install',
256+
rootDir: lockfileDir as ProjectRootDir,
257+
}]
258+
259+
const registryUrl = options.registries.default.replace(/\/$/, '')
260+
261+
function makeMeta (name: string, deps: Record<string, string>, peerDeps?: Record<string, string>, peerMeta?: Record<string, { optional?: boolean }>): PackageMeta {
262+
return {
263+
name,
264+
versions: {
265+
'1.0.0': {
266+
name,
267+
version: '1.0.0',
268+
dependencies: deps,
269+
...(peerDeps ? { peerDependencies: peerDeps } : {}),
270+
...(peerMeta ? { peerDependenciesMeta: peerMeta } : {}),
271+
dist: {
272+
shasum: '0000000000000000000000000000000000000000',
273+
tarball: `${options.registries.default}/${encodeURIComponent(name)}-1.0.0.tgz`,
274+
},
275+
},
276+
},
277+
'dist-tags': { latest: '1.0.0' },
278+
}
279+
}
280+
281+
const metaByName = {
282+
[parentName]: makeMeta(parentName, { [aName]: '1.0.0', [hName]: '1.0.0' }),
283+
[aName]: makeMeta(aName, { [bName]: '1.0.0', [cName]: '1.0.0' }),
284+
[bName]: makeMeta(bName, { [aName]: '1.0.0' }),
285+
[cName]: makeMeta(cName, { [dName]: '1.0.0' }),
286+
[dName]: makeMeta(dName, {}, { e: '1.0.0' }, { e: { optional: true } }),
287+
[hName]: makeMeta(hName, { [aName]: '1.0.0' }),
288+
}
289+
290+
await setupMockAgent()
291+
const agent = getMockAgent().get(registryUrl)
292+
for (const [name, meta] of Object.entries(metaByName)) {
293+
agent.intercept({ path: `/${name.replaceAll('/', '%2F')}`, method: 'GET' }).reply(200, meta).persist()
294+
}
295+
296+
options.storeController.clearResolutionCache()
297+
await mutateModules(installProjects, options)
298+
299+
const lockfile = rootProject.readLockfile()
300+
301+
const snapshotsWithTpd = Object.entries(lockfile.snapshots ?? {})
302+
.filter(([, snapshot]) => snapshot.transitivePeerDependencies?.includes('e'))
303+
.map(([key]) => key)
304+
305+
expect(snapshotsWithTpd).toContain(`${aName}@1.0.0`)
306+
expect(snapshotsWithTpd).toContain(`${hName}@1.0.0`)
307+
expect(snapshotsWithTpd).toContain(`${parentName}@1.0.0`)
308+
309+
await teardownMockAgent()
310+
311+
await setupMockAgent()
312+
const agent2 = getMockAgent().get(registryUrl)
313+
for (const [name, meta] of Object.entries(metaByName)) {
314+
agent2.intercept({ path: `/${name.replaceAll('/', '%2F')}`, method: 'GET' }).reply(200, meta).persist()
315+
}
316+
options.storeController.clearResolutionCache()
317+
await mutateModules(installProjects, options)
318+
319+
const lockfile2 = rootProject.readLockfile()
320+
expect(Object.keys(lockfile2.snapshots ?? {})).toStrictEqual(Object.keys(lockfile.snapshots ?? {}))
321+
})

‎installing/deps-installer/test/install/peerDependencies.ts‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2269,3 +2269,69 @@ test('no deadlock on circular aliased peers', async () => {
22692269

22702270
expect(fs.existsSync(path.resolve(WANTED_LOCKFILE))).toBeTruthy()
22712271
})
2272+
2273+
test('transitivePeerDependencies does not cause lockfile instability', async () => {
2274+
const project = prepareEmpty()
2275+
await addDistTag({ package: '@pnpm.e2e/transitive-peer-parent', version: '1.0.0', distTag: 'latest' })
2276+
await addDistTag({ package: '@pnpm.e2e/transitive-peer-carrier-a', version: '1.0.0', distTag: 'latest' })
2277+
await addDistTag({ package: '@pnpm.e2e/transitive-peer-carrier-b', version: '1.0.0', distTag: 'latest' })
2278+
await addDistTag({ package: '@pnpm.e2e/uses-optional-peer', version: '1.0.0', distTag: 'latest' })
2279+
await addDistTag({ package: '@pnpm.e2e/has-optional-peer', version: '1.0.0', distTag: 'latest' })
2280+
2281+
const { updatedManifest: manifest } = await addDependenciesToPackage(
2282+
{},
2283+
['@pnpm.e2e/transitive-peer-parent@1.0.0'],
2284+
testDefaults()
2285+
)
2286+
2287+
{
2288+
const lockfile = project.readLockfile()
2289+
expect(lockfile.snapshots['@pnpm.e2e/transitive-peer-carrier-a@1.0.0']?.transitivePeerDependencies).toStrictEqual(['@pnpm.e2e/peer-c'])
2290+
expect(lockfile.snapshots['@pnpm.e2e/transitive-peer-carrier-b@1.0.0']?.transitivePeerDependencies).toStrictEqual(['@pnpm.e2e/peer-c'])
2291+
}
2292+
2293+
await mutateModulesInSingleProject({
2294+
manifest,
2295+
mutation: 'install',
2296+
rootDir: process.cwd() as ProjectRootDir,
2297+
}, testDefaults())
2298+
2299+
const lockfileAfterSecondInstall = project.readLockfile()
2300+
expect(lockfileAfterSecondInstall.snapshots['@pnpm.e2e/transitive-peer-carrier-a@1.0.0']?.transitivePeerDependencies).toStrictEqual(['@pnpm.e2e/peer-c'])
2301+
expect(lockfileAfterSecondInstall.snapshots['@pnpm.e2e/transitive-peer-carrier-b@1.0.0']?.transitivePeerDependencies).toStrictEqual(['@pnpm.e2e/peer-c'])
2302+
})
2303+
2304+
test('adding unrelated dep does not churn transitivePeerDependencies', async () => {
2305+
const project = prepareEmpty()
2306+
await addDistTag({ package: '@pnpm.e2e/transitive-peer-parent', version: '1.0.0', distTag: 'latest' })
2307+
await addDistTag({ package: '@pnpm.e2e/transitive-peer-carrier-a', version: '1.0.0', distTag: 'latest' })
2308+
await addDistTag({ package: '@pnpm.e2e/transitive-peer-carrier-b', version: '1.0.0', distTag: 'latest' })
2309+
await addDistTag({ package: '@pnpm.e2e/uses-optional-peer', version: '1.0.0', distTag: 'latest' })
2310+
await addDistTag({ package: '@pnpm.e2e/has-optional-peer', version: '1.0.0', distTag: 'latest' })
2311+
2312+
const { updatedManifest: manifest } = await addDependenciesToPackage(
2313+
{},
2314+
['@pnpm.e2e/transitive-peer-parent@1.0.0'],
2315+
testDefaults()
2316+
)
2317+
2318+
const lockfileBefore = project.readLockfile()
2319+
const snapshotKeysBefore = Object.keys(lockfileBefore.snapshots).filter((k) => k.includes('transitive-peer-carrier'))
2320+
expect(snapshotKeysBefore).toHaveLength(2)
2321+
for (const key of snapshotKeysBefore) {
2322+
expect(lockfileBefore.snapshots[key]?.transitivePeerDependencies).toStrictEqual(['@pnpm.e2e/peer-c'])
2323+
}
2324+
2325+
const allSnapshotsBefore = { ...lockfileBefore.snapshots }
2326+
2327+
await addDependenciesToPackage(
2328+
manifest,
2329+
['is-negative@1.0.0'],
2330+
testDefaults()
2331+
)
2332+
2333+
const lockfileAfter = project.readLockfile()
2334+
for (const [key, snapshot] of Object.entries(allSnapshotsBefore)) {
2335+
expect(lockfileAfter.snapshots[key]).toStrictEqual(snapshot)
2336+
}
2337+
})

‎installing/deps-resolver/src/resolvePeers.ts‎

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -455,10 +455,11 @@ async function resolvePeersOfNode<T extends PartialResolvedPackage> (
455455
parentPkgs = { ...parentParentPkgs }
456456
const parentPkgNodes: Array<ParentPkgNode<T>> = []
457457
for (const [alias, nodeId] of Object.entries(children)) {
458-
if (ctx.allPeerDepNames.has(alias)) {
458+
const childNode = ctx.dependenciesTree.get(nodeId)!
459+
if (ctx.allPeerDepNames.has(alias) || (alias !== childNode.resolvedPackage.name && ctx.allPeerDepNames.has(childNode.resolvedPackage.name))) {
459460
parentPkgNodes.push({
460461
alias,
461-
node: ctx.dependenciesTree.get(nodeId)!,
462+
node: childNode,
462463
nodeId,
463464
parentNodeIds,
464465
})
@@ -591,7 +592,17 @@ async function resolvePeersOfNode<T extends PartialResolvedPackage> (
591592

592593
let cache: PeersCacheItem
593594
const isPure = allResolvedPeers.size === 0 && allMissingPeers.size === 0
594-
if (isPure) {
595+
// A cycle re-entry resolves against truncated children (the cycle is broken by
596+
// dropping the repeated package's subtree), so its empty/partial peer sets are
597+
// not authoritative for the package as a whole. Recording that verdict — as
598+
// pure or in peersCache — lets it short-circuit other occurrences that *can*
599+
// see the full subtree, dropping their transitivePeerDependencies depending on
600+
// traversal order and churning the lockfile (https://github.com/pnpm/pnpm/issues/5108).
601+
const resolvedThroughCycle = ctx.parentDepPathsChain.includes(resolvedPackage.pkgIdWithPatchHash)
602+
if (resolvedThroughCycle) {
603+
// Leave both caches untouched so later occurrences re-resolve (or hit the
604+
// authoritative entry of the same package) instead of reusing this partial one.
605+
} else if (isPure) {
595606
ctx.purePkgs.add(resolvedPackage.pkgIdWithPatchHash)
596607
} else {
597608
cache = {

0 commit comments

Comments
 (0)