Skip to content

Commit d63d411

Browse files
fix(nm): optimize hoisting by treating peer deps same as other deps (#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]>
1 parent 5ac7395 commit d63d411

File tree

4 files changed

+66
-3
lines changed

4 files changed

+66
-3
lines changed

.yarn/versions/a6283714.yml

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
releases:
2+
"@yarnpkg/cli": patch
3+
"@yarnpkg/nm": patch
4+
"@yarnpkg/plugin-nm": patch
5+
"@yarnpkg/pnpify": patch
6+
7+
declined:
8+
- "@yarnpkg/plugin-compat"
9+
- "@yarnpkg/plugin-constraints"
10+
- "@yarnpkg/plugin-dlx"
11+
- "@yarnpkg/plugin-essentials"
12+
- "@yarnpkg/plugin-init"
13+
- "@yarnpkg/plugin-interactive-tools"
14+
- "@yarnpkg/plugin-npm-cli"
15+
- "@yarnpkg/plugin-pack"
16+
- "@yarnpkg/plugin-patch"
17+
- "@yarnpkg/plugin-pnp"
18+
- "@yarnpkg/plugin-pnpm"
19+
- "@yarnpkg/plugin-stage"
20+
- "@yarnpkg/plugin-typescript"
21+
- "@yarnpkg/plugin-version"
22+
- "@yarnpkg/plugin-workspace-tools"
23+
- "@yarnpkg/builder"
24+
- "@yarnpkg/core"
25+
- "@yarnpkg/doctor"

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ Features in `master` can be tried out by running `yarn set version from sources`
1212
- `node-modules` linker now honors user-defined symlinks for `<workspace>/node_modules` directories
1313
- `node-modules` linker supports hoisting into inner workspaces that are parents of other workspaces
1414
- `node-modules` linker attemps to hoist tree more exhaustivel until nothing can be hoisted
15+
- `node-modules` linker uses aggregated count of peer and regular usages to decide hoisting priority, instead of preferring peer usages over regular as before, which should result in fewer duplicates
1516

1617
## 4.1.0
1718

packages/yarnpkg-nm/sources/hoist.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -298,10 +298,10 @@ const getHoistIdentMap = (rootNode: HoisterWorkTree, preferenceMap: PreferenceMa
298298
const entry2 = preferenceMap.get(key2)!;
299299
if (entry2.hoistPriority !== entry1.hoistPriority) {
300300
return entry2.hoistPriority - entry1.hoistPriority;
301-
} else if (entry2.peerDependents.size !== entry1.peerDependents.size) {
302-
return entry2.peerDependents.size - entry1.peerDependents.size;
303301
} else {
304-
return entry2.dependents.size - entry1.dependents.size;
302+
const entry1Usages = entry1.dependents.size + entry1.peerDependents.size;
303+
const entry2Usages = entry2.dependents.size + entry2.peerDependents.size;
304+
return entry2Usages - entry1Usages;
305305
}
306306
});
307307

packages/yarnpkg-nm/tests/hoist.test.ts

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -265,6 +265,43 @@ describe(`hoist`, () => {
265265
expect(getTreeHeight(hoist(toTree(tree), {check: true}))).toEqual(3);
266266
});
267267

268+
it(`should honor package popularity considering number of all references over number of references by peers`, () => {
269+
// . -> A -> Z@X
270+
// -> B -> Z@X
271+
// -> C -> Z@X
272+
// -> D -> Z@Y
273+
// -> U -> Z@Y
274+
// should be hoisted to:
275+
// . -> A
276+
// -> B
277+
// -> C
278+
// -> D -> U
279+
// -> Z@Y
280+
// -> Z@X
281+
const tree = toTree({
282+
'.': {dependencies: [`A`, `B`, `C`, `D`]},
283+
A: {dependencies: [`Z@X`]},
284+
B: {dependencies: [`Z@X`]},
285+
C: {dependencies: [`Z@X`]},
286+
D: {dependencies: [`Z@Y`, `U`]},
287+
U: {dependencies: [`Z@Y`], peerNames: [`Z`]},
288+
});
289+
const result = hoist(tree, {check: true});
290+
expect(getTreeHeight(result)).toEqual(3);
291+
292+
const topLevelDeps = [...result.dependencies];
293+
const hoistedZ = topLevelDeps.find(x => x.name === `Z`)!;
294+
expect(hoistedZ.references).toContain(`X`);
295+
expect(hoistedZ.references).not.toContain(`Y`);
296+
297+
const D = topLevelDeps.find(x => x.name === `D`)!;
298+
const dDeps = [...D.dependencies];
299+
expect(dDeps.length).toEqual(2);
300+
const nestedZ = dDeps.find(x => x.name === `Z`)!;
301+
expect(nestedZ.references).not.toContain(`X`);
302+
expect(nestedZ.references).toContain(`Y`);
303+
});
304+
268305
it(`should hoist dependencies after hoisting peer dep`, () => {
269306
// . -> A -> B --> D@X
270307
// -> D@X

0 commit comments

Comments
 (0)