fix(install): cleanup node_modules folder on install (#32058)

This is only for `deno install/add/remove`. We don't do this for other
commands because it could cause problems (ex. two deno subprocesses
running at the same time with "lazy npm deps" or whatever it's called)
and also it would add more startup time.

Author: dsherret

Cargo.lock

@@ -264,7 +264,7 @@ simd-json = "0.14.0"
 slab = "0.4"
 smallvec = "1.8"
 socket2 = { version = "0.5.3", features = ["all"] }
-sys_traits = "=0.1.22"
+sys_traits = "=0.1.24"
 tar = "=0.4.43"
 tempfile = "3.4.0"
 termcolor = "1.1.3"

Cargo.toml

@@ -67,6 +67,7 @@ use crate::args::ConfigFlag;
 use crate::args::DenoSubcommand;
 use crate::args::Flags;
 use crate::args::InstallFlags;
+use crate::args::InstallFlagsLocal;
 use crate::cache::Caches;
 use crate::cache::CodeCache;
 use crate::cache::DenoDir;
@@ -600,6 +601,54 @@ impl CliFactory {
           .cloned()
           .map(|r| r as Arc<dyn deno_npm_installer::InstallReporter>),
         NpmInstallerFactoryOptions {
+          clean_on_install: match cli_options.sub_command() {
+            DenoSubcommand::Add { .. }
+            | DenoSubcommand::ApproveScripts { .. }
+            | DenoSubcommand::Remove { .. }
+            | DenoSubcommand::Cache { .. }
+            | DenoSubcommand::Uninstall { .. } => true,
+            DenoSubcommand::Install(flags) => match flags {
+              InstallFlags::Local(flags) => match flags {
+                InstallFlagsLocal::Add { .. }
+                | InstallFlagsLocal::TopLevel { .. } => true,
+                // someone might be terribly storing dependencies in a
+                // module and needs to populate their node_modules directory
+                // with the dependencies in that file
+                InstallFlagsLocal::Entrypoints { .. } => false,
+              },
+              InstallFlags::Global(_) => false,
+            },
+            DenoSubcommand::Audit { .. }
+            | DenoSubcommand::Bench { .. }
+            | DenoSubcommand::Bundle { .. }
+            | DenoSubcommand::Check { .. }
+            | DenoSubcommand::Clean { .. }
+            | DenoSubcommand::Compile { .. }
+            | DenoSubcommand::Completions { .. }
+            | DenoSubcommand::Coverage { .. }
+            | DenoSubcommand::Deploy { .. }
+            | DenoSubcommand::Doc { .. }
+            | DenoSubcommand::Eval { .. }
+            | DenoSubcommand::Fmt { .. }
+            | DenoSubcommand::Init { .. }
+            | DenoSubcommand::Info { .. }
+            | DenoSubcommand::JSONReference { .. }
+            | DenoSubcommand::Jupyter { .. }
+            | DenoSubcommand::Lsp
+            | DenoSubcommand::Lint { .. }
+            | DenoSubcommand::Repl { .. }
+            | DenoSubcommand::Run { .. }
+            | DenoSubcommand::Serve { .. }
+            | DenoSubcommand::Task { .. }
+            | DenoSubcommand::Test { .. }
+            | DenoSubcommand::Outdated { .. }
+            | DenoSubcommand::Types
+            | DenoSubcommand::Upgrade { .. }
+            | DenoSubcommand::Vendor
+            | DenoSubcommand::Publish { .. }
+            | DenoSubcommand::Help { .. }
+            | DenoSubcommand::X { .. } => false,
+          },
           cache_setting: NpmCacheSetting::from_cache_setting(
             &cli_options.cache_setting(),
           ),

cli/factory.rs

@@ -1487,6 +1487,7 @@ impl ConfigData {
       pb,
       None,
       NpmInstallerFactoryOptions {
+        clean_on_install: false,
         cache_setting: NpmCacheSetting::Use,
         caching_strategy: NpmCachingStrategy::Eager,
         lifecycle_scripts_config: LifecycleScriptsConfig::default(),

cli/lsp/config.rs

@@ -966,6 +966,7 @@ impl<'a> ResolverFactory<'a> {
         sys.clone(),
         tarball_cache.clone(),
         deno_npm_installer::NpmInstallerOptions {
+          clean_on_install: false,
           maybe_lockfile,
           maybe_node_modules_path: maybe_node_modules_path.clone(),
           lifecycle_scripts: Arc::new(LifecycleScriptsConfig::default()),

cli/lsp/resolver.rs

@@ -34,6 +34,7 @@ use deno_runtime::deno_io::FromRawIoHandle;
 use deno_semver::package::PackageNv;
 use deno_semver::package::PackageReq;
 use deno_task_shell::KillSignal;
+use sys_traits::PathsInErrorsExt;
 
 use crate::file_fetcher::CliFileFetcher;
 use crate::http_util::HttpClientProvider;
@@ -268,9 +269,6 @@ pub enum DenoTaskLifecycleScriptsError {
   #[error(transparent)]
   Io(#[from] std::io::Error),
   #[class(inherit)]
-  #[error(transparent)]
-  BinEntries(#[from] deno_npm_installer::BinEntriesError),
-  #[class(inherit)]
   #[error(
     "failed to create npm process state tempfile for running lifecycle scripts"
   )]
@@ -296,7 +294,7 @@ impl LifecycleScriptsExecutor for DenoTaskLifeCycleScriptsExecutor {
   ) -> Result<(), AnyError> {
     let mut failed_packages = Vec::new();
     let sys = CliSys::default();
-    let mut bin_entries = BinEntries::new(&sys);
+    let mut bin_entries = BinEntries::new(sys.with_paths_in_errors());
     // get custom commands for each bin available in the node_modules dir (essentially
     // the scripts that are in `node_modules/.bin`)
     let base = self
@@ -567,7 +565,7 @@ impl DenoTaskLifeCycleScriptsExecutor {
     snapshot: &NpmResolutionSnapshot,
   ) -> crate::task_runner::TaskCustomCommands {
     let sys = CliSys::default();
-    let mut bin_entries = BinEntries::new(&sys);
+    let mut bin_entries = BinEntries::new(sys.with_paths_in_errors());
     self
       .resolve_custom_commands_from_packages(
         extra_info_provider,

cli/npm.rs

@@ -503,7 +503,7 @@ fn clean_node_modules(
     return Ok(());
   }
 
-  let keep_names = keep_pkgs
+  let keep_ids = keep_pkgs
     .iter()
     .map(deno_resolver::npm::get_package_folder_id_folder_name)
     .collect::<HashSet<_>>();
@@ -530,8 +530,9 @@ fn clean_node_modules(
   };
 
   // TODO(nathanwhit): this probably shouldn't reach directly into this code
+  let sys = CliSys::default();
   let mut setup_cache = deno_npm_installer::LocalSetupCache::load(
-    CliSys::default(),
+    sys.clone(),
     base.join(".setup-cache.bin"),
   );
 
@@ -542,7 +543,7 @@ fn clean_node_modules(
     }
     let file_name = entry.file_name();
     let file_name = file_name.to_string_lossy();
-    if keep_names.contains(file_name.as_ref()) || file_name == "node_modules" {
+    if keep_ids.contains(file_name.as_ref()) || file_name == "node_modules" {
       continue;
     } else if dry_run {
       #[allow(clippy::print_stderr)]
@@ -554,78 +555,51 @@ fn clean_node_modules(
     }
   }
 
+  let mut remove_symlink = |path: &Path| -> std::io::Result<()> {
+    if dry_run {
+      #[allow(clippy::print_stderr)]
+      {
+        eprintln!(" {}", path.display());
+      }
+      Ok(())
+    } else {
+      cleaner.remove_file(path, None)
+    }
+  };
+
   // remove top level symlinks from node_modules/<package> to node_modules/.deno/<package>
   // where the target doesn't exist (because it was removed above)
-  clean_node_modules_symlinks(
-    cleaner,
-    &keep_names,
+  deno_npm_installer::remove_unused_node_modules_symlinks(
+    &sys,
     dir,
-    dry_run,
-    &mut |name| {
+    &keep_ids,
+    &mut |name, path| {
       setup_cache.remove_root_symlink(name);
+      remove_symlink(path)
     },
-  )?;
+  )
+  .map_err(AnyError::from)?;
 
   // remove symlinks from node_modules/.deno/node_modules/<package> to node_modules/.deno/<package>
   // where the target doesn't exist (because it was removed above)
-  clean_node_modules_symlinks(
-    cleaner,
-    &keep_names,
-    &base.join("node_modules"),
-    dry_run,
-    &mut |name| {
+  let deno_nm = base.join("node_modules");
+  deno_npm_installer::remove_unused_node_modules_symlinks(
+    &sys,
+    &deno_nm,
+    &keep_ids,
+    &mut |name, path| {
       setup_cache.remove_deno_symlink(name);
+      remove_symlink(path)
     },
-  )?;
+  )
+  .map_err(AnyError::from)?;
   if !dry_run {
     setup_cache.save();
   }
 
   Ok(())
 }
 
-// node_modules/.deno/chalk@5.0.1/node_modules/chalk -> chalk@5.0.1
-fn node_modules_package_actual_dir_to_name(
-  path: &Path,
-) -> Option<Cow<'_, str>> {
-  path
-    .parent()?
-    .parent()?
-    .file_name()
-    .map(|name| name.to_string_lossy())
-}
-
-fn clean_node_modules_symlinks(
-  cleaner: &mut FsCleaner,
-  keep_names: &HashSet<String>,
-  dir: &Path,
-  dry_run: bool,
-  on_remove: &mut dyn FnMut(&str),
-) -> Result<(), AnyError> {
-  for entry in std::fs::read_dir(dir)? {
-    let entry = entry?;
-    let ty = entry.file_type()?;
-    if ty.is_symlink() {
-      let target = std::fs::read_link(entry.path())?;
-      let name = node_modules_package_actual_dir_to_name(&target);
-      if let Some(name) = name
-        && !keep_names.contains(&*name)
-      {
-        if dry_run {
-          #[allow(clippy::print_stderr)]
-          {
-            eprintln!(" {}", entry.path().display());
-          }
-        } else {
-          on_remove(&name);
-          cleaner.remove_file(&entry.path(), None)?;
-        }
-      }
-    }
-  }
-  Ok(())
-}
-
 #[cfg(test)]
 mod tests {
   use std::path::Path;

cli/tools/clean.rs

@@ -11,9 +11,10 @@ use deno_config::glob::PathOrPattern;
 use deno_config::glob::PathOrPatternSet;
 use deno_config::glob::WalkEntry;
 use deno_core::ModuleSpecifier;
-use deno_core::anyhow::Context;
 use deno_core::anyhow::anyhow;
 use deno_core::error::AnyError;
+use sys_traits::FsMetadataValue;
+use sys_traits::PathsInErrorsExt;
 
 use super::progress_bar::UpdateGuard;
 use crate::sys::CliSys;
@@ -195,14 +196,16 @@ impl FsCleaner {
     }
   }
 
-  pub fn rm_rf(&mut self, path: &Path) -> Result<(), AnyError> {
+  pub fn rm_rf(&mut self, path: &Path) -> Result<(), std::io::Error> {
+    let sys = CliSys::default();
+    let sys = sys.with_paths_in_errors();
     for entry in walkdir::WalkDir::new(path).contents_first(true) {
-      let entry = entry?;
+      let entry = entry.map_err(std::io::Error::other)?;
 
       if entry.file_type().is_dir() {
         self.dirs_removed += 1;
         self.update_progress();
-        std::fs::remove_dir_all(entry.path())?;
+        sys.fs_remove_dir_all(entry.path())?;
       } else {
         self.remove_file(entry.path(), entry.metadata().ok())?;
       }
@@ -215,29 +218,13 @@ impl FsCleaner {
     &mut self,
     path: &Path,
     meta: Option<std::fs::Metadata>,
-  ) -> Result<(), AnyError> {
+  ) -> Result<(), std::io::Error> {
     if let Some(meta) = meta {
       self.bytes_removed += meta.len();
     }
     self.files_removed += 1;
     self.update_progress();
-    match std::fs::remove_file(path)
-      .with_context(|| format!("Failed to remove file: {}", path.display()))
-    {
-      Err(e) => {
-        if cfg!(windows)
-          && let Ok(meta) = path.symlink_metadata()
-          && meta.is_symlink()
-        {
-          std::fs::remove_dir(path).with_context(|| {
-            format!("Failed to remove symlink: {}", path.display())
-          })?;
-          return Ok(());
-        }
-        Err(e)
-      }
-      _ => Ok(()),
-    }
+    remove_file_or_symlink(&CliSys::default(), path)
   }
 
   fn update_progress(&self) {
@@ -247,6 +234,30 @@ impl FsCleaner {
   }
 }
 
+pub fn remove_file_or_symlink<
+  TSys: sys_traits::BaseFsRemoveFile
+    + sys_traits::BaseFsRemoveDir
+    + sys_traits::BaseFsMetadata,
+>(
+  sys: &TSys,
+  path: &Path,
+) -> Result<(), Error> {
+  let sys = sys.with_paths_in_errors();
+  match sys.fs_remove_file(path) {
+    Err(e) => {
+      if sys_traits::impls::is_windows()
+        && let Ok(meta) = sys.fs_symlink_metadata(path)
+        && meta.file_type().is_symlink()
+      {
+        sys.fs_remove_dir(path)?;
+        return Ok(());
+      }
+      Err(e)
+    }
+    _ => Ok(()),
+  }
+}
+
 #[cfg(test)]
 mod tests {
   use pretty_assertions::assert_eq;