From 96bdd57bf4cfceea0fd88952bef00fc1adf6333f Mon Sep 17 00:00:00 2001 From: Ben Scholzen Date: Wed, 17 Jun 2026 13:37:09 +0200 Subject: [PATCH] fix: dedupe workspace deps with children in single-project mode (#11448) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Issue When `injectWorkspacePackages: true` is set and a workspace package depends on another workspace package that has its own dependencies, running `pnpm rm` from inside the dependent package's directory switches the lockfile protocol from `link:` to `file:`. Reproduction (workspace where `a` depends on workspace `b`, and `b` has any dependency of its own): ``` cd packages/a pnpm add redis pnpm rm redis # pnpm-lock.yaml: a's "b" entry switched from link:../b to file:packages/b ``` ## Root Cause The fix in #10575 added a defensive guard in `dedupeInjectedDeps` that skipped deduplication whenever the target workspace project's children weren't in `dependenciesByProjectId`: ```ts if (!targetProjectDeps) { if (children.length > 0) continue } ``` In single-project operations (`mutateModulesInSingleProject`, used by `pnpm rm` from inside a package directory) only the operated-on project is resolved. `dependenciesByProjectId` then only has that one project, so the guard fires for any workspace dependency whose target has children, and the protocol stays `file:`. ## Solution In single-project mode the injected dep is resolved against the same workspace package source, so dedupe is safe — *except* for peer-suffixed depPaths, whose resolution depends on the importer's peer context (a plain `link:` would lose it). The new code dedupes whenever `targetProjectDeps` is missing for a known workspace project and the depPath has no peer suffix. The peer-suffix check compares the depPath against its peer-free `pkgIdWithPatchHash` (depPaths are built as `${pkgIdWithPatchHash}${peerDepGraphHash}`), so it's exact rather than a `(`-substring heuristic. --------- Co-authored-by: Claude Opus 4.7 (1M context) Co-authored-by: Zoltan Kochan --- .../dedupe-injected-deps-with-children.md | 6 + ...injectWorkspacePackagesPersistence.test.ts | 206 ++++++++++++++++++ .../deps-resolver/src/dedupeInjectedDeps.ts | 26 ++- .../crates/cli/tests/dedupe_injected_deps.rs | 203 ++++++++++++++++- 4 files changed, 431 insertions(+), 10 deletions(-) create mode 100644 .changeset/dedupe-injected-deps-with-children.md diff --git a/.changeset/dedupe-injected-deps-with-children.md b/.changeset/dedupe-injected-deps-with-children.md new file mode 100644 index 0000000000..de9f9c9335 --- /dev/null +++ b/.changeset/dedupe-injected-deps-with-children.md @@ -0,0 +1,6 @@ +--- +"@pnpm/installing.deps-resolver": patch +"pnpm": patch +--- + +Fix `link:` workspace protocol switching to `file:` after `pnpm rm` is run from inside a workspace package whose target workspace dependency has its own dependencies, when `injectWorkspacePackages: true` is set. Follow-up to [#10575](https://github.com/pnpm/pnpm/pull/10575), which fixed the same symptom for workspace packages without dependencies. diff --git a/installing/deps-installer/test/install/injectWorkspacePackagesPersistence.test.ts b/installing/deps-installer/test/install/injectWorkspacePackagesPersistence.test.ts index 621ab86654..f17b8c5ead 100644 --- a/installing/deps-installer/test/install/injectWorkspacePackagesPersistence.test.ts +++ b/installing/deps-installer/test/install/injectWorkspacePackagesPersistence.test.ts @@ -107,3 +107,209 @@ test('workspace packages should maintain link: protocol after single-project pnp // when only package 'a' was in the projects list. expect(lockfileAfterRm.importers.a.dependencies!.b.version).toBe('link:../b') }) + +test('workspace packages with their own dependencies 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', + dependencies: { + 'is-negative': '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') + + // Same single-project rm path as the test above, but `b` has its own dependency + // (`is-negative`). The injected file: dep then has children, which hits a separate + // branch in dedupeInjectedDeps. + 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, dedupeInjectedDeps would skip dedupe when the injected dep had + // children and the target workspace project wasn't in the current resolution, so + // workspace dep 'b' would switch from link: to file:. + expect(lockfileAfterRm.importers.a.dependencies!.b.version).toBe('link:../b') +}) + +test('peer-resolved workspace packages should keep their file: 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', + 'is-negative': '1.0.0', + }, + } + const projectBManifest: { name: string, version: string, dependencies: Record, peerDependencies: Record } = { + name: 'b', + version: '1.0.0', + dependencies: {}, + peerDependencies: { + 'is-positive': '>=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, + autoInstallPeers: false, + })) + + const rootModules = assertProject(process.cwd()) + const lockfile = rootModules.readLockfile() + // With a peer dep on `b`, the injected resolution depends on `a`'s peer context, so the + // entry stays in file: form rather than collapsing to link:../b. + const initialVersion = lockfile.importers.a.dependencies!.b.version + expect(initialVersion).not.toBe('link:../b') + expect(initialVersion.startsWith('file:')).toBe(true) + + // Single-project rm of an unrelated dep should preserve the peer-resolved file: form. + delete projectAManifest.dependencies['is-negative'] + 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-negative'], + manifest: projectAManifest, + mutation: 'uninstallSome', + rootDir: path.resolve('a') as ProjectRootDir, + }, + testDefaults({ + workspacePackages, + injectWorkspacePackages: true, + autoInstallPeers: false, + }) + ) + + const lockfileAfterRm = rootModules.readLockfile() + + // The fast-path must skip dedupe for peer-suffixed depPaths. Without the peer-suffix + // check, dedupeInjectedDeps would collapse the peer-resolved file: entry to link:../b + // and lose the importer's peer context. + expect(lockfileAfterRm.importers.a.dependencies!.b.version).toBe(initialVersion) +}) diff --git a/installing/deps-resolver/src/dedupeInjectedDeps.ts b/installing/deps-resolver/src/dedupeInjectedDeps.ts index 96babd2259..a5eb4c153c 100644 --- a/installing/deps-resolver/src/dedupeInjectedDeps.ts +++ b/installing/deps-resolver/src/dedupeInjectedDeps.ts @@ -61,18 +61,26 @@ function getDedupeMap ( for (const [id, deps] of injectedDepsByProjects.entries()) { const dedupedInjectedDeps = new Map() for (const [alias, dep] of deps.entries()) { + const node = opts.depGraph[dep.depPath] + const targetProjectDeps = opts.dependenciesByProjectId[dep.id] + // In single-project operations (e.g. `pnpm rm` from inside a workspace package) the target + // workspace project isn't being resolved, so its children aren't in + // `dependenciesByProjectId`. The injected dep was resolved against the same workspace + // package source, so dedupe is safe. The exception is peer-suffixed depPaths, whose + // resolution depends on the importer's peer context. A plain `link:` would lose that, so + // we skip dedupe for those. A depPath is `${pkgIdWithPatchHash}${peerDepGraphHash}`, so it + // carries a peer suffix exactly when it differs from its peer-free `pkgIdWithPatchHash`. + if (!targetProjectDeps) { + if ((node.pkgIdWithPatchHash as string) === dep.depPath) { + dedupedInjectedDeps.set(alias, dep.id) + } + continue + } // Check for subgroup not equal. // The injected project in the workspace may have dev deps - 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 children = Object.entries(node.children) const isSubset = children - .every(([alias, depPath]) => targetProjectDeps?.get(alias) === depPath) + .every(([alias, depPath]) => targetProjectDeps.get(alias) === depPath) if (isSubset) { dedupedInjectedDeps.set(alias, dep.id) } diff --git a/pacquet/crates/cli/tests/dedupe_injected_deps.rs b/pacquet/crates/cli/tests/dedupe_injected_deps.rs index 87149425fc..961f989c98 100644 --- a/pacquet/crates/cli/tests/dedupe_injected_deps.rs +++ b/pacquet/crates/cli/tests/dedupe_injected_deps.rs @@ -17,7 +17,7 @@ use pacquet_testing_utils::{ bin::{AddMockedRegistry, CommandTempCwd}, fs::is_symlink_or_junction, }; -use std::fs; +use std::{fs, process::Command}; /// Two-project workspace where `a` injects leaf `b`. With the default /// `dedupeInjectedDeps: true`, the install pass rewrites `a`'s direct @@ -86,6 +86,207 @@ fn injected_leaf_workspace_dep_is_deduped_to_link() { drop((root, mock_instance)); } +/// `injectWorkspacePackages: true` with a workspace dep (`b`) that has +/// its own dependency (`@pnpm.e2e/foo`), so the injected snapshot has a +/// non-empty child set. The dedupe pass must still collapse `a`'s +/// `file:packages/b` to `link:../b` through the children-subset branch +/// (not just the vacuous empty-children case the leaf test covers), and +/// the `link:` must survive a `remove` that re-resolves the whole +/// workspace. Behavioral analog of pnpm/pnpm#11448, whose single-project +/// `pnpm rm` regression switched such a dep from `link:` to `file:`. +/// Pacquet always re-resolves the full workspace (no single-project +/// mode), so the same resolve path backs both `install` and `remove`. +#[test] +fn injected_workspace_dep_with_children_stays_link_after_remove() { + let CommandTempCwd { pacquet, root, workspace, npmrc_info, .. } = + CommandTempCwd::init().add_mocked_registry(); + let AddMockedRegistry { mock_instance, .. } = npmrc_info; + + fs::write( + workspace.join("package.json"), + serde_json::json!({ + "name": "ws-root", + "version": "0.0.0", + "private": true, + "dependencies": { "@pnpm.e2e/bar": "100.0.0" }, + }) + .to_string(), + ) + .expect("write root package.json"); + + let workspace_yaml_path = workspace.join("pnpm-workspace.yaml"); + let mut workspace_yaml = + fs::read_to_string(&workspace_yaml_path).expect("read pnpm-workspace.yaml"); + if !workspace_yaml.ends_with('\n') { + workspace_yaml.push('\n'); + } + workspace_yaml.push_str("packages:\n - 'packages/*'\ninjectWorkspacePackages: true\n"); + fs::write(&workspace_yaml_path, workspace_yaml).expect("write pnpm-workspace.yaml"); + + fs::create_dir_all(workspace.join("packages/a")).expect("mkdir packages/a"); + fs::write( + workspace.join("packages/a/package.json"), + serde_json::json!({ + "name": "a", + "version": "1.0.0", + "dependencies": { "b": "workspace:*" }, + }) + .to_string(), + ) + .expect("write packages/a/package.json"); + + fs::create_dir_all(workspace.join("packages/b")).expect("mkdir packages/b"); + fs::write( + workspace.join("packages/b/package.json"), + serde_json::json!({ + "name": "b", + "version": "1.0.0", + "dependencies": { "@pnpm.e2e/foo": "100.0.0" }, + }) + .to_string(), + ) + .expect("write packages/b/package.json"); + + pacquet.with_arg("install").assert().success(); + + let lockfile_path = workspace.join("pnpm-lock.yaml"); + let lockfile = fs::read_to_string(&lockfile_path).expect("read pnpm-lock.yaml"); + assert!( + lockfile.contains("link:../b"), + "injectWorkspacePackages should dedupe b (which has its own dependency) to link:../b:\n{lockfile}", + ); + assert!( + !lockfile.contains("file:packages/b"), + "an injected workspace dep with children must not stay file:packages/b after dedupe:\n{lockfile}", + ); + assert!( + lockfile.contains("@pnpm.e2e/foo"), + "b's own dependency should be resolved, proving the injected snapshot has children:\n{lockfile}", + ); + + // Removing an unrelated root dependency re-resolves the whole + // workspace; the injected-with-children dedupe must hold across it. + Command::cargo_bin("pacquet") + .expect("find the pacquet binary") + .with_current_dir(&workspace) + .with_arg("remove") + .with_arg("@pnpm.e2e/bar") + .assert() + .success(); + + let lockfile = fs::read_to_string(&lockfile_path).expect("read pnpm-lock.yaml after remove"); + assert!( + lockfile.contains("link:../b"), + "b must stay link:../b after remove re-resolves the workspace:\n{lockfile}", + ); + assert!( + !lockfile.contains("file:packages/b"), + "remove must not switch the injected workspace dep back to file:packages/b:\n{lockfile}", + ); + + drop((root, mock_instance)); +} + +/// `injectWorkspacePackages: true` where workspace dep `b` declares a +/// peer dependency (`@pnpm.e2e/foo`) that `a` provides. `b`'s injected +/// resolution then depends on `a`'s peer context, so its importer entry +/// is the peer-suffixed `file:packages/b(@pnpm.e2e/foo@100.0.0)` and +/// must *not* collapse to `link:../b` — a plain link would strip the +/// peer context and change the trust-relevant lockfile identity. The +/// peer-suffixed form must also survive a `remove` that re-resolves the +/// whole workspace. Behavioral analog of the peer-suffixed single-project +/// `pnpm rm` case in pnpm/pnpm#11448; the dedupe pass keeps such an entry +/// in `file:` form because its children (the resolved peer) are not a +/// subset of `b`'s own direct deps. +#[test] +fn injected_peer_suffixed_workspace_dep_stays_file_after_remove() { + let CommandTempCwd { pacquet, root, workspace, npmrc_info, .. } = + CommandTempCwd::init().add_mocked_registry(); + let AddMockedRegistry { mock_instance, .. } = npmrc_info; + + fs::write( + workspace.join("package.json"), + serde_json::json!({ + "name": "ws-root", + "version": "0.0.0", + "private": true, + "dependencies": { "@pnpm.e2e/bar": "100.0.0" }, + }) + .to_string(), + ) + .expect("write root package.json"); + + let workspace_yaml_path = workspace.join("pnpm-workspace.yaml"); + let mut workspace_yaml = + fs::read_to_string(&workspace_yaml_path).expect("read pnpm-workspace.yaml"); + if !workspace_yaml.ends_with('\n') { + workspace_yaml.push('\n'); + } + workspace_yaml.push_str( + "packages:\n - 'packages/*'\ninjectWorkspacePackages: true\nautoInstallPeers: false\n", + ); + fs::write(&workspace_yaml_path, workspace_yaml).expect("write pnpm-workspace.yaml"); + + fs::create_dir_all(workspace.join("packages/a")).expect("mkdir packages/a"); + fs::write( + workspace.join("packages/a/package.json"), + serde_json::json!({ + "name": "a", + "version": "1.0.0", + "dependencies": { "b": "workspace:*", "@pnpm.e2e/foo": "100.0.0" }, + }) + .to_string(), + ) + .expect("write packages/a/package.json"); + + fs::create_dir_all(workspace.join("packages/b")).expect("mkdir packages/b"); + fs::write( + workspace.join("packages/b/package.json"), + serde_json::json!({ + "name": "b", + "version": "1.0.0", + "peerDependencies": { "@pnpm.e2e/foo": ">=100.0.0" }, + }) + .to_string(), + ) + .expect("write packages/b/package.json"); + + pacquet.with_arg("install").assert().success(); + + let lockfile_path = workspace.join("pnpm-lock.yaml"); + let lockfile = fs::read_to_string(&lockfile_path).expect("read pnpm-lock.yaml"); + assert!( + lockfile.contains("file:packages/b(@pnpm.e2e/foo@100.0.0)"), + "a peer-resolved injected dep must stay in its peer-suffixed file: form, not collapse to a plain link::\n{lockfile}", + ); + assert!( + !lockfile.contains("link:../b"), + "the peer-suffixed injected dep must not be deduped to link:../b — that would strip b's peer context:\n{lockfile}", + ); + + // Removing an unrelated root dependency re-resolves the whole + // workspace; the peer-suffixed file: entry must survive intact. + Command::cargo_bin("pacquet") + .expect("find the pacquet binary") + .with_current_dir(&workspace) + .with_arg("remove") + .with_arg("@pnpm.e2e/bar") + .assert() + .success(); + + let lockfile = fs::read_to_string(&lockfile_path).expect("read pnpm-lock.yaml after remove"); + assert!( + lockfile.contains("file:packages/b(@pnpm.e2e/foo@100.0.0)"), + "the peer-suffixed file: entry must be preserved byte-for-byte after remove:\n{lockfile}", + ); + assert!( + !lockfile.contains("link:../b"), + "remove must not collapse the peer-suffixed injected dep to link:../b:\n{lockfile}", + ); + + drop((root, mock_instance)); +} + /// Same fixture as the dedupe-true test but with /// `dedupeInjectedDeps: false` in `pnpm-workspace.yaml`: the injected /// snapshot stays as `file:packages/b` in the importer entry, the