Skip to content

fix: correctly identify workspace packages in all operations#10575

Merged
zkochan merged 8 commits into
pnpm:mainfrom
DASPRiD:fix/workspace-package-identification
Mar 13, 2026
Merged

fix: correctly identify workspace packages in all operations#10575
zkochan merged 8 commits into
pnpm:mainfrom
DASPRiD:fix/workspace-package-identification

Conversation

@DASPRiD

@DASPRiD DASPRiD commented Feb 8, 2026

Copy link
Copy Markdown
Contributor

Issue

When injectWorkspacePackages: true, workspace packages would inconsistently change protocols between link: and file: after operations like pnpm rm.

Root Cause

The dedupeInjectedDeps function checks if a dependency is a workspace package using opts.projects.some((project) => project.id === id). This fails when:

  • Full workspace install: opts.projects includes all packages > deduplication works > uses link: protocol
  • Single package operation (pnpm rm in one package): opts.projects only includes that package > deduplication fails > uses file: protocol

Solution

Pass wantedLockfile.importers keys (which contains ALL workspace packages) to dedupeInjectedDeps instead of relying on opts.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.

@DASPRiD DASPRiD requested a review from zkochan as a code owner February 8, 2026 14:38
Copilot AI review requested due to automatic review settings February 8, 2026 14:38
@welcome

welcome Bot commented Feb 8, 2026

Copy link
Copy Markdown

💖 Thanks for opening this pull request! 💖
Please be patient and we will get back to you as soon as we can.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.importers keys) into the peer-resolution/dedupe flow.
  • Update dedupeInjectedDeps to identify workspace packages using the provided workspace-id list rather than opts.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.

Comment thread pkg-manager/resolve-dependencies/src/dedupeInjectedDeps.ts
Comment thread pkg-manager/resolve-dependencies/src/dedupeInjectedDeps.ts
Comment thread pkg-manager/resolve-dependencies/src/index.ts
zkochan and others added 7 commits March 13, 2026 15:24
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 zkochan merged commit 2fc9139 into pnpm:main Mar 13, 2026
4 checks passed
@welcome

welcome Bot commented Mar 13, 2026

Copy link
Copy Markdown

Congrats on merging your first pull request! 🎉🎉🎉

@DASPRiD DASPRiD deleted the fix/workspace-package-identification branch March 13, 2026 16:33
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pnpm rm in workspace package changes link: to file: with injectWorkspacePackages set to true

3 participants