fix(plugin-nm): Avoid duplicate copies inside aliased dependencies. (#4571)

* Avoids creation of circular symlinks and duplicates inside aliases

* Add a test to check that duplicate copies of aliased packages are not created

* Stricter ident comparison for hoisted packages

* chore: update changelog

Co-authored-by: merceyz <[email protected]>

Author: larixer

.yarn/versions/6da17a68.yml

@@ -0,0 +1,25 @@
+releases:
+  "@yarnpkg/cli": patch
+  "@yarnpkg/nm": patch
+  "@yarnpkg/plugin-nm": 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"
+  - "@yarnpkg/pnpify"

CHANGELOG.md

@@ -67,6 +67,8 @@ The following changes only affect people writing Yarn plugins:
 
 - The `pnpm` linker avoids creating symlinks that lead to loops on the file system, by moving them higher up in the directory structure.
 - The `pnpm` linker no longer reports duplicate "incompatible virtual" warnings.
+- The node-modules linker avoids creation of circular symlinks
+- The node-modules linker no longer creates duplicate copies inside of aliased packages
 
 ### Bugfixes
 

packages/acceptance-tests/pkg-tests-specs/sources/dragon.test.js

@@ -550,7 +550,6 @@ describe(`Dragon tests`, () => {
   for (const [nodeLinker, shouldHaveAccessToTheSameInstance] of [
     [`pnp`, true],
     [`pnpm`, true],
-    [`node-modules`, false],
   ]) {
     test(`it should pass the dragon test 11 with "nodeLinker: ${nodeLinker}"`,
       makeTemporaryEnv(
@@ -580,9 +579,10 @@ describe(`Dragon tests`, () => {
           await expect(run(`install`)).resolves.toBeTruthy();
 
           // Make sure that both the root and dragon-test-11-b have access to the same instance.
-          // This is only possible with the PnP and pnpm linkers, because the node-modules linker
-          // can't fulfill the peer dependency promise. For the NM linker we test that it at least
-          // fulfills the require promise (installing dragon-test-11-a both under the aliased and original name).
+          // This is only possible with the PnP and pnpm linkers.
+          // The node-modules linker can't fullfil these requirements for aliased packages,
+          // without resorting to symlinks and a layout resembling pnpm for aliased dependencies,
+          // which will be too different from the classic node_modules layout for all the other dependencies.
           await expect(source(`
             (() => {
               const {createRequire} = require(\`module\`);

packages/acceptance-tests/pkg-tests-specs/sources/node-modules.test.ts

@@ -347,6 +347,25 @@ describe(`Node_Modules`, () => {
     ),
   );
 
+  test(`should not create duplicate copies of aliased packages`,
+    makeTemporaryEnv(
+      {
+        private: true,
+        dependencies: {
+          [`no-deps2`]: `npm:[email protected]`,
+        },
+      },
+      {
+        nodeLinker: `node-modules`,
+      },
+      async ({path, run, source}) => {
+        await run(`install`);
+
+        expect(await xfs.existsPromise(`${path}/node_modules/no-deps2/node_modules/no-deps` as PortablePath)).toBe(false);
+      },
+    ),
+  );
+
   test(`should not hoist package peer dependent on parent if conflict exists`,
     // . -> dep -> conflict@2 -> unhoistable --> conflict
     //   -> conflict@1
@@ -563,6 +582,27 @@ describe(`Node_Modules`, () => {
     ),
   );
 
+  test(`should not create circular self-reference symlinks`,
+    makeTemporaryEnv(
+      {
+        workspaces: [`ws`],
+      },
+      {
+        nodeLinker: `node-modules`,
+        nmHoistingLimits: `workspaces`,
+      },
+      async ({path, run}) => {
+        await writeJson(npath.toPortablePath(`${path}/ws/package.json`), {
+          name: `ws`,
+        });
+
+        await run(`install`);
+
+        expect(await xfs.existsPromise(`${path}/ws/node_modules/ws` as PortablePath)).toEqual(false);
+      },
+    ),
+  );
+
   test(`should not hoist multiple packages past workspace hoist border`,
     // . -> workspace -> dep1 -> dep2
     // should be hoisted to:

packages/yarnpkg-nm/sources/buildNodeModulesTree.ts

@@ -517,9 +517,11 @@ const populateNodeModulesTree = (pnp: PnpApi, hoistedTree: HoisterResult, option
 
     seenNodes.add(pkg);
 
+    const pkgReferences = Array.from(pkg.references).sort().join(`#`);
     for (const dep of pkg.dependencies) {
+      const depReferences = Array.from(dep.references).sort().join(`#`);
       // We do not want self-references in node_modules, since they confuse existing tools
-      if (dep === pkg)
+      if (dep.identName === pkg.identName && depReferences === pkgReferences)
         continue;
       const references = Array.from(dep.references).sort();
       const locator = {name: dep.identName, reference: references[0]};
@@ -542,7 +544,9 @@ const populateNodeModulesTree = (pnp: PnpApi, hoistedTree: HoisterResult, option
         isAnonymousWorkspace = !!(workspace && !workspace.manifest.name);
       }
 
-      if (!dep.name.endsWith(WORKSPACE_NAME_SUFFIX) && !isAnonymousWorkspace) {
+      const isCircularSymlink = leafNode.linkType === LinkType.SOFT && nodeModulesLocation.startsWith(leafNode.target);
+
+      if (!dep.name.endsWith(WORKSPACE_NAME_SUFFIX) && !isAnonymousWorkspace && !isCircularSymlink) {
         const prevNode = tree.get(nodeModulesLocation);
         if (prevNode) {
           if (prevNode.dirList) {