fix(nm): Stop hoisting rounds only when nothing were hoisted

.yarn/versions/40a42318.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

@@ -10,6 +10,8 @@ Features in `master` can be tried out by running `yarn set version from sources`
 
 - Fixes `preferInteractive` forcing interactive mode in non-TTY environments.
 - `node-modules` linker now honors user-defined symlinks for `<workspace>/node_modules` directories
+- `node-modules` linker supports hoisting into inner workspaces that are parents of other workspaces
+- `node-modules` linker attemps to hoist tree more exhaustivel until nothing can be hoisted
 
 ## 4.1.0
 

packages/yarnpkg-nm/sources/hoist.ts

@@ -132,7 +132,8 @@ export const hoist = (tree: HoisterTree, opts: HoistOptions = {}): HoisterResult
   let anotherRoundNeeded = false;
   let round = 0;
   do {
-    anotherRoundNeeded = hoistTo(treeCopy, [treeCopy], new Set([treeCopy.locator]), new Map(), options).anotherRoundNeeded;
+    const result = hoistTo(treeCopy, [treeCopy], new Set([treeCopy.locator]), new Map(), options);
+    anotherRoundNeeded = result.anotherRoundNeeded || result.isGraphChanged;
     options.fastLookupPossible = false;
     round++;
   } while (anotherRoundNeeded);
@@ -480,24 +481,6 @@ const getNodeHoistInfo = (rootNode: HoisterWorkTree, rootNodePathLocators: Set<L
     }
   }
 
-  if (isHoistable) {
-    // Direct workspace dependencies must be hoisted to any common ancestor workspace of all the
-    // graph paths that include the dependency, because otherwise running app with
-    // `--preserve-symlinks` will become broken (without this flag the Node.js will pick dependency
-    // from the ancestor on the file system and with this flag it will pick ancestor from the graph
-    // and if these ancestors are different, the behavious of the application will be different).
-    // Another problem, which is prevented - is a creation of multiple hoisting layouts
-    // for the same workspace, because different dependencies of the same workspace might be hoisted
-    // differently, depending on the recepient workspace.
-    // It is difficult to find all common ancestors, but there is one easy to find common ancestor -
-    // the root workspace, so, for now, we either hoist direct dependencies into the root workspace, or we keep them
-    // unhoisted, thus we are safe from various pathological cases with `--preserve-symlinks`
-    isHoistable = parentNode.dependencyKind !== HoisterDependencyKind.WORKSPACE || parentNode.hoistedFrom.has(node.name) || rootNodePathLocators.size === 1;
-    if (outputReason && !isHoistable) {
-      reason = parentNode.reasons.get(node.name)!;
-    }
-  }
-
   if (isHoistable) {
     isHoistable = !rootNode.peerNames.has(node.name);
     if (outputReason && !isHoistable) {

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

@@ -534,22 +534,52 @@ describe(`hoist`, () => {
     expect(getTreeHeight(hoist(toTree(tree), {check: true}))).toEqual(2);
   });
 
-  it(`should avoid hoisting direct workspace dependencies into non-root workspace`, () => {
+  it(`should hoist direct workspace dependencies into non-root workspace`, () => {
     // . -> W1(w) -> W2(w) -> W3(w)-> A@X
     //            -> A@Y
     //   -> W3
     //   -> A@Z
-    // The A@X must not be hoisted into W2(w)
-    // otherwise accessing A via . -> W3 with --preserve-symlinks will result in A@Z,
-    // but accessing it via W3(w) will result in A@Y
+    // The A@X must be hoisted into W2(w)
+    // Accessing A via . -> W3 with --preserve-symlinks will result in A@Z,
+    // but accessing it via W3(w) will result in A@Y, however if we don't do it,
+    // inner workspaces will have multiple unexpected copies of dependencies
     const tree = {
       '.': {dependencies: [`W1(w)`, `W3`, `A@Z`], dependencyKind: HoisterDependencyKind.WORKSPACE},
       'W1(w)': {dependencies: [`W2(w)`, `A@Y`], dependencyKind: HoisterDependencyKind.WORKSPACE},
       'W2(w)': {dependencies: [`W3(w)`], dependencyKind: HoisterDependencyKind.WORKSPACE},
       'W3(w)': {dependencies: [`A@X`], dependencyKind: HoisterDependencyKind.WORKSPACE},
     };
 
-    expect(getTreeHeight(hoist(toTree(tree), {check: true}))).toEqual(5);
+    expect(getTreeHeight(hoist(toTree(tree), {check: true}))).toEqual(4);
+  });
+
+  it(`should hoist dependencies to the top from workspaces that have no hoist borders given there is workspace with hoist borders`, () => {
+    // . -> W1(w)| -> A@X --> B
+    //             -> B@X
+    //   -> W2(w)  -> A@Y --> B
+    //             -> B@Y
+    // should be hoisted to:
+    // . -> W1(w)| -> A@X -->B
+    //             -> B@X
+    //   -> W2(w)
+    //   -> A@Y --> B
+    //   -> B@Y
+
+    const tree = {
+      '.': {dependencies: [`W1(w)`, `W2(w)`], dependencyKind: HoisterDependencyKind.WORKSPACE},
+      'W1(w)': {dependencies: [`A@X`, `B@X`], dependencyKind: HoisterDependencyKind.WORKSPACE},
+      'A@X': {dependencies: [`B@X`], peerNames: [`B`]},
+      'A@Y': {dependencies: [`B@Y`], peerNames: [`B`]},
+      'W2(w)': {dependencies: [`A@Y`, `B@Y`], dependencyKind: HoisterDependencyKind.WORKSPACE},
+    };
+
+    const hoistingLimits = new Map([
+      [`.@`, new Set([`W1(w)`])],
+    ]);
+
+    const hoistedTree = hoist(toTree(tree), {check: true, hoistingLimits});
+    const W2 = Array.from(Array.from(hoistedTree.dependencies).filter(x => x.name === `W2(w)`)[0].dependencies);
+    expect(W2).toEqual([]);
   });
 
   it(`should hoist aliased packages`, () => {