[node-modules] Avoids recreating nm folders and only clears them (#1185)

* Avoids recreating top-level node_modules folders and only clears their contents * Use imperative dir removal to make the code easier to read * Add removeDir behavior explanation * Always remove contents in root node_modules dir, never delete the dir itself * Remove known issues section, since there is no such problem anymore * Add failed manifest path into manifest load errors * Add test to check that linker does not recreate folders (fails for now) * Compare inodes to detect folder recreation
yarnpkg · Apr 14, 2020 · a6f9e6d · a6f9e6d
1 parent f35e31b
commit a6f9e6d
Show file tree

Hide file tree

Showing 4 changed files with 90 additions and 59 deletions.
diff --git a/.yarn/versions/badae20d.yml b/.yarn/versions/badae20d.yml
@@ -0,0 +1,20 @@
+releases:
+  "@yarnpkg/cli": prerelease
+  "@yarnpkg/plugin-node-modules": prerelease
+
+declined:
+  - "@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-stage"
+  - "@yarnpkg/plugin-typescript"
+  - "@yarnpkg/plugin-version"
+  - "@yarnpkg/plugin-workspace-tools"
+  - "@yarnpkg/core"
+  - "@yarnpkg/doctor"
diff --git a/packages/acceptance-tests/pkg-tests-specs/sources/node-modules.test.ts b/packages/acceptance-tests/pkg-tests-specs/sources/node-modules.test.ts
@@ -201,4 +201,37 @@ describe('Node_Modules', () => {
       },
     ),
   );
+
+  test(`should not recreated folders when package is updated`,
+    makeTemporaryEnv(
+      {
+        private: true,
+        dependencies: {
+          [`no-deps`]: `1.0.0`,
+        },
+      },
+      async ({path, run}) => {
+        await writeFile(npath.toPortablePath(`${path}/.yarnrc.yml`), `
+          nodeLinker: "node-modules"
+        `);
+
+        await expect(run(`install`)).resolves.toBeTruthy();
+
+        const nmFolderInode = xfs.statSync(npath.toPortablePath(`${path}/node_modules`)).ino;
+        const depFolderInode = xfs.statSync(npath.toPortablePath(`${path}/node_modules/no-deps`)).ino;
+
+        await writeJson(npath.toPortablePath(`${path}/package.json`), {
+          private: true,
+          dependencies: {
+            [`no-deps`]: `2.0.0`,
+          },
+        });
+
+        await expect(run(`install`)).resolves.toBeTruthy();
+
+        expect(xfs.statSync(npath.toPortablePath(`${path}/node_modules`)).ino).toEqual(nmFolderInode);
+        expect(xfs.statSync(npath.toPortablePath(`${path}/node_modules/no-deps`)).ino).toEqual(depFolderInode);
+      },
+    ),
+  );
 });
diff --git a/packages/plugin-node-modules/README.md b/packages/plugin-node-modules/README.md
@@ -13,7 +13,3 @@ nodeLinker: node-modules
 ## Word of caution
 
 While they are supported by virtually every tool, installs using the `node_modules` strategy have various fundamental issues that the default Plug'n'Play installations don't suffer from (for more details, check out our [documentation](https://yarnpkg.com/features/pnp)). Carefully consider the pros and cons before enabling this plugin.
-
-## Known issues
-
-- A same package / reference combination present multiple times within the same `node_modules` dependency tree will have issues calling `yarn run` from within its postinstall scripts. This is because this plugin is able to extract the package locator from the current cwd, but since a same locator may be found in multiple places it's not possible to convert it back from the locator to its location on the disk.
diff --git a/packages/plugin-node-modules/sources/NodeModulesLinker.ts b/packages/plugin-node-modules/sources/NodeModulesLinker.ts
@@ -166,7 +166,12 @@ class NodeModulesInstaller extends AbstractPnpInstaller {
     if (manifest)
       return manifest;
 
-    manifest = await Manifest.find(sourceLocation);
+    try {
+      manifest = await Manifest.find(sourceLocation);
+    } catch (e) {
+      e.message = `While loading ${sourceLocation}: ${e.message}`;
+      throw e;
+    }
     this.manifestCache.set(sourceLocation, manifest);
 
     return manifest;
@@ -311,12 +316,12 @@ async function findInstallState(project: Project, {unrollAliases = false}: {unro
   return {locatorMap, binSymlinks, locationTree: buildLocationTree(locatorMap, {skipPrefix: project.cwd})};
 };
 
-const removeDir = async (dir: PortablePath, options?: {innerLoop?: boolean}): Promise<any> => {
+const removeDir = async (dir: PortablePath, options: {contentsOnly: boolean, innerLoop?: 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 || !options.innerLoop) {
+    if (!options.innerLoop) {
       const stats = await xfs.lstatPromise(dir);
       if (stats.isSymbolicLink()) {
         await xfs.unlinkPromise(dir);
@@ -328,13 +333,15 @@ const removeDir = async (dir: PortablePath, options?: {innerLoop?: boolean}): Pr
       const targetPath = ppath.join(dir, toFilename(entry.name));
       if (entry.isDirectory()) {
         if (entry.name !== NODE_MODULES || (options && options.innerLoop)) {
-          await removeDir(targetPath, {innerLoop: true});
+          await removeDir(targetPath, {innerLoop: true, contentsOnly: false});
         }
       } else {
         await xfs.unlinkPromise(targetPath);
       }
     }
-    await xfs.rmdirPromise(dir);
+    if (!options.contentsOnly) {
+      await xfs.rmdirPromise(dir);
+    }
   } catch (e) {
     if (e.code !== 'ENOENT' && e.code !== 'ENOTEMPTY') {
       throw e;
@@ -345,7 +352,7 @@ const removeDir = async (dir: PortablePath, options?: {innerLoop?: boolean}): Pr
 const CONCURRENT_OPERATION_LIMIT = 4;
 
 type LocatorKey = string;
-type LocationNode = { children: Map<Filename, LocationNode>, locator?: LocatorKey };
+type LocationNode = { children: Map<Filename, LocationNode>, locator?: LocatorKey, linkType: LinkType };
 type LocationRoot = PortablePath;
 
 /**
@@ -408,6 +415,7 @@ const buildLocationTree = (locatorMap: NodeModulesLocatorMap | null, {skipPrefix
 
   const makeNode: () => LocationNode = () => ({
     children: new Map(),
+    linkType: LinkType.HARD,
   });
 
   for (const [locator, info] of locatorMap.entries()) {
@@ -416,6 +424,7 @@ const buildLocationTree = (locatorMap: NodeModulesLocatorMap | null, {skipPrefix
       if (internalPath !== null) {
         const node = miscUtils.getFactoryWithDefault(locationTree, info.target, makeNode);
         node.locator = locator;
+        node.linkType = info.linkType;
       }
     }
 
@@ -436,6 +445,7 @@ const buildLocationTree = (locatorMap: NodeModulesLocatorMap | null, {skipPrefix
 
         if (idx === segments.length - 1) {
           node.locator = locator;
+          node.linkType = info.linkType;
         }
       }
     }
@@ -656,38 +666,16 @@ async function persistNodeModules(preinstallState: InstallState, installState: N
     }
   };
 
-  const deleteQueue: Promise<any>[] = [];
-  const deleteModule = async (dstDir: PortablePath) => {
-    const promise: Promise<any> = (async () => {
-      try {
-        await removeDir(dstDir);
-      } catch (e) {
-        e.message = `While removing ${dstDir} ${e.message}`;
-        throw e;
-      }
-    })().then(() => deleteQueue.splice(deleteQueue.indexOf(promise), 1));
-    deleteQueue.push(promise);
-    if (deleteQueue.length > CONCURRENT_OPERATION_LIMIT) {
-      await Promise.race(deleteQueue);
-    }
-  };
-
-
-  // Delete locations that no longer exist
-  const deleteList = new Set<PortablePath>();
-  // Delete locations of inner node_modules
-  const innerDeleteList = new Set<PortablePath>();
-
-  const recordOutdatedDirsToRemove = (location: PortablePath, prevNode: LocationNode, node?: LocationNode) => {
+  const removeOutdatedDirs = async (location: PortablePath, prevNode: LocationNode, node?: LocationNode) => {
     if (!node) {
-      deleteList.add(location);
-      if (prevNode.children.has(NODE_MODULES)) {
-        innerDeleteList.add(ppath.join(location, NODE_MODULES));
-      }
+      if (prevNode.children.has(NODE_MODULES))
+        await removeDir(ppath.join(location, NODE_MODULES), {contentsOnly: false});
+
+      await removeDir(location, {contentsOnly: location === rootNmDirPath});
     } else {
       for (const [segment, prevChildNode] of prevNode.children) {
         let childNode = node.children.get(segment);
-        recordOutdatedDirsToRemove(ppath.join(location, segment), prevChildNode, childNode);
+        await removeOutdatedDirs(ppath.join(location, segment), prevChildNode, childNode);
       }
     }
   };
@@ -700,24 +688,29 @@ async function persistNodeModules(preinstallState: InstallState, installState: N
       if (segment === '.')
         continue;
       let childNode = node ? node.children.get(segment) : node;
-      recordOutdatedDirsToRemove(ppath.join(location, segment), prevChildNode, childNode);
+      await removeOutdatedDirs(ppath.join(location, segment), prevChildNode, childNode);
     }
   }
 
-  const recordNewDirsToClean = (location: PortablePath, node: LocationNode, prevNode?: LocationNode, ) => {
+  const cleanNewDirs = async (location: PortablePath, node: LocationNode, prevNode?: LocationNode) => {
     if (!prevNode) {
-      deleteList.add(location);
-      if (node.children.has(NODE_MODULES)) {
-        innerDeleteList.add(ppath.join(location, NODE_MODULES));
-      }
+      // We want to clean only contents of top-level node_modules dir, since we need these dirs to be present
+      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 hardlinks - 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});
     } else {
-      // Location is changed and will be occupied by a different locator - clean it
       if (node.locator !== prevNode.locator)
-        deleteList.add(location);
+        await removeDir(location, {contentsOnly: node.linkType === LinkType.HARD});
 
       for (const [segment, childNode] of node.children) {
         let prevChildNode = prevNode.children.get(segment);
-        recordNewDirsToClean(ppath.join(location, segment), childNode, prevChildNode);
+        await cleanNewDirs(ppath.join(location, segment), childNode, prevChildNode);
       }
     }
   };
@@ -730,19 +723,10 @@ async function persistNodeModules(preinstallState: InstallState, installState: N
       if (segment === '.')
         continue;
       let prevChildNode = prevNode ? prevNode.children.get(segment) : prevNode;
-      recordNewDirsToClean(ppath.join(location, segment), childNode, prevChildNode);
+      await cleanNewDirs(ppath.join(location, segment), childNode, prevChildNode);
     }
   }
 
-  // Handle inner node_modules deletions first
-  for (const dstDir of innerDeleteList)
-    await deleteModule(dstDir);
-  await Promise.all(deleteQueue);
-  deleteQueue.length = 0;
-
-  for (const dstDir of deleteList)
-    await deleteModule(dstDir);
-
   // Update changed locations
   const addList: Array<{srcDir: PortablePath, dstDir: PortablePath, linkType: LinkType}> = [];
   for (const [prevLocator, {locations}] of preinstallState.locatorMap.entries()) {
@@ -817,8 +801,6 @@ async function persistNodeModules(preinstallState: InstallState, installState: N
   try {
     const persistedLocations = new Map<PortablePath, PortablePath>();
 
-    await Promise.all(deleteQueue);
-
     // For the first pass we'll only want to install a single copy for each
     // source directory. We'll later use the resulting install directories for
     // the other instances of the same package (this will avoid us having to