fix(nm): Reinstall removed module directories (#3467)

Author: larixer

.yarn/versions/934c31c2.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

@@ -8,7 +8,9 @@ Yarn now accepts sponsorships! Please give a look at our [OpenCollective](https:
 
 ### Installs
 - The pnpm linker no longer tries to remove `node_modules` directory, when `node-modules` linker is active
-- The nm linker applies hoisting algorithm on aliased dependencies
+- 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
 
 **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/acceptance-tests/pkg-tests-specs/sources/node-modules.test.ts

@@ -1620,6 +1620,35 @@ describe(`Node_Modules`, () => {
       }),
   );
 
+  it(`should reinstall and rebuild dependencies deleted by the user on the next install`,
+    makeTemporaryEnv(
+      {
+        dependencies: {
+          [`no-deps-scripted`]: `1.0.0`,
+          [`one-dep-scripted`]: `1.0.0`,
+        },
+      },
+      {
+        nodeLinker: `node-modules`,
+      },
+      async ({path, run, source}) => {
+        await run(`install`);
+        await xfs.removePromise(`${path}/node_modules/one-dep-scripted` as PortablePath);
+
+        const {stdout} = await run(`install`);
+
+        // Yarn must reinstall and rebuild only the removed package
+        expect(stdout).not.toMatch(new RegExp(`no-deps-scripted@npm:1.0.0 must be built`));
+        expect(stdout).toMatch(new RegExp(`one-dep-scripted@npm:1.0.0 must be built`));
+
+        await expect(source(`require('one-dep-scripted')`)).resolves.toMatchObject({
+          name: `one-dep-scripted`,
+          version: `1.0.0`,
+        });
+      },
+    ),
+  );
+
   it(`should support portals to external workspaces`,
     makeTemporaryEnv(
       {

packages/plugin-nm/sources/NodeModulesLinker.ts

@@ -21,7 +21,7 @@ const NODE_MODULES = `node_modules` as Filename;
 const DOT_BIN = `.bin` as Filename;
 const INSTALL_STATE_FILE = `.yarn-state.yml` as Filename;
 
-type InstallState = {locatorMap: NodeModulesLocatorMap, locationTree: LocationTree, binSymlinks: BinSymlinkMap, nmMode: NodeModulesMode};
+type InstallState = {locatorMap: NodeModulesLocatorMap, locationTree: LocationTree, binSymlinks: BinSymlinkMap, nmMode: NodeModulesMode, mtimeMs: number};
 type BinSymlinkMap = Map<PortablePath, Map<Filename, PortablePath>>;
 type LoadManifest = (locator: LocatorKey, installLocation: PortablePath) => Promise<Pick<Manifest, 'bin'>>;
 
@@ -232,7 +232,7 @@ class NodeModulesInstaller implements Installer {
     if (preinstallState === null || nmModeSetting !== preinstallState.nmMode) {
       this.opts.project.storedBuildState.clear();
 
-      preinstallState = {locatorMap: new Map(), binSymlinks: new Map(), locationTree: new Map(), nmMode: nmModeSetting};
+      preinstallState = {locatorMap: new Map(), binSymlinks: new Map(), locationTree: new Map(), nmMode: nmModeSetting, mtimeMs: 0};
     }
 
     const hoistingLimitsByCwd = new Map(this.opts.project.workspaces.map(workspace => {
@@ -392,7 +392,7 @@ async function extractCustomPackageData(pkg: Package, fetchResult: FetchResult)
   };
 }
 
-async function writeInstallState(project: Project, locatorMap: NodeModulesLocatorMap, binSymlinks: BinSymlinkMap, nmMode: {value: NodeModulesMode}) {
+async function writeInstallState(project: Project, locatorMap: NodeModulesLocatorMap, binSymlinks: BinSymlinkMap, nmMode: {value: NodeModulesMode}, {installChangedByUser}: {installChangedByUser: boolean}) {
   let locatorState = ``;
 
   locatorState += `# Warning: This file is automatically generated. Removing it is fine, but will\n`;
@@ -445,6 +445,10 @@ async function writeInstallState(project: Project, locatorMap: NodeModulesLocato
   const rootPath = project.cwd;
   const installStatePath = ppath.join(rootPath, NODE_MODULES, INSTALL_STATE_FILE);
 
+  // Force install state file rewrite, so that it has mtime bigger than all node_modules subfolders
+  if (installChangedByUser)
+    await xfs.removePromise(installStatePath);
+
   await xfs.changeFilePromise(installStatePath, locatorState, {
     automaticNewlines: true,
   });
@@ -454,7 +458,13 @@ async function findInstallState(project: Project, {unrollAliases = false}: {unro
   const rootPath = project.cwd;
   const installStatePath = ppath.join(rootPath, NODE_MODULES, INSTALL_STATE_FILE);
 
-  if (!xfs.existsSync(installStatePath))
+  let stats;
+  try {
+    stats = await xfs.statPromise(installStatePath);
+  } catch (e) {
+  }
+
+  if (!stats)
     return null;
 
   const locatorState = parseSyml(await xfs.readFilePromise(installStatePath, `utf8`));
@@ -481,7 +491,7 @@ async function findInstallState(project: Project, {unrollAliases = false}: {unro
         const location = ppath.join(rootPath, npath.toPortablePath(relativeLocation));
         const symlinks = miscUtils.getMapWithDefault(binSymlinks, location);
         for (const [name, target] of Object.entries(locationSymlinks as any)) {
-          symlinks.set(toFilename(name), npath.toPortablePath([location, NODE_MODULES, target].join(ppath.delimiter)));
+          symlinks.set(toFilename(name), npath.toPortablePath([location, NODE_MODULES, target].join(ppath.sep)));
         }
       }
     }
@@ -510,7 +520,7 @@ async function findInstallState(project: Project, {unrollAliases = false}: {unro
     }
   }
 
-  return {locatorMap, binSymlinks, locationTree: buildLocationTree(locatorMap, {skipPrefix: project.cwd}), nmMode};
+  return {locatorMap, binSymlinks, locationTree: buildLocationTree(locatorMap, {skipPrefix: project.cwd}), nmMode, mtimeMs: stats.mtimeMs};
 }
 
 const removeDir = async (dir: PortablePath, options: {contentsOnly: boolean, innerLoop?: boolean, allowSymlink?: boolean}): Promise<any> => {
@@ -818,40 +828,109 @@ const copyPromise = async (dstDir: PortablePath, srcDir: PortablePath, {baseFs,
 };
 
 /**
- * This function removes node_modules roots that do not exist on the filesystem from the location tree.
- *
- * This is needed to transparently support workflows on CI systems. When
- * user caches only top-level node_modules and forgets to cache node_modules
- * from deeper workspaces. By removing non-existent node_modules roots
- * we make our location tree to represent the real tree on the file system.
- *
- * Please note, that this function doesn't help with any other inconsistency
- * on a deeper level inside node_modules tree, it helps only when some node_modules roots
- * do not exist at all
+ * Synchronizes previous install state with the actual directories available on disk
  *
  * @param locationTree location tree
+ * @param binSymlinks bin symlinks map
+ * @param stateMtimeMs state file timestamp (this file is written after all node_modules files and directories)
  *
- * @returns location tree with non-existent node_modules roots stripped
+ * @returns location tree and bin symlinks with modules, unavailable on disk, removed
  */
-function refineNodeModulesRoots(locationTree: LocationTree, binSymlinks: BinSymlinkMap): {locationTree: LocationTree, binSymlinks: BinSymlinkMap} {
-  const refinedLocationTree: LocationTree = new Map([...locationTree]);
-  const refinedBinSymlinks: BinSymlinkMap = new Map([...binSymlinks]);
+function syncPreinstallStateWithDisk(locationTree: LocationTree, binSymlinks: BinSymlinkMap, stateMtimeMs: number, project: Project): {locationTree: LocationTree, binSymlinks: BinSymlinkMap, locatorLocations: Map<LocatorKey, Set<PortablePath>>, installChangedByUser: boolean} {
+  const refinedLocationTree: LocationTree = new Map();
+  const refinedBinSymlinks = new Map();
+  const locatorLocations = new Map();
+  let installChangedByUser = false;
+
+  const syncNodeWithDisk = (parentPath: PortablePath, entry: Filename, parentNode: LocationNode, refinedNode: LocationNode, nodeModulesDiskEntries: Set<Filename>) => {
+    let doesExistOnDisk = true;
+    const entryPath = ppath.join(parentPath, entry);
+    let childNodeModulesDiskEntries = new Set<Filename>();
+
+    if (entry === NODE_MODULES) {
+      let stats;
+      try {
+        stats = xfs.statSync(entryPath);
+      } catch (e) {
+      }
 
-  for (const [workspaceRoot, node] of locationTree) {
-    const nodeModulesRoot = ppath.join(workspaceRoot, NODE_MODULES);
-    if (!xfs.existsSync(nodeModulesRoot)) {
-      node.children.delete(NODE_MODULES);
-
-      // O(m^2) complexity algorithm, but on a very few values, so not worth the trouble to optimize it
-      for (const location of refinedBinSymlinks.keys()) {
-        if (ppath.contains(nodeModulesRoot, location) !== null) {
-          refinedBinSymlinks.delete(location);
+      doesExistOnDisk = !!stats;
+
+      if (!stats) {
+        installChangedByUser = true;
+      } else if (stats.mtimeMs > stateMtimeMs) {
+        installChangedByUser = true;
+        childNodeModulesDiskEntries = new Set(xfs.readdirSync(entryPath));
+      } else {
+        childNodeModulesDiskEntries = new Set(parentNode.children.get(NODE_MODULES)!.children.keys());
+      }
+
+      const binarySymlinks = binSymlinks.get(parentPath);
+      if (binarySymlinks) {
+        const binPath = ppath.join(parentPath, NODE_MODULES, DOT_BIN);
+        let binStats;
+        try {
+          binStats = xfs.statSync(binPath);
+        } catch (e) {
         }
+
+        if (!binStats) {
+          installChangedByUser = true;
+        } else if (binStats.mtimeMs > stateMtimeMs) {
+          installChangedByUser = true;
+
+          const diskEntries = new Set(xfs.readdirSync(binPath));
+          const refinedBinarySymlinks = new Map();
+          refinedBinSymlinks.set(parentPath, refinedBinarySymlinks);
+
+          for (const [entry, target] of binarySymlinks) {
+            if (diskEntries.has(entry)) {
+              refinedBinarySymlinks.set(entry, target);
+            }
+          }
+        } else {
+          refinedBinSymlinks.set(parentPath, binarySymlinks);
+        }
+      }
+    } else {
+      doesExistOnDisk = nodeModulesDiskEntries.has(entry);
+    }
+
+    const node = parentNode.children.get(entry)!;
+    if (doesExistOnDisk) {
+      const {linkType, locator} = node;
+      const childRefinedNode = {children: new Map(), linkType, locator};
+      refinedNode.children.set(entry, childRefinedNode);
+      if (locator) {
+        const locations = miscUtils.getSetWithDefault(locatorLocations, locator);
+        locations.add(entryPath);
+        locatorLocations.set(locator, locations);
+      }
+
+      for (const childEntry of node.children.keys()) {
+        syncNodeWithDisk(entryPath, childEntry, node, childRefinedNode, childNodeModulesDiskEntries);
       }
+    } else if (node.locator) {
+      project.storedBuildState.delete(structUtils.parseLocator(node.locator).locatorHash);
+    }
+  };
+
+  for (const [workspaceRoot, node] of locationTree) {
+    const {linkType, locator} = node;
+    const refinedNode = {children: new Map(), linkType, locator};
+    refinedLocationTree.set(workspaceRoot, refinedNode);
+    if (locator) {
+      const locations = miscUtils.getSetWithDefault(locatorLocations, node.locator);
+      locations.add(workspaceRoot);
+      locatorLocations.set(node.locator, locations);
+    }
+
+    if (node.children.has(NODE_MODULES)) {
+      syncNodeWithDisk(workspaceRoot, NODE_MODULES, node, refinedNode, new Set());
     }
   }
 
-  return {locationTree: refinedLocationTree, binSymlinks: refinedBinSymlinks};
+  return {locationTree: refinedLocationTree, binSymlinks: refinedBinSymlinks, locatorLocations, installChangedByUser};
 }
 
 function isLinkLocator(locatorKey: LocatorKey): boolean {
@@ -942,7 +1021,12 @@ export function getGlobalHardlinksStore(configuration: Configuration): PortableP
 async function persistNodeModules(preinstallState: InstallState, installState: NodeModulesLocatorMap, {baseFs, project, report, loadManifest, realLocatorChecksums}: {project: Project, baseFs: FakeFS<PortablePath>, report: Report, loadManifest: LoadManifest, realLocatorChecksums: Map<LocatorHash, string | null>}) {
   const rootNmDirPath = ppath.join(project.cwd, NODE_MODULES);
 
-  const {locationTree: prevLocationTree, binSymlinks: prevBinSymlinks} = refineNodeModulesRoots(preinstallState.locationTree, preinstallState.binSymlinks);
+  const {
+    locationTree: prevLocationTree,
+    binSymlinks: prevBinSymlinks,
+    locatorLocations: prevLocatorLocations,
+    installChangedByUser,
+  } = syncPreinstallStateWithDisk(preinstallState.locationTree, preinstallState.binSymlinks, preinstallState.mtimeMs, project);
 
   const locationTree = buildLocationTree(installState, {skipPrefix: project.cwd});
 
@@ -1082,7 +1166,7 @@ async function persistNodeModules(preinstallState: InstallState, installState: N
 
   // Update changed locations
   const addList: Array<{srcDir: PortablePath, dstDir: PortablePath, linkType: LinkType, realLocatorHash: LocatorHash}> = [];
-  for (const [prevLocator, {locations}] of preinstallState.locatorMap.entries()) {
+  for (const [prevLocator, locations] of prevLocatorLocations) {
     for (const location of locations) {
       const {locationRoot, segments} = parseLocation(location, {
         skipPrefix: project.cwd,
@@ -1205,7 +1289,7 @@ async function persistNodeModules(preinstallState: InstallState, installState: N
     const binSymlinks = await createBinSymlinkMap(installState, locationTree, project.cwd, {loadManifest});
     await persistBinSymlinks(prevBinSymlinks, binSymlinks, project.cwd);
 
-    await writeInstallState(project, installState, binSymlinks, nmMode);
+    await writeInstallState(project, installState, binSymlinks, nmMode, {installChangedByUser});
 
     if (nmModeSetting == NodeModulesMode.HARDLINKS_GLOBAL && nmMode.value == NodeModulesMode.HARDLINKS_LOCAL) {
       report.reportWarningOnce(MessageName.NM_HARDLINKS_MODE_DOWNGRADED, `'nmMode' has been downgraded to 'hardlinks-local' due to global cache and install folder being on different devices`);