mirror of
https://github.com/pnpm/pnpm.git
synced 2026-06-28 09:55:39 -04:00
fix: dedupe workspace deps with children in single-project mode (#11448)
## 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) <noreply@anthropic.com> Co-authored-by: Zoltan Kochan <z@kochan.io>
This commit is contained in:
Ben Scholzen
6
.changeset/dedupe-injected-deps-with-children.md
Normal file
6
.changeset/dedupe-injected-deps-with-children.md
Normal file
@@ -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.
|
||||
@@ -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<string, string> } = {
|
||||
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<string, string> } = {
|
||||
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<string, string>, peerDependencies: Record<string, string> } = {
|
||||
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)
|
||||
})
|
||||
|
||||
@@ -61,18 +61,26 @@ function getDedupeMap<T extends PartialResolvedPackage> (
|
||||
for (const [id, deps] of injectedDepsByProjects.entries()) {
|
||||
const dedupedInjectedDeps = new Map<string, string>()
|
||||
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)
|
||||
}
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user