Skip to content

Commit 96bdd57

Browse files
DASPRiDclaudezkochan
authored
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>
1 parent 1c05876 commit 96bdd57

4 files changed

Lines changed: 430 additions & 9 deletions

File tree

‎.changeset/dedupe-injected-deps-with-children.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+
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.

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

Lines changed: 206 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,3 +107,209 @@ test('workspace packages should maintain link: protocol after single-project pnp
107107
// when only package 'a' was in the projects list.
108108
expect(lockfileAfterRm.importers.a.dependencies!.b.version).toBe('link:../b')
109109
})
110+
111+
test('workspace packages with their own dependencies should maintain link: protocol after single-project pnpm rm with injectWorkspacePackages', async () => {
112+
const projectAManifest: { name: string, version: string, dependencies: Record<string, string> } = {
113+
name: 'a',
114+
version: '1.0.0',
115+
dependencies: {
116+
'b': 'workspace:*',
117+
'is-positive': '1.0.0',
118+
},
119+
}
120+
const projectBManifest = {
121+
name: 'b',
122+
version: '1.0.0',
123+
dependencies: {
124+
'is-negative': '1.0.0',
125+
},
126+
}
127+
128+
preparePackages([
129+
{
130+
location: 'a',
131+
package: projectAManifest,
132+
},
133+
{
134+
location: 'b',
135+
package: projectBManifest,
136+
},
137+
])
138+
139+
const allProjects: ProjectOptions[] = [
140+
{
141+
buildIndex: 0,
142+
manifest: projectAManifest,
143+
rootDir: path.resolve('a') as ProjectRootDir,
144+
},
145+
{
146+
buildIndex: 0,
147+
manifest: projectBManifest,
148+
rootDir: path.resolve('b') as ProjectRootDir,
149+
},
150+
]
151+
152+
// Initial full install with all projects
153+
await mutateModules([
154+
{
155+
mutation: 'install',
156+
rootDir: path.resolve('a') as ProjectRootDir,
157+
},
158+
{
159+
mutation: 'install',
160+
rootDir: path.resolve('b') as ProjectRootDir,
161+
},
162+
], testDefaults({
163+
allProjects,
164+
injectWorkspacePackages: true,
165+
}))
166+
167+
const rootModules = assertProject(process.cwd())
168+
const lockfile = rootModules.readLockfile()
169+
expect(lockfile.importers.a.dependencies!.b.version).toBe('link:../b')
170+
171+
// Same single-project rm path as the test above, but `b` has its own dependency
172+
// (`is-negative`). The injected file: dep then has children, which hits a separate
173+
// branch in dedupeInjectedDeps.
174+
delete projectAManifest.dependencies['is-positive']
175+
const workspacePackages = new Map([
176+
['a', new Map([
177+
['1.0.0', {
178+
rootDir: path.resolve('a') as ProjectRootDir,
179+
manifest: projectAManifest,
180+
}],
181+
])],
182+
['b', new Map([
183+
['1.0.0', {
184+
rootDir: path.resolve('b') as ProjectRootDir,
185+
manifest: projectBManifest,
186+
}],
187+
])],
188+
])
189+
await mutateModulesInSingleProject(
190+
{
191+
binsDir: path.resolve('a', 'node_modules', '.bin'),
192+
dependencyNames: ['is-positive'],
193+
manifest: projectAManifest,
194+
mutation: 'uninstallSome',
195+
rootDir: path.resolve('a') as ProjectRootDir,
196+
},
197+
testDefaults({
198+
workspacePackages,
199+
injectWorkspacePackages: true,
200+
})
201+
)
202+
203+
const lockfileAfterRm = rootModules.readLockfile()
204+
205+
// Without the fix, dedupeInjectedDeps would skip dedupe when the injected dep had
206+
// children and the target workspace project wasn't in the current resolution, so
207+
// workspace dep 'b' would switch from link: to file:.
208+
expect(lockfileAfterRm.importers.a.dependencies!.b.version).toBe('link:../b')
209+
})
210+
211+
test('peer-resolved workspace packages should keep their file: protocol after single-project pnpm rm with injectWorkspacePackages', async () => {
212+
const projectAManifest: { name: string, version: string, dependencies: Record<string, string> } = {
213+
name: 'a',
214+
version: '1.0.0',
215+
dependencies: {
216+
'b': 'workspace:*',
217+
'is-positive': '1.0.0',
218+
'is-negative': '1.0.0',
219+
},
220+
}
221+
const projectBManifest: { name: string, version: string, dependencies: Record<string, string>, peerDependencies: Record<string, string> } = {
222+
name: 'b',
223+
version: '1.0.0',
224+
dependencies: {},
225+
peerDependencies: {
226+
'is-positive': '>=1.0.0',
227+
},
228+
}
229+
230+
preparePackages([
231+
{
232+
location: 'a',
233+
package: projectAManifest,
234+
},
235+
{
236+
location: 'b',
237+
package: projectBManifest,
238+
},
239+
])
240+
241+
const allProjects: ProjectOptions[] = [
242+
{
243+
buildIndex: 0,
244+
manifest: projectAManifest,
245+
rootDir: path.resolve('a') as ProjectRootDir,
246+
},
247+
{
248+
buildIndex: 0,
249+
manifest: projectBManifest,
250+
rootDir: path.resolve('b') as ProjectRootDir,
251+
},
252+
]
253+
254+
// Initial full install with all projects
255+
await mutateModules([
256+
{
257+
mutation: 'install',
258+
rootDir: path.resolve('a') as ProjectRootDir,
259+
},
260+
{
261+
mutation: 'install',
262+
rootDir: path.resolve('b') as ProjectRootDir,
263+
},
264+
], testDefaults({
265+
allProjects,
266+
injectWorkspacePackages: true,
267+
autoInstallPeers: false,
268+
}))
269+
270+
const rootModules = assertProject(process.cwd())
271+
const lockfile = rootModules.readLockfile()
272+
// With a peer dep on `b`, the injected resolution depends on `a`'s peer context, so the
273+
// entry stays in file: form rather than collapsing to link:../b.
274+
const initialVersion = lockfile.importers.a.dependencies!.b.version
275+
expect(initialVersion).not.toBe('link:../b')
276+
expect(initialVersion.startsWith('file:')).toBe(true)
277+
278+
// Single-project rm of an unrelated dep should preserve the peer-resolved file: form.
279+
delete projectAManifest.dependencies['is-negative']
280+
const workspacePackages = new Map([
281+
['a', new Map([
282+
['1.0.0', {
283+
rootDir: path.resolve('a') as ProjectRootDir,
284+
manifest: projectAManifest,
285+
}],
286+
])],
287+
['b', new Map([
288+
['1.0.0', {
289+
rootDir: path.resolve('b') as ProjectRootDir,
290+
manifest: projectBManifest,
291+
}],
292+
])],
293+
])
294+
await mutateModulesInSingleProject(
295+
{
296+
binsDir: path.resolve('a', 'node_modules', '.bin'),
297+
dependencyNames: ['is-negative'],
298+
manifest: projectAManifest,
299+
mutation: 'uninstallSome',
300+
rootDir: path.resolve('a') as ProjectRootDir,
301+
},
302+
testDefaults({
303+
workspacePackages,
304+
injectWorkspacePackages: true,
305+
autoInstallPeers: false,
306+
})
307+
)
308+
309+
const lockfileAfterRm = rootModules.readLockfile()
310+
311+
// The fast-path must skip dedupe for peer-suffixed depPaths. Without the peer-suffix
312+
// check, dedupeInjectedDeps would collapse the peer-resolved file: entry to link:../b
313+
// and lose the importer's peer context.
314+
expect(lockfileAfterRm.importers.a.dependencies!.b.version).toBe(initialVersion)
315+
})

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

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -61,18 +61,26 @@ function getDedupeMap<T extends PartialResolvedPackage> (
6161
for (const [id, deps] of injectedDepsByProjects.entries()) {
6262
const dedupedInjectedDeps = new Map<string, string>()
6363
for (const [alias, dep] of deps.entries()) {
64-
// Check for subgroup not equal.
65-
// The injected project in the workspace may have dev deps
66-
const children = Object.entries(opts.depGraph[dep.depPath].children)
64+
const node = opts.depGraph[dep.depPath]
6765
const targetProjectDeps = opts.dependenciesByProjectId[dep.id]
68-
// When the target project wasn't part of the current resolution (e.g. single-project
69-
// operation), its dependencies aren't available. We can only deduplicate safely when the
70-
// injected dep has no children (the empty set is always a subset).
66+
// In single-project operations (e.g. `pnpm rm` from inside a workspace package) the target
67+
// workspace project isn't being resolved, so its children aren't in
68+
// `dependenciesByProjectId`. The injected dep was resolved against the same workspace
69+
// package source, so dedupe is safe. The exception is peer-suffixed depPaths, whose
70+
// resolution depends on the importer's peer context. A plain `link:` would lose that, so
71+
// we skip dedupe for those. A depPath is `${pkgIdWithPatchHash}${peerDepGraphHash}`, so it
72+
// carries a peer suffix exactly when it differs from its peer-free `pkgIdWithPatchHash`.
7173
if (!targetProjectDeps) {
72-
if (children.length > 0) continue
74+
if ((node.pkgIdWithPatchHash as string) === dep.depPath) {
75+
dedupedInjectedDeps.set(alias, dep.id)
76+
}
77+
continue
7378
}
79+
// Check for subgroup not equal.
80+
// The injected project in the workspace may have dev deps
81+
const children = Object.entries(node.children)
7482
const isSubset = children
75-
.every(([alias, depPath]) => targetProjectDeps?.get(alias) === depPath)
83+
.every(([alias, depPath]) => targetProjectDeps.get(alias) === depPath)
7684
if (isSubset) {
7785
dedupedInjectedDeps.set(alias, dep.id)
7886
}

0 commit comments

Comments
 (0)