From 2fc913969bb091e011acac77cde4681330e44778 Mon Sep 17 00:00:00 2001 From: Ben Scholzen Date: Fri, 13 Mar 2026 17:04:11 +0100 Subject: [PATCH] fix: correctly identify workspace packages in all operations (#10575) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: correctly identify workspace packages in all operations * fix: use Set for workspacePackages lookup and add uninstallSome test Use Set 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 * 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 * 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 * 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 * 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 * 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 --------- Co-authored-by: Zoltan Kochan Co-authored-by: Claude Opus 4.6 --- .../fix-workspace-protocol-consistency.md | 13 +++ .../src/resolveManifestDependencies.ts | 1 + .../core/src/getPeerDependencyIssues.ts | 1 + pkg-manager/core/src/install/index.ts | 1 + ...injectWorkspacePackagesPersistence.test.ts | 108 ++++++++++++++++++ .../src/dedupeInjectedDeps.ts | 17 ++- pkg-manager/resolve-dependencies/src/index.ts | 2 + .../resolve-dependencies/src/resolvePeers.ts | 2 + .../test/dedupeDepPaths.test.ts | 1 + .../resolve-dependencies/test/resolvePeers.ts | 6 + 10 files changed, 148 insertions(+), 4 deletions(-) create mode 100644 .changeset/fix-workspace-protocol-consistency.md create mode 100644 pkg-manager/core/test/install/injectWorkspacePackagesPersistence.test.ts diff --git a/.changeset/fix-workspace-protocol-consistency.md b/.changeset/fix-workspace-protocol-consistency.md new file mode 100644 index 0000000000..033fe3fdfb --- /dev/null +++ b/.changeset/fix-workspace-protocol-consistency.md @@ -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 diff --git a/config/deps-installer/src/resolveManifestDependencies.ts b/config/deps-installer/src/resolveManifestDependencies.ts index b0ac438eb7..3070ab2fa4 100644 --- a/config/deps-installer/src/resolveManifestDependencies.ts +++ b/config/deps-installer/src/resolveManifestDependencies.ts @@ -82,6 +82,7 @@ export async function resolveManifestDependencies ( wantedLockfile: emptyLockfile, workspacePackages: new Map(), peersSuffixMaxLength: 1000, + allProjectIds: ['.'], } ) await waitTillAllFetchingsFinish() diff --git a/pkg-manager/core/src/getPeerDependencyIssues.ts b/pkg-manager/core/src/getPeerDependencyIssues.ts index a30c374df1..398deef393 100644 --- a/pkg-manager/core/src/getPeerDependencyIssues.ts +++ b/pkg-manager/core/src/getPeerDependencyIssues.ts @@ -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), } ) diff --git a/pkg-manager/core/src/install/index.ts b/pkg-manager/core/src/install/index.ts index b7d573a17c..a273ec60a3 100644 --- a/pkg-manager/core/src/install/index.ts +++ b/pkg-manager/core/src/install/index.ts @@ -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) { diff --git a/pkg-manager/core/test/install/injectWorkspacePackagesPersistence.test.ts b/pkg-manager/core/test/install/injectWorkspacePackagesPersistence.test.ts new file mode 100644 index 0000000000..5e7504155b --- /dev/null +++ b/pkg-manager/core/test/install/injectWorkspacePackagesPersistence.test.ts @@ -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 } = { + 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') +}) diff --git a/pkg-manager/resolve-dependencies/src/dedupeInjectedDeps.ts b/pkg-manager/resolve-dependencies/src/dedupeInjectedDeps.ts index 7c518ddd41..96babd2259 100644 --- a/pkg-manager/resolve-dependencies/src/dedupeInjectedDeps.ts +++ b/pkg-manager/resolve-dependencies/src/dedupeInjectedDeps.ts @@ -20,6 +20,7 @@ export interface DedupeInjectedDepsOptions { pathsByNodeId: Map projects: ProjectToResolve[] resolvedImporters: ResolvedImporters + workspaceProjectIds: Set } export function dedupeInjectedDeps ( @@ -33,7 +34,7 @@ export function dedupeInjectedDeps ( type InjectedDepsByProjects = Map> function getInjectedDepsByProjects ( - opts: Pick, 'projects' | 'pathsByNodeId' | 'depGraph'> + opts: Pick, 'projects' | 'pathsByNodeId' | 'depGraph' | 'workspaceProjectIds'> ): InjectedDepsByProjects { const injectedDepsByProjects = new Map>() for (const project of opts.projects) { @@ -41,7 +42,7 @@ function getInjectedDepsByProjects ( 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 ( 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) } diff --git a/pkg-manager/resolve-dependencies/src/index.ts b/pkg-manager/resolve-dependencies/src/index.ts index 83fddcd0db..c932430745 100644 --- a/pkg-manager/resolve-dependencies/src/index.ts +++ b/pkg-manager/resolve-dependencies/src/index.ts @@ -124,6 +124,7 @@ export async function resolveDependencies ( lockfileIncludeTarballUrl?: boolean allowUnusedPatches?: boolean enableGlobalVirtualStore?: boolean + allProjectIds: string[] } ): Promise { 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 = {} diff --git a/pkg-manager/resolve-dependencies/src/resolvePeers.ts b/pkg-manager/resolve-dependencies/src/resolvePeers.ts index ce9104cf18..4454497a48 100644 --- a/pkg-manager/resolve-dependencies/src/resolvePeers.ts +++ b/pkg-manager/resolve-dependencies/src/resolvePeers.ts @@ -91,6 +91,7 @@ export async function resolvePeers ( dedupeInjectedDeps?: boolean resolvedImporters: ResolvedImporters peersSuffixMaxLength: number + workspaceProjectIds: Set } ): Promise<{ dependenciesGraph: GenericDependenciesGraphWithResolvedChildren @@ -180,6 +181,7 @@ export async function resolvePeers ( pathsByNodeId, lockfileDir: opts.lockfileDir, resolvedImporters: opts.resolvedImporters, + workspaceProjectIds: opts.workspaceProjectIds, }) } if (opts.dedupePeerDependents) { diff --git a/pkg-manager/resolve-dependencies/test/dedupeDepPaths.test.ts b/pkg-manager/resolve-dependencies/test/dedupeDepPaths.test.ts index 402c89432f..ffdf64a282 100644 --- a/pkg-manager/resolve-dependencies/test/dedupeDepPaths.test.ts +++ b/pkg-manager/resolve-dependencies/test/dedupeDepPaths.test.ts @@ -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')) diff --git a/pkg-manager/resolve-dependencies/test/resolvePeers.ts b/pkg-manager/resolve-dependencies/test/resolvePeers.ts index 0e374825f0..032ed8d86d 100644 --- a/pkg-manager/resolve-dependencies/test/resolvePeers.ts +++ b/pkg-manager/resolve-dependencies/test/resolvePeers.ts @@ -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',