[Bug?]: Inverse hoisting behaviour #6516

akwodkiewicz · 2024-09-24T08:39:43Z

Self-service

I'd be willing to implement a fix

Describe the bug

Setting the stage

I have a monorepo with multiple frontend projects, all of them using @testing-library/dom in some version.

Currently, due to various misusages of the library, most of the projects are stuck on @testing-library/[email protected] and cannot be upgraded without additional changes in tests.

When I run yarn info on @testing-library/dom, this is what I see:

yarn info -A -R --dependents @testing-library/dom

├─ @testing-library/dom@npm:10.1.0
│  ├─ Version: 10.1.0
│  │
│  ├─ Dependents
│  │  ├─ @storybook/test@npm:8.2.7
│  │  ├─ @storybook/test@npm:8.2.7 [0469c]
│  │  └─ @testing-library/user-event@npm:14.5.2 [036c2]
│  │
│  └─ Dependencies
│     ├─ @babel/code-frame@npm:^7.10.4 → npm:7.22.10
│     ├─ @babel/runtime@npm:^7.12.5 → npm:7.22.15
│     ├─ @types/aria-query@npm:^5.0.1 → npm:5.0.4
│     ├─ aria-query@npm:5.3.0 → npm:5.3.0
│     ├─ chalk@npm:^4.1.0 → npm:4.1.2
│     ├─ dom-accessibility-api@npm:^0.5.9 → npm:0.5.14
│     ├─ lz-string@npm:^1.5.0 → npm:1.5.0
│     └─ pretty-format@npm:^27.0.2 → npm:27.5.1
│
├─ @testing-library/dom@npm:8.18.1
│  ├─ Version: 8.18.1
│  │
│  ├─ Dependents
│  │  ├─ @acme/A@workspace:packages/A
│  │  ├─ @acme/B@workspace:packages/B [3dfbe]
│  │  ├─ @acme/B@workspace:packages/B [86669]
│  │  ├─ @acme/B@workspace:packages/B
│  │  ├─ @acme/testing-utils@workspace:packages/testing-utils
│  │  ├─ @acme/C@workspace:packages/C
│  │  ├─ @acme/D@workspace:packages/D
│  │  ├─ @acme/E@workspace:packages/E
│  │  ├─ @acme/F@workspace:packages/F
│  │  ├─ @acme/G@workspace:packages/G
│  │  ├─ @acme/H@workspace:packages/H
│  │  ├─ @testing-library/react@npm:13.2.0
│  │  ├─ @testing-library/react@npm:13.4.0
│  │  ├─ @testing-library/react@npm:13.4.0 [192f3]
│  │  ├─ @testing-library/react@npm:16.0.1 [3dfbe]
│  │  ├─ @testing-library/react@npm:13.4.0 [90307]
│  │  ├─ @testing-library/react@npm:13.4.0 [90d28]
│  │  ├─ @testing-library/react@npm:13.2.0 [b1b51]
│  │  ├─ @testing-library/user-event@npm:14.5.2 [3a89e]
│  │  └─ react-select-event@npm:5.5.1
│  │
│  └─ Dependencies
│     ├─ @babel/code-frame@npm:^7.10.4 → npm:7.22.10
│     ├─ @babel/runtime@npm:^7.12.5 → npm:7.22.15
│     ├─ @types/aria-query@npm:^4.2.0 → npm:4.2.2
│     ├─ aria-query@npm:^5.0.0 → npm:5.0.2
│     ├─ chalk@npm:^4.1.0 → npm:4.1.2
│     ├─ dom-accessibility-api@npm:^0.5.9 → npm:0.5.14
│     ├─ lz-string@npm:^1.4.4 → npm:1.5.0
│     └─ pretty-format@npm:^27.0.2 → npm:27.5.1
│
└─ @testing-library/dom@npm:9.2.0
   ├─ Version: 9.2.0
   │
   ├─ Dependents
   │  ├─ @storybook/test@npm:7.6.20
   │  ├─ @testing-library/react@npm:14.2.1
   │  ├─ @testing-library/react@npm:14.2.1 [3a89e]
   │  ├─ @testing-library/react@npm:14.2.1 [4661f]
   │  ├─ @testing-library/react@npm:14.2.1 [93668]
   │  └─ @testing-library/user-event@npm:14.3.0 [f5ed0]
   │
   └─ Dependencies
      ├─ @babel/code-frame@npm:^7.10.4 → npm:7.22.10
      ├─ @babel/runtime@npm:^7.12.5 → npm:7.22.15
      ├─ @types/aria-query@npm:^5.0.1 → npm:5.0.4
      ├─ aria-query@npm:^5.0.0 → npm:5.0.2
      ├─ chalk@npm:^4.1.0 → npm:4.1.2
      ├─ dom-accessibility-api@npm:^0.5.9 → npm:0.5.14
      ├─ lz-string@npm:^1.5.0 → npm:1.5.0
      └─ pretty-format@npm:^27.0.2 → npm:27.5.1

Additionally, I like to look at the actual filesystem, so when I ran find with jq I got this:

find . -regex '.*/node_modules/@testing-library/dom/package.json' \
  -exec jq '.version | { (input_filename): . }' {} +
  
{
  "./node_modules/@testing-library/dom/package.json": "8.18.1"
}
{
  "./node_modules/@storybook/test/node_modules/@testing-library/dom/package.json": "9.2.0"
}
{
  "./node_modules/@storybook/addon-interactions/node_modules/@testing-library/dom/package.json": "10.1.0"
}
{
  "./packages/B/node_modules/@testing-library/react/node_modules/@testing-library/dom/package.json": "9.2.0"
}

So far, so good. Obviously it would be better if I did not have 3 different majors of the library installed, but that's a different topic -- I guess that's the feature of Yarn that I can afford upgrading some deps and don't care about others, as long as it works on paper.

Upgrading `@testing-library/dom` in a single package

Now, let's say there's a team that wants to boost their work and they want to work with the latest version of @testing-library/dom. The team only works on @acme/H, so they won't spend time upgrading the dependencies in other packages.

We run yarn workspace @acme/H add -D @testing-library/dom@latest (that currently, with our yarnrc settings, resolves to a pinned 10.4.0)

Knowing how hoisting works, and given how the whole monorepo uses 8.18.1 explicitly (7 packages, from A to G), I'd expect that @acme/H gets its own nested packages/H/node_modules/@testing-library/dom with 10.4.0, but the top-level hoisted version will still be 8.18.1.

However, this is what happens after upgrading the version:

find . -regex '.*/node_modules/@testing-library/dom/package.json' \
  -exec jq '.version | { (input_filename): . }' {} +

{
  "./packages/G/node_modules/@testing-library/dom/package.json": "8.18.1"
}
{
  "./packages/D/node_modules/@testing-library/dom/package.json": "8.18.1"
}
{
  "./packages/C/node_modules/@testing-library/dom/package.json": "8.18.1"
}
{
  "./packages/F/node_modules/@testing-library/dom/package.json": "8.18.1"
}
{
  "./packages/E/node_modules/@testing-library/dom/package.json": "8.18.1"
}
{
  "./node_modules/react-select-event/node_modules/@testing-library/dom/package.json": "8.18.1"
}
{
  "./node_modules/@testing-library/dom/package.json": "10.4.0"
}
{
  "./node_modules/@testing-library/react/node_modules/@testing-library/dom/package.json": "8.18.1"
}
{
  "./node_modules/@storybook/test/node_modules/@testing-library/dom/package.json": "9.2.0"
}
{
  "./node_modules/@storybook/addon-interactions/node_modules/@testing-library/dom/package.json": "10.1.0"
}
{
  "./packages/testing-utils/node_modules/@testing-library/dom/package.json": "8.18.1"
}
{
  "./packages/B/node_modules/@testing-library/dom/package.json": "8.18.1"
}
{
  "./packages/B/node_modules/@testing-library/react/node_modules/@testing-library/dom/package.json": "9.2.0"
}
{
  "./packages/A/node_modules/@testing-library/dom/package.json": "8.18.1"
}

It is the newest version that gets hoisted.

What I thought would happen

I was under the impression that the package that is referenced the "highest number of times" is the one that gets hoisted. In my case that should've been 8.18.1.

I checked yarn info to make sure that it is actually the case with the references:

yarn info -A -R --dependents @testing-library/dom

├─ @testing-library/dom@npm:10.1.0
│  ├─ Version: 10.1.0
│  │
│  ├─ Dependents
│  │  ├─ @storybook/test@npm:8.2.7
│  │  ├─ @storybook/test@npm:8.2.7 [0469c]
│  │  └─ @testing-library/user-event@npm:14.5.2 [036c2]
│  │
│  └─ Dependencies
│     ├─ @babel/code-frame@npm:^7.10.4 → npm:7.22.10
│     ├─ @babel/runtime@npm:^7.12.5 → npm:7.22.15
│     ├─ @types/aria-query@npm:^5.0.1 → npm:5.0.4
│     ├─ aria-query@npm:5.3.0 → npm:5.3.0
│     ├─ chalk@npm:^4.1.0 → npm:4.1.2
│     ├─ dom-accessibility-api@npm:^0.5.9 → npm:0.5.14
│     ├─ lz-string@npm:^1.5.0 → npm:1.5.0
│     └─ pretty-format@npm:^27.0.2 → npm:27.5.1
│
├─ @testing-library/dom@npm:10.4.0
│  ├─ Version: 10.4.0
│  │
│  ├─ Dependents
│  │  ├─ @acme/H@workspace:packages/H
│  │  ├─ @testing-library/react@npm:16.0.1 [3dfbe]
│  │  └─ @testing-library/user-event@npm:14.5.2 [3dfbe]
│  │
│  └─ Dependencies
│     ├─ @babel/code-frame@npm:^7.10.4 → npm:7.22.10
│     ├─ @babel/runtime@npm:^7.12.5 → npm:7.22.15
│     ├─ @types/aria-query@npm:^5.0.1 → npm:5.0.4
│     ├─ aria-query@npm:5.3.0 → npm:5.3.0
│     ├─ chalk@npm:^4.1.0 → npm:4.1.2
│     ├─ dom-accessibility-api@npm:^0.5.9 → npm:0.5.14
│     ├─ lz-string@npm:^1.5.0 → npm:1.5.0
│     └─ pretty-format@npm:^27.0.2 → npm:27.5.1
│
├─ @testing-library/dom@npm:8.18.1
│  ├─ Version: 8.18.1
│  │
│  ├─ Dependents
│  │  ├─ @acme/A@workspace:packages/A
│  │  ├─ @acme/B@workspace:packages/B [3dfbe]
│  │  ├─ @acme/B@workspace:packages/B [86669]
│  │  ├─ @acme/B@workspace:packages/B
│  │  ├─ @acme/testing-utils@workspace:packages/testing-utils
│  │  ├─ @acme/C@workspace:packages/C
│  │  ├─ @acme/D@workspace:packages/D
│  │  ├─ @acme/E@workspace:packages/E
│  │  ├─ @acme/F@workspace:packages/F
│  │  ├─ @acme/G@workspace:packages/G
│  │  ├─ @testing-library/react@npm:13.2.0
│  │  ├─ @testing-library/react@npm:13.4.0
│  │  ├─ @testing-library/react@npm:13.4.0 [192f3]
│  │  ├─ @testing-library/react@npm:13.4.0 [90307]
│  │  ├─ @testing-library/react@npm:13.4.0 [90d28]
│  │  ├─ @testing-library/react@npm:13.2.0 [b1b51]
│  │  ├─ @testing-library/user-event@npm:14.5.2 [3a89e]
│  │  └─ react-select-event@npm:5.5.1
│  │
│  └─ Dependencies
│     ├─ @babel/code-frame@npm:^7.10.4 → npm:7.22.10
│     ├─ @babel/runtime@npm:^7.12.5 → npm:7.22.15
│     ├─ @types/aria-query@npm:^4.2.0 → npm:4.2.2
│     ├─ aria-query@npm:^5.0.0 → npm:5.0.2
│     ├─ chalk@npm:^4.1.0 → npm:4.1.2
│     ├─ dom-accessibility-api@npm:^0.5.9 → npm:0.5.14
│     ├─ lz-string@npm:^1.4.4 → npm:1.5.0
│     └─ pretty-format@npm:^27.0.2 → npm:27.5.1
│
└─ @testing-library/dom@npm:9.2.0
   ├─ Version: 9.2.0
   │
   ├─ Dependents
   │  ├─ @storybook/test@npm:7.6.20
   │  ├─ @testing-library/react@npm:14.2.1
   │  ├─ @testing-library/react@npm:14.2.1 [3a89e]
   │  ├─ @testing-library/react@npm:14.2.1 [4661f]
   │  ├─ @testing-library/react@npm:14.2.1 [93668]
   │  └─ @testing-library/user-event@npm:14.3.0 [f5ed0]
   │
   └─ Dependencies
      ├─ @babel/code-frame@npm:^7.10.4 → npm:7.22.10
      ├─ @babel/runtime@npm:^7.12.5 → npm:7.22.15
      ├─ @types/aria-query@npm:^5.0.1 → npm:5.0.4
      ├─ aria-query@npm:^5.0.0 → npm:5.0.2
      ├─ chalk@npm:^4.1.0 → npm:4.1.2
      ├─ dom-accessibility-api@npm:^0.5.9 → npm:0.5.14
      ├─ lz-string@npm:^1.5.0 → npm:1.5.0
      └─ pretty-format@npm:^27.0.2 → npm:27.5.1

As you can see, there are a lot more dependents of 8.18.1.

Side-note: I don't understand why there are 2 more dependents (@testing-library/react@npm:16.0.1 [3dfbe] and @testing-library/user-event@npm:14.5.2 [3dfbe]) -- these were not visible in yarn info previously at all. @acme/H is using @testing-library/react@16.

(EDIT: I checked the peer deps of @testing-library/react@16 and it relies on ...dom@^10, so that's why it was not showing up before)

Summary (a.k.a TL;DR)

After installing @testing-library/[email protected] in one of the packages in the monorepo, it got hoisted, forcing Yarn to install 7 copies of @testing-libray/[email protected].

I'd expect version 8.18.1 to be the one that's still hoisted, and 10.4.0 to be the version nested in the dependent package.

To reproduce

I'll try to add some reproduction, once I get a confirmation that the behaviour is unexpected. Maybe this is a known behaviour of Yarn nm hosting algorithm.

Environment

System:
    OS: macOS 14.5
    CPU: (10) arm64 Apple M1 Pro
  Binaries:
    Node: 18.20.4 - /private/var/folders/9v/c77m1k_90wl3225q3pl3335w0000gq/T/xfs-8ff197fa/node
    Yarn: 4.5.0 - /private/var/folders/9v/c77m1k_90wl3225q3pl3335w0000gq/T/xfs-8ff197fa/yarn
    npm: 10.7.0 - /opt/homebrew/opt/nvm/versions/node/v18.20.4/bin/npm
    bun: 1.1.14 - ~/.bun/bin/bun
  npmPackages:
    jest: 29.7.0 => 29.7.0

Additional context

Why do I even care? Well, the obvious thing is the disk usage. But actually, after @testing-library/[email protected] got hoisted to top-level, my tests in @acme/A - @acme/G are now failing due to some timer incompatibilities that were introduced in @testing-library/[email protected]. This is an issue on its own, because I'd imagine that all projects from @acme/A to @acme/G should be using their copy of @testing-library/dom anyway, and the hoisting of 10.4.0 should not even matter here (but for some reason it does :/)

The text was updated successfully, but these errors were encountered:

larixer · 2024-09-24T08:57:54Z

This is the preference sorting algorithm:

berry/packages/yarnpkg-nm/sources/hoist.ts

Lines 296 to 306 in 5ac7395

    
           keyList.sort((key1, key2) => { 
        
             const entry1 = preferenceMap.get(key1)!; 
        
             const entry2 = preferenceMap.get(key2)!; 
        
             if (entry2.hoistPriority !== entry1.hoistPriority) { 
        
               return entry2.hoistPriority - entry1.hoistPriority; 
        
             } else if (entry2.peerDependents.size !== entry1.peerDependents.size) { 
        
               return entry2.peerDependents.size - entry1.peerDependents.size; 
        
             } else { 
        
               return entry2.dependents.size - entry1.dependents.size; 
        
             } 
        
           });

hoistPriority is used for portal and workspace dependencies, so it is not relevant for this case. The debatable part of the algorithm is that peer usages are compared first, maybe it is non-optimal, maybe peer usages and regular usages should instead be added up first and then compared.

akwodkiewicz · 2024-09-24T09:03:27Z

The debatable part of the algorithm is that peer usages are compared first, maybe it is non-optimal, maybe peer usages and regular usages should instead be added up first and then compared.

Oh, that is interesting. Aside from it being non-optimal (which probably is more important to you, as the maintainer), it is definitely not obvious to mere Yarn users, such as myself. Even though I am somehow capable to explain the hoisting mechanism, and I was aware of peer deps taking part in the hoisting algorithm, I wouldn't expect them to have a higher precedence over direct/regular usages.

But again, my personal preference does not matter, what's important is whether it's optimal or not, in terms of the disk usage, I suppose.

@larixer, I can try changing this snippet locally and show you the results, if you think it's worth pursuing

larixer · 2024-09-24T09:08:41Z

@akwodkiewicz As you see peer usages have higher precedence now over direct/regular usages. You can however play with the comparator function locally and figure out which behavior feels more natural and then we can discuss it.

akwodkiewicz · 2024-09-24T09:29:15Z

Original (peer usages first)

 keyList.sort((key1, key2) => { 
   const entry1 = preferenceMap.get(key1)!; 
   const entry2 = preferenceMap.get(key2)!; 
   if (entry2.hoistPriority !== entry1.hoistPriority) { 
     return entry2.hoistPriority - entry1.hoistPriority; 
   } else if (entry2.peerDependents.size !== entry1.peerDependents.size) { 
     return entry2.peerDependents.size - entry1.peerDependents.size; 
   } else { 
     return entry2.dependents.size - entry1.dependents.size; 
   } 
 });

Result

{
  "./packages/G/node_modules/@testing-library/dom/package.json": "8.18.1"
}
{
  "./packages/D/node_modules/@testing-library/dom/package.json": "8.18.1"
}
{
  "./packages/C/node_modules/@testing-library/dom/package.json": "8.18.1"
}
{
  "./packages/F/node_modules/@testing-library/dom/package.json": "8.18.1"
}
{
  "./packages/E/node_modules/@testing-library/dom/package.json": "8.18.1"
}
{
  "./node_modules/react-select-event/node_modules/@testing-library/dom/package.json": "8.18.1"
}
{
  "./node_modules/@testing-library/dom/package.json": "10.4.0"
}
{
  "./node_modules/@testing-library/react/node_modules/@testing-library/dom/package.json": "8.18.1"
}
{
  "./node_modules/@storybook/test/node_modules/@testing-library/dom/package.json": "9.2.0"
}
{
  "./node_modules/@storybook/addon-interactions/node_modules/@testing-library/dom/package.json": "10.1.0"
}
{
  "./packages/testing-utils/node_modules/@testing-library/dom/package.json": "8.18.1"
}
{
  "./packages/B/node_modules/@testing-library/dom/package.json": "8.18.1"
}
{
  "./packages/B/node_modules/@testing-library/react/node_modules/@testing-library/dom/package.json": "9.2.0"
}
{
  "./packages/A/node_modules/@testing-library/dom/package.json": "8.18.1"
}

Direct usages first

  keyList.sort((key1, key2) => {
    const entry1 = preferenceMap.get(key1)!;
    const entry2 = preferenceMap.get(key2)!;
    if (entry2.hoistPriority !== entry1.hoistPriority) {
      return entry2.hoistPriority - entry1.hoistPriority;
    } else if (entry2.dependents.size !== entry1.dependents.size) {
      return entry2.dependents.size - entry1.dependents.size;
    } else {
      return entry2.peerDependents.size - entry1.peerDependents.size;
    }
  });

Result

{
  "./packages/H/node_modules/@testing-library/dom/package.json": "10.4.0"
}
{
  "./node_modules/@testing-library/dom/package.json": "8.18.1"
}
{
  "./node_modules/@storybook/test/node_modules/@testing-library/dom/package.json": "9.2.0"
}
{
  "./node_modules/@storybook/addon-interactions/node_modules/@testing-library/dom/package.json": "10.1.0"
}
{
  "./packages/B/node_modules/@testing-library/react/node_modules/@testing-library/dom/package.json": "9.2.0"
}

Sum of usages

  keyList.sort((key1, key2) => {
    const entry1 = preferenceMap.get(key1)!;
    const entry2 = preferenceMap.get(key2)!;
    if (entry2.hoistPriority !== entry1.hoistPriority) {
      return entry2.hoistPriority - entry1.hoistPriority;
    } else {
      const entry1Usages = entry1.dependents.size + entry1.peerDependents.size;
      const entry2Usages = entry2.dependents.size + entry2.peerDependents.size;
      return entry2Usages - entry1Usages;
    }
  });

Result

{
  "./packages/H/node_modules/@testing-library/dom/package.json": "10.4.0"
}
{
  "./node_modules/@testing-library/dom/package.json": "8.18.1"
}
{
  "./node_modules/@storybook/test/node_modules/@testing-library/dom/package.json": "9.2.0"
}
{
  "./node_modules/@storybook/addon-interactions/node_modules/@testing-library/dom/package.json": "10.1.0"
}
{
  "./packages/B/node_modules/@testing-library/react/node_modules/@testing-library/dom/package.json": "9.2.0"
}

Remarks

In my case both options 2 and 3 give the same results.

Option 3 is probably the most efficient in terms of disk usage, and is the easiest to explain.

Option 2 might help hacking the hoisting mechanism though, in cases where the output is not desired, it's probably easier to modify some of your own manifests to successfully increase the number of direct usages. But if you need to do that, then you probably have some other issues with your setup (like I do, seems from my additional context section).

So, I'd be happy to prepare a PR with the option no. 2 3, the sum of the usages.

larixer · 2024-09-24T09:31:40Z

So, I'd be happy to prepare a PR with the option no. 2 3, the sum of the usages.

Sounds good, please go ahead.

akwodkiewicz · 2024-09-24T09:34:54Z

Do you consider this change to be a patch or a minor change within @yarnpkg/nm? ...or a major, given the drastic changes in outputs in some cases 👀

larixer · 2024-09-24T09:36:57Z

I consider it a patch, because it is a change in heuristics.

…6517) ## What's the problem this PR addresses? Fixes #6516 ## How did you fix it? Simplified the sorting algorithm in `getHoistIndetMap`. ## Checklist    - [x] I have read the [Contributing Guide](https://yarnpkg.com/advanced/contributing).   - [x] I have set the packages that need to be released for my changes to be effective.   - [x] I will check that all automated PR checks pass before the PR gets reviewed. --------- Co-authored-by: Victor Vlasenko <[email protected]>

akwodkiewicz added the bug Something isn't working label Sep 24, 2024

akwodkiewicz mentioned this issue Sep 24, 2024

fix(nm): optimize hoisting by treating peer deps same as other deps #6517

Merged

3 tasks

larixer closed this as completed in #6517 Sep 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Bug?]: Inverse hoisting behaviour #6516

[Bug?]: Inverse hoisting behaviour #6516

akwodkiewicz commented Sep 24, 2024 •

edited

Loading

larixer commented Sep 24, 2024

akwodkiewicz commented Sep 24, 2024

larixer commented Sep 24, 2024

akwodkiewicz commented Sep 24, 2024 •

edited

Loading

larixer commented Sep 24, 2024

akwodkiewicz commented Sep 24, 2024 •

edited

Loading

larixer commented Sep 24, 2024

[Bug?]: Inverse hoisting behaviour #6516

[Bug?]: Inverse hoisting behaviour #6516

Comments

akwodkiewicz commented Sep 24, 2024 • edited Loading

Self-service

Describe the bug

Setting the stage

Upgrading @testing-library/dom in a single package

What I thought would happen

Summary (a.k.a TL;DR)

To reproduce

Environment

Additional context

larixer commented Sep 24, 2024

akwodkiewicz commented Sep 24, 2024

larixer commented Sep 24, 2024

akwodkiewicz commented Sep 24, 2024 • edited Loading

Original (peer usages first)

Result

Direct usages first

Result

Sum of usages

Result

Remarks

larixer commented Sep 24, 2024

akwodkiewicz commented Sep 24, 2024 • edited Loading

larixer commented Sep 24, 2024

akwodkiewicz commented Sep 24, 2024 •

edited

Loading

Upgrading `@testing-library/dom` in a single package

akwodkiewicz commented Sep 24, 2024 •

edited

Loading

akwodkiewicz commented Sep 24, 2024 •

edited

Loading