fix: resolve peer of peer from the deps of direct dependent package #7606

zkochan · 2024-02-02T01:47:48Z

Peer dependencies of peer dependencies are now resolved correctly. When peer dependencies have peer dependencies of their own, the peer dependencies are grouped with their own peer dependencies before being linked to their dependents.

For instance, if card has react in peer dependencies and react has typescript in its peer dependencies, then the same version of react may be linked from different places if there are multiple versions of typescript. For instance:

project1/package.json
{
  "dependencies": {
    "card": "1.0.0",
    "react": "16.8.0",
    "typescript": "7.0.0"
  }
}
project2/package.json
{
  "dependencies": {
    "card": "1.0.0",
    "react": "16.8.0",
    "typescript": "8.0.0"
  }
}
node_modules
  .pnpm
    card@1.0.0(react@16.8.0(typescript@7.0.0))
      node_modules
        card
        react --> ../../react@16.8.0(typescript@7.0.0)/node_modules/react
    react@16.8.0(typescript@7.0.0)
      node_modules
        react
        typescript --> ../../typescript@7.0.0/node_modules/typescript
    typescript@7.0.0
      node_modules
        typescript
    card@1.0.0(react@16.8.0(typescript@8.0.0))
      node_modules
        card
        react --> ../../react@16.8.0(typescript@8.0.0)/node_modules/react
    react@16.8.0(typescript@8.0.0)
      node_modules
        react
        typescript --> ../../typescript@8.0.0/node_modules/typescript
    typescript@8.0.0
      node_modules
        typescript

In the above example, both projects have card in dependencies but the projects use different versions of typescript. Hence, even though the same version of card is used, card in project1 will reference react from a directory where it is placed with typescript@7.0.0 (because it resolves typescript from the dependencies of project1), while card in project2 will reference react with typescript@8.0.0.

stevenpetryk · 2024-02-07T17:37:37Z

pkg-manager/core/test/install/autoInstallPeers.ts

@@ -143,7 +143,7 @@ test('don\'t install the same missing peer dependency twice', async () => {
  const lockfile = await project.readLockfile()
  expect(Object.keys(lockfile.packages).sort()).toStrictEqual([
    '/@pnpm/y@1.0.0',
-    `/@pnpm.e2e/has-has-y-peer-peer@1.0.0${createPeersFolderSuffix([{ name: '@pnpm/y', version: '1.0.0' }, { name: '@pnpm.e2e/has-y-peer', version: '1.0.0' }])}`,
+    '/@pnpm.e2e/has-has-y-peer-peer@1.0.0(@pnpm.e2e/has-y-peer@1.0.0(@pnpm/y@1.0.0))(@pnpm/y@1.0.0)',


the nested syntax here is pretty interesting—that's new, right? is it representing a small subtree of peer dep resolutions?

Yes, if a peer dependency has peer dependencies of its own, they will be nested. The nesting may be deep if there is a long chain of peer dependency peer dependencies.

stevenpetryk · 2024-02-07T17:38:03Z

pkg-manager/core/test/install/fromRepo.ts

-    '@pnpm.e2e/has-say-hi-peer': '1.0.0(hi@1.0.0)',
+    '@pnpm.e2e/has-say-hi-peer': '1.0.0(github.com/zkochan/hi/4cdebec76b7b9d1f6e219e06c42d92a6b8ea60cd)',


oh awesome, this seems like it would fix #7393 wouldn't it?

I am not sure. It was a byproduct of my changes. I'll be happy if it fixes more issues.

stevenpetryk · 2024-02-07T18:17:11Z

This might be annoying to mention if this PR is still quite early on, but when I try out this branch locally on Discord I get the following error (enabled sourcemaps with pd install):

Stacktrace

ERROR  graph.get is not a function or its return value is not iterable

pnpm: graph.get is not a function or its return value is not iterable
    at findCycle (/Users/steven/src/pnpm/deps/graph-sequencer/src/index.ts:103:30)
    at graphSequencer (/Users/steven/src/pnpm/deps/graph-sequencer/src/index.ts:66:23)
    at resolvePeersOfChildren (/Users/steven/src/pnpm/pkg-manager/resolve-dependencies/src/resolvePeers.ts:605:22)
    at resolvePeers (/Users/steven/src/pnpm/pkg-manager/resolve-dependencies/src/resolvePeers.ts:97:27)
    at resolveDependencies2 (/Users/steven/src/pnpm/pkg-manager/resolve-dependencies/src/index.ts:181:7)
    at _installInContext (/Users/steven/src/pnpm/pkg-manager/core/src/install/index.ts:1001:7)
    at installInContext (/Users/steven/src/pnpm/pkg-manager/core/src/install/index.ts:1399:12)
    at _install (/Users/steven/src/pnpm/pkg-manager/core/src/install/index.ts:679:20)
    at mutateModules (/Users/steven/src/pnpm/pkg-manager/core/src/install/index.ts:274:18)
    at recursive (/Users/steven/src/pnpm/pkg-manager/plugin-commands-installation/src/recursive.ts:273:46)
Progress: resolved 5629, reused 5510, downloaded 0, added 0
TypeError: Cannot read properties of undefined (reading 'promise')
    at <anonymous> (/Users/steven/src/pnpm/pkg-manager/resolve-dependencies/src/resolvePeers.ts:463:58)
    at Array.map (<anonymous>)
    at calculateDepPaths (/Users/steven/src/pnpm/pkg-manager/resolve-dependencies/src/resolvePeers.ts:448:10)
    at <anonymous> (/Users/steven/src/pnpm/pkg-manager/resolve-dependencies/src/resolvePeers.ts:608:53)
    at Array.map (<anonymous>)
    at resolvePeersOfChildren (/Users/steven/src/pnpm/pkg-manager/resolve-dependencies/src/resolvePeers.ts:608:26)
    at resolvePeersOfNode (/Users/steven/src/pnpm/pkg-manager/resolve-dependencies/src/resolvePeers.ts:376:7)
    at resolvePeersOfChildren (/Users/steven/src/pnpm/pkg-manager/resolve-dependencies/src/resolvePeers.ts:590:9)
    at resolvePeersOfNode (/Users/steven/src/pnpm/pkg-manager/resolve-dependencies/src/resolvePeers.ts:376:7)
    at resolvePeersOfChildren (/Users/steven/src/pnpm/pkg-manager/resolve-dependencies/src/resolvePeers.ts:590:9)
TypeError: Cannot read properties of undefined (reading 'promise')
    at <anonymous> (/Users/steven/src/pnpm/pkg-manager/resolve-dependencies/src/resolvePeers.ts:463:58)
    at Array.map (<anonymous>)
    at calculateDepPaths (/Users/steven/src/pnpm/pkg-manager/resolve-dependencies/src/resolvePeers.ts:448:10)
    at <anonymous> (/Users/steven/src/pnpm/pkg-manager/resolve-dependencies/src/resolvePeers.ts:608:53)
    at Array.map (<anonymous>)
    at resolvePeersOfChildren (/Users/steven/src/pnpm/pkg-manager/resolve-dependencies/src/resolvePeers.ts:608:26)
    at resolvePeersOfNode (/Users/steven/src/pnpm/pkg-manager/resolve-dependencies/src/resolvePeers.ts:376:7)
    at resolvePeersOfChildren (/Users/steven/src/pnpm/pkg-manager/resolve-dependencies/src/resolvePeers.ts:590:9)
    at resolvePeersOfNode (/Users/steven/src/pnpm/pkg-manager/resolve-dependencies/src/resolvePeers.ts:376:7)
    at resolvePeersOfChildren (/Users/steven/src/pnpm/pkg-manager/resolve-dependencies/src/resolvePeers.ts:590:9)
TypeError: Cannot read properties of undefined (reading 'promise')
    at <anonymous> (/Users/steven/src/pnpm/pkg-manager/resolve-dependencies/src/resolvePeers.ts:463:58)
    at Array.map (<anonymous>)
    at calculateDepPaths (/Users/steven/src/pnpm/pkg-manager/resolve-dependencies/src/resolvePeers.ts:448:10)
    at <anonymous> (/Users/steven/src/pnpm/pkg-manager/resolve-dependencies/src/resolvePeers.ts:608:53)
    at Array.map (<anonymous>)
    at resolvePeersOfChildren (/Users/steven/src/pnpm/pkg-manager/resolve-dependencies/src/resolvePeers.ts:608:26)
    at resolvePeersOfNode (/Users/steven/src/pnpm/pkg-manager/resolve-dependencies/src/resolvePeers.ts:376:7)
    at resolvePeersOfChildren (/Users/steven/src/pnpm/pkg-manager/resolve-dependencies/src/resolvePeers.ts:590:9)
    at resolvePeers (/Users/steven/src/pnpm/pkg-manager/resolve-dependencies/src/resolvePeers.ts:97:27)
    at resolveDependencies2 (/Users/steven/src/pnpm/pkg-manager/resolve-dependencies/src/index.ts:181:7)
TypeError: Cannot read properties of undefined (reading 'promise')
    at <anonymous> (/Users/steven/src/pnpm/pkg-manager/resolve-dependencies/src/resolvePeers.ts:463:58)
    at Array.map (<anonymous>)
    at calculateDepPaths (/Users/steven/src/pnpm/pkg-manager/resolve-dependencies/src/resolvePeers.ts:448:10)
    at <anonymous> (/Users/steven/src/pnpm/pkg-manager/resolve-dependencies/src/resolvePeers.ts:608:53)
    at Array.map (<anonymous>)
    at resolvePeersOfChildren (/Users/steven/src/pnpm/pkg-manager/resolve-dependencies/src/resolvePeers.ts:608:26)
    at resolvePeersOfNode (/Users/steven/src/pnpm/pkg-manager/resolve-dependencies/src/resolvePeers.ts:376:7)
    at resolvePeersOfChildren (/Users/steven/src/pnpm/pkg-manager/resolve-dependencies/src/resolvePeers.ts:590:9)
    at resolvePeers (/Users/steven/src/pnpm/pkg-manager/resolve-dependencies/src/resolvePeers.ts:97:27)
    at resolveDependencies2 (/Users/steven/src/pnpm/pkg-manager/resolve-dependencies/src/index.ts:181:7)

zkochan · 2024-02-07T20:46:29Z

Actually the PR is ready. I only need to add changesets and a description. So, it is not good that it failed for you. But I think I know how to fix the issue.

zkochan · 2024-02-07T20:49:49Z

This is a breaking change but it is a very important fix, so I will probably add this change to v8 as well (as an opt-in).

This reverts commit 6dc1284.

zkochan · 2024-02-08T16:12:11Z

Steven has found a scenario in which these changes make pnpm hang:

{
  "devDependencies": {
    "webpack-cli": "^5.1.4",
    "webpack-dev-server": "^4.0.0"
  }
}

I'll try to fix this.

This reverts commit ec979f3.

zkochan · 2024-02-08T20:57:24Z

@stevenpetryk you can try it again. I have fixed the issue.

stevenpetryk · 2024-02-08T21:33:55Z

Seems fixed on our end!

jakebailey · 2024-02-08T22:14:29Z

I'm currently running a test locally with this PR for DT, just to see. It's caused a few failures so far, so I need to try and narrow this down to understand what differed. Mainly, it looks like certain packages used to have access to @types/node, but now don't. (And other related errors.)

zkochan · 2024-02-08T22:22:44Z

Before these changes, the resolution algorithm was somewhat undeterministic. There was a possibility that, before the change, a peer dependency would have access to @types/node simply because it was accessible in a different workspace project. Conversely, there were instances where it might not have had access to @types/node, even though it should have.

That might be the cause of the issue.

zkochan · 2024-02-08T22:39:04Z

@jakebailey let me know what you find out and whether we should wait with merging this.

jakebailey · 2024-02-08T23:14:43Z

I haven't had time to analyze it yet, but https://gist.github.com/jakebailey/d4ac5d357dcf84dd6ef8fd265264be00 contains the raw output of running tsc on every DT project when installed using this branch. I suspect that a lot of these are "okay" in that they represent more packages that need a @types/node dep to pull them into the tree (anything that says cannot find type defintion file for node), but the cases where there are now new duplicated identifiers are more concerning.

zkochan · 2024-02-08T23:49:30Z

This will only be released in v9. We will fix any issues that you find before the first stable version.

jakebailey · 2024-02-09T00:19:11Z

Thankfully, most of the errors were my mistake (running the wrong typescript version on typesVersions dirs).

The rest are all places where we could add deps on node/react to fix the problem, which is something we had to do originally when switching to pnpm and can be done here to make tests pass. So, I don't think there are any issues for us.

jakebailey · 2024-02-09T00:20:58Z

I did notice that one of the paths mentioned has an unmatched closing paren:

types/react-native-safari-view/node_modules/.pnpm/react-native@0.73.4_@babel+core@7.23.9_@babel+preset-env@7.23.9_@babel+core@7.23.9)_react@18.2.0/node_modules/react-native/Libraries/Animated/Animated.d.ts

Which may be a bug in this PR.

zkochan · 2024-02-09T00:24:03Z

I see, I already started the release of v9.0.0-alpha.2. We can create an issue about this issue that you have found.

zkochan · 2024-02-13T01:23:51Z

It probably makes sense to bump the lockfile version to v7 with this change. As it appears that pnpm v8 fails with the new nested brackets syntax. I have backported a fix but I don't think it will be enough to avoid confusion.

zkochan added 13 commits February 2, 2024 02:47

fix: resolve peer of peer from the deps of direct dependent package

f27c9f4

test: fix

c0e08d8

test: fix

8589f8f

test: add more tests

188830b

Merge remote-tracking branch 'origin/main' into fix-peers-resolution

2b6febf

fix: wip

3c3fdae

fix: wip

f3ac421

fix: wip

f9fe449

fix: wip

320a869

fix: wip

4ef2fd7

fix: wip

d23a9a8

fix: wip

c47a182

fix: wip

bbf4ab1

zkochan added this to the v9.0 milestone Feb 5, 2024

zkochan added the type: breaking change label Feb 5, 2024

zkochan added 9 commits February 5, 2024 18:02

test: fix

2aa831e

fix: wip

f7d084d

fix: peers resolution

21e28ec

test: fix

f95bf4f

test: fix

7cca596

test: fix

8ed1606

perf: faster peers resolution

81e65e9

refactor: resolve peers

ddba271

test: refactor

9b66d64

stevenpetryk reviewed Feb 7, 2024

View reviewed changes

fix: graph-sequencer

e3ebf15

Revert "Revert "fix: graph-sequencer""

ec979f3

This reverts commit 6dc1284.

zkochan added 3 commits February 8, 2024 21:53

fix: installing deps with circular peers

8015879

Revert "Revert "Revert "fix: graph-sequencer"""

eb5077e

This reverts commit ec979f3.

fix: types

3bd416f

Merge branch 'main' into fix-peers-resolution

835f214

stevenpetryk approved these changes Feb 8, 2024

View reviewed changes

zkochan added 2 commits February 8, 2024 23:03

test: add test to graph-sequencer

23845ca

test: use smaller packages for testing

3da0ad0

zkochan merged commit 98a1266 into main Feb 8, 2024
14 checks passed

zkochan deleted the fix-peers-resolution branch February 8, 2024 23:50

stevenpetryk mentioned this pull request Feb 11, 2024

pnpm randomly chooses between registry version and equivalent fork when resolving workspace dependencies #7393

Closed

4 tasks

This was referenced Feb 13, 2024

fix: do not fail when the same package with peer dependency is installed via different aliases #7648

Merged

feat!: bump lockfile to v7 #7666

Merged

sokcuri mentioned this pull request Sep 9, 2024

Configuration option to specify packages that should only be resolved once #8502

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: resolve peer of peer from the deps of direct dependent package #7606

fix: resolve peer of peer from the deps of direct dependent package #7606

zkochan commented Feb 2, 2024 •

edited

Loading

stevenpetryk Feb 7, 2024

zkochan Feb 7, 2024

stevenpetryk Feb 7, 2024

zkochan Feb 7, 2024

stevenpetryk commented Feb 7, 2024

zkochan commented Feb 7, 2024

zkochan commented Feb 7, 2024

zkochan commented Feb 8, 2024 •

edited

Loading

zkochan commented Feb 8, 2024

stevenpetryk commented Feb 8, 2024

jakebailey commented Feb 8, 2024

zkochan commented Feb 8, 2024 •

edited

Loading

zkochan commented Feb 8, 2024

jakebailey commented Feb 8, 2024

zkochan commented Feb 8, 2024

jakebailey commented Feb 9, 2024

jakebailey commented Feb 9, 2024 •

edited

Loading

zkochan commented Feb 9, 2024

zkochan commented Feb 13, 2024

		'@pnpm.e2e/has-say-hi-peer': '1.0.0(hi@1.0.0)',
		'@pnpm.e2e/has-say-hi-peer': '1.0.0(github.com/zkochan/hi/4cdebec76b7b9d1f6e219e06c42d92a6b8ea60cd)',

fix: resolve peer of peer from the deps of direct dependent package #7606

fix: resolve peer of peer from the deps of direct dependent package #7606

Conversation

zkochan commented Feb 2, 2024 • edited Loading

stevenpetryk Feb 7, 2024

Choose a reason for hiding this comment

zkochan Feb 7, 2024

Choose a reason for hiding this comment

stevenpetryk Feb 7, 2024

Choose a reason for hiding this comment

zkochan Feb 7, 2024

Choose a reason for hiding this comment

stevenpetryk commented Feb 7, 2024

zkochan commented Feb 7, 2024

zkochan commented Feb 7, 2024

zkochan commented Feb 8, 2024 • edited Loading

zkochan commented Feb 8, 2024

stevenpetryk commented Feb 8, 2024

jakebailey commented Feb 8, 2024

zkochan commented Feb 8, 2024 • edited Loading

zkochan commented Feb 8, 2024

jakebailey commented Feb 8, 2024

zkochan commented Feb 8, 2024

jakebailey commented Feb 9, 2024

jakebailey commented Feb 9, 2024 • edited Loading

zkochan commented Feb 9, 2024

zkochan commented Feb 13, 2024

zkochan commented Feb 2, 2024 •

edited

Loading

zkochan commented Feb 8, 2024 •

edited

Loading

zkochan commented Feb 8, 2024 •

edited

Loading

jakebailey commented Feb 9, 2024 •

edited

Loading