fix: correctly identify workspace packages in all operations (#10575)

* fix: correctly identify workspace packages in all operations

* fix: use Set for workspacePackages lookup and add uninstallSome test

Use Set<string> instead of string[] for workspacePackages in
dedupeInjectedDeps for O(1) lookups. Add test covering pnpm rm
(uninstallSome) to directly reproduce the scenario from #9518.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: rename workspacePackages to workspaceProjectIds and add defensive guard

Address Copilot review comments:
- Rename `workspacePackages` to `workspaceProjectIds` to avoid confusion
  with the `WorkspacePackages` type used elsewhere in the codebase.
- Add a defensive guard in `getDedupeMap` to skip deps whose target
  project is not in `dependenciesByProjectId`, preventing a potential
  runtime error if the function is called with a partial project set.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: use allProjectIds from ctx.projects instead of wantedLockfile.importers

wantedLockfile.importers may not always contain all workspace projects
(e.g. when pruneLockfileImporters is true during subset operations).
Pass allProjectIds explicitly from ctx.projects, which is the
authoritative source for all workspace project IDs.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: make allProjectIds a required field

All callers of resolveDependencies now explicitly pass allProjectIds
rather than falling back to wantedLockfile.importers.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: rewrite test to actually reproduce the bug from #9518

The previous tests used mutateModules with all projects in allProjects,
which caused installInContext to expand the projects list — hiding the
bug. The real bug occurs when mutateModulesInSingleProject is used
(as pnpm rm does from a single package directory), where allProjects
contains only the operated-on package.

The new test uses mutateModulesInSingleProject for the removal step,
matching the actual pnpm rm code path. It correctly fails without the
fix (receives "file:b" instead of "link:../b") and passes with it.

Also fixes the workspaceProjectIds source to merge both allProjectIds
and wantedLockfile.importers, since in single-project operations
allProjectIds only has one project while the lockfile has all of them.

Refines the defensive guard in getDedupeMap to allow deduplication when
the target project has no children (empty set is always a subset).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: make workspaceProjectIds required in resolvePeers

Remove the optional marker and empty-set fallback. All callers now
provide this explicitly, and test call sites pass new Set().

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Zoltan Kochan <z@kochan.io>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
Ben Scholzen
2026-03-13 17:04:11 +01:00
committed by GitHub
parent 3417386cb7
commit 2fc913969b
10 changed files with 148 additions and 4 deletions

View File

@@ -0,0 +1,13 @@
---
"@pnpm/resolve-dependencies": patch
"@pnpm/core": patch
"pnpm": patch
---
Fix workspace package protocol consistency when using `injectWorkspacePackages`
Previously, workspace packages would inconsistently switch between `link:` and `file:` protocols after operations like `pnpm rm` when `injectWorkspacePackages` was enabled. The issue was that deduplication logic couldn't identify workspace packages in single-package operation contexts.
This fix ensures workspace packages maintain consistent protocols by checking against all workspace packages from the lockfile, not just packages in the current operation context.
Fixes #9518

View File

@@ -82,6 +82,7 @@ export async function resolveManifestDependencies (
wantedLockfile: emptyLockfile,
workspacePackages: new Map(),
peersSuffixMaxLength: 1000,
allProjectIds: ['.'],
}
)
await waitTillAllFetchingsFinish()

View File

@@ -98,6 +98,7 @@ export async function getPeerDependencyIssues (
workspacePackages: ctx.workspacePackages ?? new Map(),
supportedArchitectures: opts.supportedArchitectures,
peersSuffixMaxLength: opts.peersSuffixMaxLength,
allProjectIds: Object.values(ctx.projects).map((p) => p.id),
}
)

View File

@@ -1283,6 +1283,7 @@ const _installInContext: InstallFunction = async (projects, ctx, opts) => {
trustPolicyExclude: opts.trustPolicyExclude,
trustPolicyIgnoreAfter: opts.trustPolicyIgnoreAfter,
blockExoticSubdeps: opts.blockExoticSubdeps,
allProjectIds: Object.values(ctx.projects).map((p) => p.id),
}
)
if (!opts.include.optionalDependencies || !opts.include.devDependencies || !opts.include.dependencies) {

View File

@@ -0,0 +1,108 @@
import path from 'node:path'
import { assertProject } from '@pnpm/assert-project'
import { mutateModules, mutateModulesInSingleProject, type ProjectOptions } from '@pnpm/core'
import { preparePackages } from '@pnpm/prepare'
import type { ProjectRootDir } from '@pnpm/types'
import { testDefaults } from '../utils/index.js'
test('workspace packages should maintain link: protocol after single-project pnpm rm with injectWorkspacePackages', async () => {
const projectAManifest: { name: string, version: string, dependencies: Record<string, string> } = {
name: 'a',
version: '1.0.0',
dependencies: {
'b': 'workspace:*',
'is-positive': '1.0.0',
},
}
const projectBManifest = {
name: 'b',
version: '1.0.0',
}
preparePackages([
{
location: 'a',
package: projectAManifest,
},
{
location: 'b',
package: projectBManifest,
},
])
const allProjects: ProjectOptions[] = [
{
buildIndex: 0,
manifest: projectAManifest,
rootDir: path.resolve('a') as ProjectRootDir,
},
{
buildIndex: 0,
manifest: projectBManifest,
rootDir: path.resolve('b') as ProjectRootDir,
},
]
// Initial full install with all projects
await mutateModules([
{
mutation: 'install',
rootDir: path.resolve('a') as ProjectRootDir,
},
{
mutation: 'install',
rootDir: path.resolve('b') as ProjectRootDir,
},
], testDefaults({
allProjects,
injectWorkspacePackages: true,
}))
const rootModules = assertProject(process.cwd())
const lockfile = rootModules.readLockfile()
expect(lockfile.importers.a.dependencies!.b.version).toBe('link:../b')
// Remove a dependency using mutateModulesInSingleProject.
// This is the code path used by `pnpm rm` when run from within a single
// workspace package directory. It passes allProjects with only the single
// project, so ctx.projects won't contain the other workspace packages.
// The workspacePackages map must still include all workspace packages
// for resolution to work.
delete projectAManifest.dependencies['is-positive']
const workspacePackages = new Map([
['a', new Map([
['1.0.0', {
rootDir: path.resolve('a') as ProjectRootDir,
manifest: projectAManifest,
}],
])],
['b', new Map([
['1.0.0', {
rootDir: path.resolve('b') as ProjectRootDir,
manifest: projectBManifest,
}],
])],
])
await mutateModulesInSingleProject(
{
binsDir: path.resolve('a', 'node_modules', '.bin'),
dependencyNames: ['is-positive'],
manifest: projectAManifest,
mutation: 'uninstallSome',
rootDir: path.resolve('a') as ProjectRootDir,
},
testDefaults({
workspacePackages,
injectWorkspacePackages: true,
})
)
const lockfileAfterRm = rootModules.readLockfile()
// Without the fix, workspace dep 'b' would switch from link: to file: protocol
// because dedupeInjectedDeps couldn't identify 'b' as a workspace package
// when only package 'a' was in the projects list.
expect(lockfileAfterRm.importers.a.dependencies!.b.version).toBe('link:../b')
})

View File

@@ -20,6 +20,7 @@ export interface DedupeInjectedDepsOptions<T extends PartialResolvedPackage> {
pathsByNodeId: Map<NodeId, DepPath>
projects: ProjectToResolve[]
resolvedImporters: ResolvedImporters
workspaceProjectIds: Set<string>
}
export function dedupeInjectedDeps<T extends PartialResolvedPackage> (
@@ -33,7 +34,7 @@ export function dedupeInjectedDeps<T extends PartialResolvedPackage> (
type InjectedDepsByProjects = Map<string, Map<string, { depPath: DepPath, id: string }>>
function getInjectedDepsByProjects<T extends PartialResolvedPackage> (
opts: Pick<DedupeInjectedDepsOptions<T>, 'projects' | 'pathsByNodeId' | 'depGraph'>
opts: Pick<DedupeInjectedDepsOptions<T>, 'projects' | 'pathsByNodeId' | 'depGraph' | 'workspaceProjectIds'>
): InjectedDepsByProjects {
const injectedDepsByProjects = new Map<string, Map<string, { depPath: DepPath, id: string }>>()
for (const project of opts.projects) {
@@ -41,7 +42,7 @@ function getInjectedDepsByProjects<T extends PartialResolvedPackage> (
const depPath = opts.pathsByNodeId.get(nodeId)!
if (!opts.depGraph[depPath].id.startsWith('file:')) continue
const id = opts.depGraph[depPath].id.substring(5)
if (opts.projects.some((project) => project.id === id)) {
if (opts.workspaceProjectIds.has(id)) {
if (!injectedDepsByProjects.has(project.id)) injectedDepsByProjects.set(project.id, new Map())
injectedDepsByProjects.get(project.id)!.set(alias, { depPath, id })
}
@@ -62,8 +63,16 @@ function getDedupeMap<T extends PartialResolvedPackage> (
for (const [alias, dep] of deps.entries()) {
// Check for subgroup not equal.
// The injected project in the workspace may have dev deps
const isSubset = Object.entries(opts.depGraph[dep.depPath].children)
.every(([alias, depPath]) => opts.dependenciesByProjectId[dep.id].get(alias) === depPath)
const children = Object.entries(opts.depGraph[dep.depPath].children)
const targetProjectDeps = opts.dependenciesByProjectId[dep.id]
// When the target project wasn't part of the current resolution (e.g. single-project
// operation), its dependencies aren't available. We can only deduplicate safely when the
// injected dep has no children (the empty set is always a subset).
if (!targetProjectDeps) {
if (children.length > 0) continue
}
const isSubset = children
.every(([alias, depPath]) => targetProjectDeps?.get(alias) === depPath)
if (isSubset) {
dedupedInjectedDeps.set(alias, dep.id)
}

View File

@@ -124,6 +124,7 @@ export async function resolveDependencies (
lockfileIncludeTarballUrl?: boolean
allowUnusedPatches?: boolean
enableGlobalVirtualStore?: boolean
allProjectIds: string[]
}
): Promise<ResolveDependenciesResult> {
const _toResolveImporter = toResolveImporter.bind(null, {
@@ -216,6 +217,7 @@ export async function resolveDependencies (
resolvePeersFromWorkspaceRoot: Boolean(opts.resolvePeersFromWorkspaceRoot),
resolvedImporters,
peersSuffixMaxLength: opts.peersSuffixMaxLength,
workspaceProjectIds: new Set([...opts.allProjectIds, ...Object.keys(opts.wantedLockfile.importers)]),
})
const linkedDependenciesByProjectId: Record<string, LinkedDependency[]> = {}

View File

@@ -91,6 +91,7 @@ export async function resolvePeers<T extends PartialResolvedPackage> (
dedupeInjectedDeps?: boolean
resolvedImporters: ResolvedImporters
peersSuffixMaxLength: number
workspaceProjectIds: Set<string>
}
): Promise<{
dependenciesGraph: GenericDependenciesGraphWithResolvedChildren<T>
@@ -180,6 +181,7 @@ export async function resolvePeers<T extends PartialResolvedPackage> (
pathsByNodeId,
lockfileDir: opts.lockfileDir,
resolvedImporters: opts.resolvedImporters,
workspaceProjectIds: opts.workspaceProjectIds,
})
}
if (opts.dedupePeerDependents) {

View File

@@ -103,6 +103,7 @@ test('packages are not deduplicated when versions do not match', async () => {
virtualStoreDirMaxLength: 120,
lockfileDir: '',
peersSuffixMaxLength: 1000,
workspaceProjectIds: new Set(),
})
expect(dependenciesByProjectId.project1.get('foo')).toEqual(dependenciesByProjectId.project2.get('foo'))

View File

@@ -112,6 +112,7 @@ test('resolve peer dependencies of cyclic dependencies', async () => {
lockfileDir: '',
virtualStoreDirMaxLength: 120,
peersSuffixMaxLength: 1000,
workspaceProjectIds: new Set(),
})
expect(Object.keys(dependenciesGraph)).toStrictEqual([
'foo/1.0.0',
@@ -216,6 +217,7 @@ test('when a package is referenced twice in the dependencies graph and one of th
virtualStoreDirMaxLength: 120,
lockfileDir: '',
peersSuffixMaxLength: 1000,
workspaceProjectIds: new Set(),
})
expect(Object.keys(dependenciesGraph).sort()).toStrictEqual([
'bar/1.0.0',
@@ -398,6 +400,7 @@ describe('peer dependency issues', () => {
virtualStoreDirMaxLength: 120,
lockfileDir: '',
peersSuffixMaxLength: 1000,
workspaceProjectIds: new Set(),
})).peerDependencyIssuesByProjects
})
it('should find peer dependency conflicts', () => {
@@ -484,6 +487,7 @@ describe('unmet peer dependency issues', () => {
virtualStoreDirMaxLength: 120,
lockfileDir: '',
peersSuffixMaxLength: 1000,
workspaceProjectIds: new Set(),
})).peerDependencyIssuesByProjects
})
it('should not warn when the found package has prerelease version and the wanted range is *', () => {
@@ -557,6 +561,7 @@ describe('unmet peer dependency issue resolved from subdependency', () => {
virtualStoreDirMaxLength: 120,
lockfileDir: '',
peersSuffixMaxLength: 1000,
workspaceProjectIds: new Set(),
})).peerDependencyIssuesByProjects
})
it('should return from where the bad peer dependency is resolved', () => {
@@ -659,6 +664,7 @@ test('resolve peer dependencies with npm aliases', async () => {
virtualStoreDirMaxLength: 120,
lockfileDir: '',
peersSuffixMaxLength: 1000,
workspaceProjectIds: new Set(),
})
expect(Object.keys(dependenciesGraph).sort()).toStrictEqual([
'bar/1.0.0',