deps: migrate pnpm internals to v12#10508
Open
zkochan wants to merge 3 commits into
Open
Conversation
7 tasks
PR Summary by QodoMigrate Bit’s pnpm integration to pnpm v12 (N-API) with ESM shims
AI Description
Diagram
High-Level Assessment
Files changed (37)
|
Code Review by Qodo
1. Null refToRelative neighbor id
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
|
Code review by qodo was updated up to the latest commit 4dc7b80 |
|
Code review by qodo was updated up to the latest commit ab13018 |
|
Code review by qodo was updated up to the latest commit 9068b8c |
Code Review by QodoNew Review StartedThis review has been superseded by a new analysis |
|
Code review by qodo was updated up to the latest commit 3d3660f |
# Conflicts: # pnpm-lock.yaml
|
Code review by qodo was updated up to the latest commit aacaa7f |
zkochan
enabled auto-merge (squash)
July 21, 2026 09:01
Comment on lines
78
to
92
| merge(graph: DependenciesGraph): void { | ||
| const directDependencies = graph.findRootEdge()?.neighbours; | ||
| if (directDependencies) { | ||
| const rootEdge = this.findRootEdge(); | ||
| const incomingRootEdge = graph.findRootEdge(); | ||
| const directDependencies = incomingRootEdge?.neighbours; | ||
| if (directDependencies && rootEdge) { | ||
| for (const directDep of directDependencies) { | ||
| const existingDirectDeps = this.findRootEdge()?.neighbours; | ||
| if (existingDirectDeps) { | ||
| const existingDirectDep = existingDirectDeps.find( | ||
| ({ name, specifier }) => name === directDep.name && specifier === directDep.specifier | ||
| ); | ||
| if (existingDirectDep == null) { | ||
| existingDirectDeps.push(directDep); | ||
| } else if (existingDirectDep.id !== directDep.id && nodeIdLessThan(existingDirectDep.id, directDep.id)) { | ||
| existingDirectDep.id = directDep.id; | ||
| } | ||
| const existingDirectDep = rootEdge.neighbours.find((existingDep) => | ||
| isSameDirectDependency(existingDep, directDep) | ||
| ); | ||
| if (existingDirectDep == null) { | ||
| rootEdge.neighbours.push(directDep); | ||
| } else if (existingDirectDep.id !== directDep.id && nodeIdLessThan(existingDirectDep.id, directDep.id)) { | ||
| existingDirectDep.id = directDep.id; | ||
| } | ||
| } |
There was a problem hiding this comment.
1. Wildcard direct deps merged wrong 🐞 Bug ≡ Correctness
DependenciesGraph.merge() can treat two direct deps as the same when either specifier is '*' and then only updates the id, leaving the wildcard specifier in place. convertGraphToLockfile() later matches root direct deps by exact specifier, so merged graphs can fail to emit importer entries for non-wildcard manifest specs, producing an incomplete lockfile.
Agent Prompt
## Issue description
`DependenciesGraph.merge()` merges two root-edge neighbours if `isSameDirectDependency()` returns true. The helper currently considers a wildcard specifier (`*`/empty/undefined) compatible with any specifier, but the merge only updates `existingDirectDep.id` and does not update `existingDirectDep.specifier`.
Downstream, `convertGraphToLockfile()` searches for a root-edge neighbour by `(directDep.name === name && (directDep.specifier === specifier || dp.removeSuffix(directDep.id) === `${name}@${specifier}`))`. If the merged neighbour retained `specifier='*'` while the manifest specifier is `^x.y.z`, the lookup can fail and the importer dependency entry will not be written.
## Issue Context
A minimal failure scenario:
- Existing root neighbour: `{ name: 'foo', specifier: '*', id: 'foo@1.0.0' }`
- Incoming root neighbour: `{ name: 'foo', specifier: '^2.0.0', id: 'foo@2.0.0' }`
After merge, neighbour can become `{ name: 'foo', specifier: '*', id: 'foo@2.0.0' }`.
Then `convertGraphToLockfile()` searching for manifest `foo: '^2.0.0'` won’t match `directDep.specifier === specifier`, and won’t match the `${name}@${specifier}` fallback either.
## Fix Focus Areas
- scopes/scope/objects/models/dependencies-graph.ts[78-102]
- scopes/scope/objects/models/dependencies-graph.ts[260-268]
- scopes/dependencies/pnpm/lockfile-deps-graph-converter.ts[339-364]
## Suggested fix
Option A (recommended): when merging two direct deps that match by wildcard, prefer the more specific specifier:
- If `existing.specifier` is wildcard and `incoming.specifier` is not, set `existing.specifier = incoming.specifier`.
- If both are non-wildcard but differ, keep both entries (don’t treat them as identical).
Option B: tighten `isSameDirectDependency()` so wildcard does not match a concrete specifier (only treat as same when both specifiers are equal or both wildcard).
Add a unit test that merges graphs with wildcard vs non-wildcard specifiers and asserts `convertGraphToLockfile()` writes the importer dependency entry.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit dda3274 |
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
1 participant
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.
Summary
@pnpm/napi@12.0.0-alpha.17and the pnpm v12 package split, including the new@pnpm/<domain>.<leaf>helper packages.@pnpm/config.readerand typed config fields.ERR_PNPM_SPEC_NOT_SUPPORTED_BY_ANY_RESOLVERpath.pull/<id>/merge, bounded GitHub API lookup, and regression coverage for the branch-ref matcher.@pnpm/napi.*native binaries for linux/macos/windows bundles, and rebuild second-arch bundles from a clean install directory.Test plan
bit compile teambit.dependencies/pnpm teambit.dependencies/dependency-resolver teambit.git/cibit test scopes/dependencies/pnpm/lynx.spec.ts scopes/dependencies/pnpm/lockfile-deps-graph-converter.spec.ts scopes/dependencies/dependency-resolver/dependency-resolver.main.runtime.spec.ts— 34/34 passedbit test scopes/git/ci/pull-request-ref.spec.ts— 2/2 passednpm run mocha-circleci -- e2e/harmony/deps-graph.e2e.ts --grep "two components with different peer dependencies( should| importing)"— 7 passingnpm run mocha-circleci -- e2e/harmony/dependencies/never-built-dependencies.e2e.ts --grep "using pnpm"— 1 passingnpm run mocha-circleci -- e2e/harmony/ci-commands.e2e.ts --grep "bit ci pr workflow"— 6 passingnpm run mocha-circleci -- e2e/harmony/root-components.e2e.ts --grep "pnpm hoisted linker"— 12 passing, 2 pendingpnpm@10.14.0hoisted/copy install of@pnpm/napi@12.0.0-alpha.17 --os=linux --cpu=arm64verified the targetpnpm-napi.nodebinary.circleci/config.ymlparsed with the repoyamlpackagenpm run lint-circlebit test scopes/dependencies/pnpm/lynx.spec.ts scopes/dependencies/dependency-resolver/dependency-resolver.main.runtime.spec.ts— 13/13 passed after lint-only fixesReplaces #10293.