-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[Bug?]: Inverse hoisting behaviour #6516
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
This is the preference sorting algorithm: berry/packages/yarnpkg-nm/sources/hoist.ts Lines 296 to 306 in 5ac7395
|
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 |
@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. |
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
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
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
RemarksIn 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. |
Sounds good, please go ahead. |
Do you consider this change to be a patch or a minor change within |
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 <!--- Don't worry if you miss something, chores are automatically tested. --> <!--- This checklist exists to help you remember doing the chores when you submit a PR. --> <!--- Put an `x` in all the boxes that apply. --> - [x] I have read the [Contributing Guide](https://yarnpkg.com/advanced/contributing). <!-- See https://yarnpkg.com/advanced/contributing#preparing-your-pr-to-be-released for more details. --> <!-- Check with `yarn version check` and fix with `yarn version check -i` --> - [x] I have set the packages that need to be released for my changes to be effective. <!-- The "Testing chores" workflow validates that your PR follows our guidelines. --> <!-- If it doesn't pass, click on it to see details as to what your PR might be missing. --> - [x] I will check that all automated PR checks pass before the PR gets reviewed. --------- Co-authored-by: Victor Vlasenko <[email protected]>
Uh oh!
There was an error while loading. Please reload this page.
Self-service
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
Additionally, I like to look at the actual filesystem, so when I ran
find
withjq
I got this: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 packageNow, 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 ouryarnrc
settings, resolves to a pinned10.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 nestedpackages/H/node_modules/@testing-library/dom
with10.4.0
, but the top-level hoisted version will still be8.18.1
.However, this is what happens after upgrading the version:
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
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 inyarn 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, and10.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
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 of10.4.0
should not even matter here (but for some reason it does :/)The text was updated successfully, but these errors were encountered: