fix: correctly identify workspace packages in all operations#10575
Merged
Conversation
|
💖 Thanks for opening this pull request! 💖 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to keep workspace dependency protocols stable when injectWorkspacePackages: true, fixing a case where single-package operations (e.g. pnpm rm run in one workspace package) could cause dependencies to flip between link: and file: in the lockfile.
Changes:
- Thread a list of all workspace importer ids (from
wantedLockfile.importerskeys) into the peer-resolution/dedupe flow. - Update
dedupeInjectedDepsto identify workspace packages using the provided workspace-id list rather thanopts.projects. - Add a regression test to ensure the protocol stays consistent across repeated operations, plus a changeset entry.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg-manager/resolve-dependencies/src/resolvePeers.ts | Adds workspacePackages option and forwards it into injected-deps dedupe. |
| pkg-manager/resolve-dependencies/src/index.ts | Passes Object.keys(wantedLockfile.importers) into resolvePeers. |
| pkg-manager/resolve-dependencies/src/dedupeInjectedDeps.ts | Switches workspace-package detection from opts.projects to the new list. |
| pkg-manager/core/test/install/injectWorkspacePackagesPersistence.test.ts | Adds a regression test for protocol stability with injectWorkspacePackages. |
| .changeset/fix-workspace-protocol-consistency.md | Declares patch releases for impacted packages. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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 pnpm#9518. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…e 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>
…porters 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>
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>
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>
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>
zkochan
approved these changes
Mar 13, 2026
|
Congrats on merging your first pull request! 🎉🎉🎉 |
zkochan
pushed a commit
to DASPRiD/pnpm
that referenced
this pull request
Jun 17, 2026
Removes the defensive guard added in pnpm#10575 that skipped deduplication whenever the target workspace project's children weren't available in the current resolution. In single-project operations the injected dep is resolved against the same workspace package source, so dedupe is safe and the link: protocol is preserved. Adds a test case where the workspace dep target has its own children, reproducing the bug the original PR was meant to fix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
zkochan
pushed a commit
to DASPRiD/pnpm
that referenced
this pull request
Jun 17, 2026
Removes the defensive guard added in pnpm#10575 that skipped deduplication whenever the target workspace project's children weren't available in the current resolution. In single-project operations the injected dep is resolved against the same workspace package source, so dedupe is safe and the link: protocol is preserved. Adds a test case where the workspace dep target has its own children, reproducing the bug the original PR was meant to fix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
zkochan
added a commit
that referenced
this pull request
Jun 17, 2026
## 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 file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue
When
injectWorkspacePackages: true, workspace packages would inconsistently change protocols betweenlink:andfile:after operations likepnpm rm.Root Cause
The
dedupeInjectedDepsfunction checks if a dependency is a workspace package usingopts.projects.some((project) => project.id === id). This fails when:opts.projectsincludes all packages > deduplication works > useslink:protocolpnpm rmin one package):opts.projectsonly includes that package > deduplication fails > usesfile:protocolSolution
Pass
wantedLockfile.importerskeys (which contains ALL workspace packages) todedupeInjectedDepsinstead of relying onopts.projects. This ensures workspace packages are correctly identified in all operation contexts.This fixes #9518
Disclaimer: I used Copilot to identify and fix the root cause.