fix(node-modules): Tolerate user-created module directories (#2487)

* Tolerates user-created module directories

* Update versions

* Removes typo

Co-authored-by: Maël Nison <nison.mael@gmail.com>

.yarn/versions/66c148c7.yml

@@ -0,0 +1,23 @@
+releases:
+  "@yarnpkg/cli": 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"

CHANGELOG.md

@@ -14,6 +14,7 @@ Yarn now accepts sponsorships! Please give a look at our [OpenCollective](https:
 
 ### Installs
 
+- The node-modules linker now tolerates if node_modules is a symbolic link and does not recreates it
 - The pnpm linker has received various improvements:
   - It will now remove the `node_modules/.store` and `node_modules` folders if they are empty.
   - It now supports running binaries of soft links.

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

@@ -1388,7 +1388,6 @@ describe(`Node_Modules`, () => {
     ),
   );
 
-
   test(
     `should fallback to dependencies if the parent doesn't provide the peer dependency`,
     makeTemporaryEnv(
@@ -1505,6 +1504,55 @@ describe(`Node_Modules`, () => {
     ),
   );
 
+  it(`should handle the edge case when node_modules is a file`, () => {
+    makeTemporaryEnv(
+      {
+        private: true,
+        dependencies: {
+          'no-deps': `1.0.0`,
+        },
+      },
+      {
+        nodeLinker: `node-modules`,
+      },
+      async ({path, run, source}) => {
+        await xfs.writeFilePromise(ppath.resolve(path, `node_modules` as Filename), ``);
+
+        await run(`install`);
+
+        await expect(source(`require('no-deps')`)).resolves.toEqual({
+          name: `no-deps`,
+          version: `1.0.0`,
+        });
+      },
+    );
+  });
+
+  it(`should tolerate if node_modules is a symlink to other directory`, () => {
+    makeTemporaryEnv(
+      {
+        private: true,
+        dependencies: {
+          'no-deps': `1.0.0`,
+        },
+      },
+      {
+        nodeLinker: `node-modules`,
+      },
+      async ({path, run}) => {
+        const nmDir = ppath.resolve(path, `node_modules` as Filename);
+        const trueInstallDir = ppath.resolve(path, `target` as Filename);
+        await xfs.mkdirPromise(trueInstallDir);
+        await xfs.symlinkPromise(trueInstallDir, nmDir);
+
+        await run(`install`);
+
+        expect((await xfs.lstatPromise(nmDir)).isSymbolicLink()).toBeTruthy();
+        expect(await xfs.existsSync(ppath.join(trueInstallDir, `no-deps` as Filename))).toBeTruthy();
+      },
+    );
+  });
+
   it(`should install project when portal is pointing to a workspace`,
     makeTemporaryEnv(
       {

packages/plugin-nm/sources/NodeModulesLinker.ts

@@ -513,14 +513,15 @@ async function findInstallState(project: Project, {unrollAliases = false}: {unro
   return {locatorMap, binSymlinks, locationTree: buildLocationTree(locatorMap, {skipPrefix: project.cwd}), nmMode};
 }
 
-const removeDir = async (dir: PortablePath, options: {contentsOnly: boolean, innerLoop?: boolean}): Promise<any> => {
+const removeDir = async (dir: PortablePath, options: {contentsOnly: boolean, innerLoop?: boolean, allowSymlink?: boolean}): Promise<any> => {
   if (dir.split(ppath.sep).indexOf(NODE_MODULES) < 0)
     throw new Error(`Assertion failed: trying to remove dir that doesn't contain node_modules: ${dir}`);
 
   try {
     if (!options.innerLoop) {
-      const stats = await xfs.lstatPromise(dir);
-      if (stats.isSymbolicLink()) {
+      const stats = options.allowSymlink ? await xfs.statPromise(dir) : await xfs.lstatPromise(dir);
+      if (options.allowSymlink && !stats.isDirectory() ||
+        (!options.allowSymlink && stats.isSymbolicLink())) {
         await xfs.unlinkPromise(dir);
         return;
       }
@@ -1021,7 +1022,8 @@ async function persistNodeModules(preinstallState: InstallState, installState: N
       if (prevNode.children.has(NODE_MODULES))
         await removeDir(ppath.join(location, NODE_MODULES), {contentsOnly: false});
 
-      await removeDir(location, {contentsOnly: location === rootNmDirPath});
+      const isRootNmLocation = ppath.basename(location) === NODE_MODULES && locationTree.has(ppath.join(ppath.dirname(location), ppath.sep));
+      await removeDir(location, {contentsOnly: location === rootNmDirPath, allowSymlink: isRootNmLocation});
     } else {
       for (const [segment, prevChildNode] of prevNode.children) {
         const childNode = node.children.get(segment);
@@ -1038,7 +1040,8 @@ async function persistNodeModules(preinstallState: InstallState, installState: N
       if (segment === `.`)
         continue;
       const childNode = node ? node.children.get(segment) : node;
-      await removeOutdatedDirs(ppath.join(location, segment), prevChildNode, childNode);
+      const dirPath = ppath.join(location, segment);
+      await removeOutdatedDirs(dirPath, prevChildNode, childNode);
     }
   }
 
@@ -1048,12 +1051,10 @@ async function persistNodeModules(preinstallState: InstallState, installState: N
       if (node.children.has(NODE_MODULES))
         await removeDir(ppath.join(location, NODE_MODULES), {contentsOnly: true});
 
-      // 1. If old directory is a symlink removeDir will remove it, regardless contentsOnly value
-      // 2. If old and new directories are hard links - we pass contentsOnly: true
-      // so that removeDir cleared only contents
-      // 3. If new directory is a symlink - we pass contentsOnly: false
-      // so that removeDir removed the whole directory
-      await removeDir(location, {contentsOnly: node.linkType === LinkType.HARD});
+      // 1. If new directory is a symlink, we need to remove it fully
+      // 2. If new directory is a hardlink - we just need to clean it up
+      const isRootNmLocation = ppath.basename(location) === NODE_MODULES && locationTree.has(ppath.join(ppath.dirname(location), ppath.sep));
+      await removeDir(location, {contentsOnly: node.linkType === LinkType.HARD, allowSymlink: isRootNmLocation});
     } else {
       if (!areRealLocatorsEqual(node.locator, prevNode.locator))
         await removeDir(location, {contentsOnly: node.linkType === LinkType.HARD});