fix(nm): improves external soft links hoisting (#4269)

* Improves portal hoisting

* Move hasUnhoistedDependencies into a separate function

* Updates test, removes debugging code

* Adds support and a test for deep nested portals hoisting

* Fixes test description

* Lint

* Update packages/yarnpkg-nm/sources/hoist.ts

Co-authored-by: Trivikram Kamat <[email protected]>

* Update packages/yarnpkg-nm/sources/hoist.ts

Co-authored-by: Trivikram Kamat <[email protected]>

* Lint

Co-authored-by: Trivikram Kamat <[email protected]>

Author: 2 people

.yarn/versions/567d3c84.yml

@@ -0,0 +1,25 @@
+releases:
+  "@yarnpkg/cli": patch
+  "@yarnpkg/nm": patch
+  "@yarnpkg/plugin-nm": patch
+  "@yarnpkg/pnpify": patch
+
+declined:
+  - "@yarnpkg/plugin-compat"
+  - "@yarnpkg/plugin-constraints"
+  - "@yarnpkg/plugin-dlx"
+  - "@yarnpkg/plugin-essentials"
+  - "@yarnpkg/plugin-init"
+  - "@yarnpkg/plugin-interactive-tools"
+  - "@yarnpkg/plugin-npm-cli"
+  - "@yarnpkg/plugin-pack"
+  - "@yarnpkg/plugin-patch"
+  - "@yarnpkg/plugin-pnp"
+  - "@yarnpkg/plugin-pnpm"
+  - "@yarnpkg/plugin-stage"
+  - "@yarnpkg/plugin-typescript"
+  - "@yarnpkg/plugin-version"
+  - "@yarnpkg/plugin-workspace-tools"
+  - "@yarnpkg/builder"
+  - "@yarnpkg/core"
+  - "@yarnpkg/doctor"

CHANGELOG.md

@@ -11,6 +11,7 @@ Yarn now accepts sponsorships! Please give a look at our [OpenCollective](https:
 - The node-modules linker has received various improvements:
   - applies hoisting algorithm on aliased dependencies
   - reinstalls modules that have their directories removed from node_modules by the user
+  - improves portal hoisting
 
 **Note:** features in `master` can be tried out by running `yarn set version from sources` in your project (existing contrib plugins are updated automatically, while new contrib plugins can be added by running `yarn plugin import from sources <name>`).
 

packages/yarnpkg-nm/sources/hoist.ts

@@ -396,7 +396,12 @@ const hoistTo = (tree: HoisterWorkTree, rootNodePath: Array<HoisterWorkTree>, ro
 
   const hoistIdentMap = getHoistIdentMap(rootNode, preferenceMap);
 
-  const usedDependencies = tree == rootNode ? new Map() : (options.fastLookupPossible ? getZeroRoundUsedDependencies(rootNodePath) :  getUsedDependencies(rootNodePath));
+  const usedDependencies = tree == rootNode ? new Map() :
+    (options.fastLookupPossible
+      ? getZeroRoundUsedDependencies(rootNodePath)
+      : getUsedDependencies(rootNodePath)
+    );
+
   let wasStateChanged;
 
   let anotherRoundNeeded = false;
@@ -438,6 +443,15 @@ const hoistTo = (tree: HoisterWorkTree, rootNodePath: Array<HoisterWorkTree>, ro
   return {anotherRoundNeeded, isGraphChanged};
 };
 
+const hasUnhoistedDependencies = (node: HoisterWorkTree): boolean => {
+  for (const [subName, subDependency] of node.dependencies) {
+    if (!node.peerNames.has(subName) && subDependency.ident !== node.ident) {
+      return true;
+    }
+  }
+  return false;
+};
+
 const getNodeHoistInfo = (rootNode: HoisterWorkTree, rootNodePathLocators: Set<Locator>, nodePath: Array<HoisterWorkTree>, node: HoisterWorkTree, usedDependencies: Map<PackageName, HoisterWorkTree>, hoistIdents: Map<PackageName, Ident>, hoistIdentMap: Map<Ident, Array<Ident>>, shadowedNodes: ShadowedNodes, {outputReason, fastLookupPossible}: {outputReason: boolean, fastLookupPossible: boolean}): HoistInfo => {
   let reasonRoot;
   let reason: string | null = null;
@@ -459,8 +473,8 @@ const getNodeHoistInfo = (rootNode: HoisterWorkTree, rootNodePathLocators: Set<L
     }
   }
 
-  if (isHoistable) {
-    isHoistable = node.dependencyKind !== HoisterDependencyKind.EXTERNAL_SOFT_LINK || node.dependencies.size === 0;
+  if (isHoistable && node.dependencyKind === HoisterDependencyKind.EXTERNAL_SOFT_LINK) {
+    isHoistable = !hasUnhoistedDependencies(node);
     if (outputReason && !isHoistable) {
       reason = `- external soft link with unhoisted dependencies`;
     }
@@ -559,7 +573,7 @@ const getNodeHoistInfo = (rootNode: HoisterWorkTree, rootNodePathLocators: Set<L
 
   if (isHoistable && !fastLookupPossible) {
     for (const origDep of node.hoistedDependencies.values()) {
-      const usedDep = usedDependencies.get(origDep.name);
+      const usedDep = usedDependencies.get(origDep.name) || rootNode.dependencies.get(origDep.name);
       if (!usedDep || origDep.ident !== usedDep.ident) {
         isHoistable = false;
         if (outputReason)
@@ -630,13 +644,15 @@ const hoistGraph = (tree: HoisterWorkTree, rootNodePath: Array<HoisterWorkTree>,
       if (hoistInfo.isHoistable === Hoistable.NO)
         addUnhoistableNode(node, hoistInfo, hoistInfo.reason!);
 
+    let wereNodesHoisted = false;
     for (const node of hoistInfos.keys()) {
       if (!unhoistableNodes.has(node)) {
         isGraphChanged = true;
         const shadowedNames = parentShadowedNodes.get(parentNode);
         if (shadowedNames && shadowedNames.has(node.name))
           anotherRoundNeeded = true;
 
+        wereNodesHoisted = true;
         parentNode.dependencies.delete(node.name);
         parentNode.hoistedDependencies.set(node.name, node);
         parentNode.reasons.delete(node.name);
@@ -668,6 +684,9 @@ const hoistGraph = (tree: HoisterWorkTree, rootNodePath: Array<HoisterWorkTree>,
       }
     }
 
+    if (parentNode.dependencyKind === HoisterDependencyKind.EXTERNAL_SOFT_LINK && wereNodesHoisted)
+      anotherRoundNeeded = true;
+
     if (options.check) {
       const checkLog = selfCheck(tree);
       if (checkLog) {

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

@@ -561,6 +561,25 @@ describe(`hoist`, () => {
     expect(getTreeHeight(hoist(toTree(tree), {check: true}))).toEqual(3);
   });
 
+  it(`should not hoist portal with unhoistable dependencies`, () => {
+    const tree = {
+      '.': {dependencies: [`P1`, `B@Y`]},
+      P1: {dependencies: [`P2`], dependencyKind: HoisterDependencyKind.EXTERNAL_SOFT_LINK},
+      P2: {dependencies: [`B@X`], dependencyKind: HoisterDependencyKind.EXTERNAL_SOFT_LINK},
+    };
+    expect(getTreeHeight(hoist(toTree(tree), {check: true}))).toEqual(3);
+  });
+
+  it(`should hoist nested portals with hoisted dependencies`, () => {
+    const tree = {
+      '.': {dependencies: [`P1`, `B@X`]},
+      P1: {dependencies: [`P2`, `B@X`], dependencyKind: HoisterDependencyKind.EXTERNAL_SOFT_LINK},
+      P2: {dependencies: [`P3`, `B@X`], dependencyKind: HoisterDependencyKind.EXTERNAL_SOFT_LINK},
+      P3: {dependencies: [`B@X`], dependencyKind: HoisterDependencyKind.EXTERNAL_SOFT_LINK},
+    };
+    expect(getTreeHeight(hoist(toTree(tree), {check: true}))).toEqual(2);
+  });
+
   it(`should support two branch circular graph hoisting`, () => {
     // . -> B -> D@X -> F@X
     //               -> E@X -> D@X