fix(npm): lazily install package.json dependencies only when necessary (#17931)

This lazily does an "npm install" when any package name matches what's
found in the package.json or when running a script from package.json
with deno task.

Part of #17916

Closes #17928

cli/cache/mod.rs

@@ -2,7 +2,6 @@
 
 use crate::errors::get_error_class_name;
 use crate::file_fetcher::FileFetcher;
-use crate::util::fs::canonicalize_path;
 
 use deno_core::futures;
 use deno_core::futures::FutureExt;
@@ -108,12 +107,8 @@ impl Loader for FetchCacher {
       // is in a node_modules dir to avoid needlessly canonicalizing, then compare
       // against the canonicalized specifier.
       if specifier.path().contains("/node_modules/") {
-        let specifier = specifier
-          .to_file_path()
-          .ok()
-          .and_then(|path| canonicalize_path(&path).ok())
-          .and_then(|path| ModuleSpecifier::from_file_path(path).ok())
-          .unwrap_or_else(|| specifier.clone());
+        let specifier =
+          crate::node::resolve_specifier_into_node_modules(specifier);
         if specifier.as_str().starts_with(node_modules_url.as_str()) {
           return Box::pin(futures::future::ready(Ok(Some(
             LoadResponse::External { specifier },

cli/graph_util.rs

@@ -161,17 +161,13 @@ pub async fn create_graph_and_maybe_check(
     ps.options.node_modules_dir_specifier(),
   );
   let maybe_imports = ps.options.to_maybe_imports()?;
-  let maybe_package_json_deps = ps.options.maybe_package_json_deps()?;
-  ps.npm_resolver
-    .add_package_json_deps(maybe_package_json_deps.as_ref())
-    .await?;
   let cli_resolver = CliGraphResolver::new(
     ps.options.to_maybe_jsx_import_source_config(),
     ps.maybe_import_map.clone(),
     ps.options.no_npm(),
     ps.npm_resolver.api().clone(),
     ps.npm_resolver.resolution().clone(),
-    maybe_package_json_deps,
+    ps.package_json_deps_installer.clone(),
   );
   let graph_resolver = cli_resolver.as_graph_resolver();
   let graph_npm_resolver = cli_resolver.as_graph_npm_resolver();

cli/lsp/documents.rs

@@ -21,6 +21,7 @@ use crate::node::NodeResolution;
 use crate::npm::NpmPackageResolver;
 use crate::npm::NpmRegistryApi;
 use crate::npm::NpmResolution;
+use crate::npm::PackageJsonDepsInstaller;
 use crate::resolver::CliGraphResolver;
 use crate::util::path::specifier_to_file_path;
 use crate::util::text_encoding;
@@ -1212,13 +1213,18 @@ impl Documents {
         None => Vec::new(),
       }
     });
+    let deps_installer = PackageJsonDepsInstaller::new(
+      npm_registry_api.clone(),
+      npm_resolution.clone(),
+      maybe_package_json_deps,
+    );
     self.resolver = CliGraphResolver::new(
       maybe_jsx_config,
       maybe_import_map,
       false,
       npm_registry_api,
       npm_resolution,
-      maybe_package_json_deps,
+      deps_installer,
     );
     self.imports = Arc::new(
       if let Some(Ok(imports)) =

cli/node/mod.rs

@@ -40,6 +40,7 @@ use regex::Regex;
 use crate::cache::NodeAnalysisCache;
 use crate::file_fetcher::FileFetcher;
 use crate::npm::NpmPackageResolver;
+use crate::util::fs::canonicalize_path_maybe_not_exists;
 
 mod analyze;
 
@@ -285,6 +286,25 @@ pub fn node_resolve_npm_reference(
   Ok(Some(resolve_response))
 }
 
+/// Resolves a specifier that is pointing into a node_modules folder.
+///
+/// Note: This should be called whenever getting the specifier from
+/// a Module::External(module) reference because that module might
+/// not be fully resolved at the time deno_graph is analyzing it
+/// because the node_modules folder might not exist at that time.
+pub fn resolve_specifier_into_node_modules(
+  specifier: &ModuleSpecifier,
+) -> ModuleSpecifier {
+  specifier
+    .to_file_path()
+    .ok()
+    // this path might not exist at the time the graph is being created
+    // because the node_modules folder might not yet exist
+    .and_then(|path| canonicalize_path_maybe_not_exists(&path).ok())
+    .and_then(|path| ModuleSpecifier::from_file_path(path).ok())
+    .unwrap_or_else(|| specifier.clone())
+}
+
 pub fn node_resolve_binary_commands(
   pkg_nv: &NpmPackageNv,
   npm_resolver: &NpmPackageResolver,

cli/npm/installer.rs

@@ -0,0 +1,88 @@
+// Copyright 2018-2023 the Deno authors. All rights reserved. MIT license.
+
+use std::collections::BTreeMap;
+use std::sync::atomic::AtomicBool;
+use std::sync::Arc;
+
+use deno_core::error::AnyError;
+use deno_graph::npm::NpmPackageReq;
+
+use super::NpmRegistryApi;
+use super::NpmResolution;
+
+#[derive(Debug)]
+struct PackageJsonDepsInstallerInner {
+  has_installed: AtomicBool,
+  npm_registry_api: NpmRegistryApi,
+  npm_resolution: NpmResolution,
+  package_deps: BTreeMap<String, NpmPackageReq>,
+}
+
+/// Holds and controls installing dependencies from package.json.
+#[derive(Debug, Clone, Default)]
+pub struct PackageJsonDepsInstaller(Option<Arc<PackageJsonDepsInstallerInner>>);
+
+impl PackageJsonDepsInstaller {
+  pub fn new(
+    npm_registry_api: NpmRegistryApi,
+    npm_resolution: NpmResolution,
+    deps: Option<BTreeMap<String, NpmPackageReq>>,
+  ) -> Self {
+    Self(deps.map(|package_deps| {
+      Arc::new(PackageJsonDepsInstallerInner {
+        has_installed: AtomicBool::new(false),
+        npm_registry_api,
+        npm_resolution,
+        package_deps,
+      })
+    }))
+  }
+
+  pub fn package_deps(&self) -> Option<&BTreeMap<String, NpmPackageReq>> {
+    self.0.as_ref().map(|inner| &inner.package_deps)
+  }
+
+  /// Gets if the package.json has the specified package name.
+  pub fn has_package_name(&self, name: &str) -> bool {
+    if let Some(package_deps) = self.package_deps() {
+      // ensure this looks at the package name and not the
+      // bare specifiers (do not look at the keys!)
+      package_deps.values().any(|v| v.name == name)
+    } else {
+      false
+    }
+  }
+
+  /// Installs the top level dependencies in the package.json file
+  /// without going through and resolving the descendant dependencies yet.
+  pub async fn ensure_top_level_install(&self) -> Result<(), AnyError> {
+    use std::sync::atomic::Ordering;
+    let inner = match &self.0 {
+      Some(inner) => inner,
+      None => return Ok(()),
+    };
+
+    if inner.has_installed.swap(true, Ordering::SeqCst) {
+      return Ok(()); // already installed by something else
+    }
+
+    let mut package_reqs =
+      inner.package_deps.values().cloned().collect::<Vec<_>>();
+    package_reqs.sort(); // deterministic resolution
+
+    inner
+      .npm_registry_api
+      .cache_in_parallel(
+        package_reqs.iter().map(|req| req.name.clone()).collect(),
+      )
+      .await?;
+
+    for package_req in package_reqs {
+      inner
+        .npm_resolution
+        .resolve_package_req_as_pending(&package_req)?;
+    }
+
+    Ok(())
+  }
+}

cli/npm/mod.rs

@@ -1,6 +1,7 @@
 // Copyright 2018-2023 the Deno authors. All rights reserved. MIT license.
 
 mod cache;
+mod installer;
 mod registry;
 mod resolution;
 mod resolvers;
@@ -8,6 +9,7 @@ mod tarball;
 
 pub use cache::should_sync_download;
 pub use cache::NpmCache;
+pub use installer::PackageJsonDepsInstaller;
 #[cfg(test)]
 pub use registry::NpmPackageVersionDistInfo;
 pub use registry::NpmRegistryApi;

cli/npm/resolution/mod.rs

@@ -404,8 +404,9 @@ impl NpmResolution {
   }
 
   /// Resolves a package requirement for deno graph. This should only be
-  /// called by deno_graph's NpmResolver.
-  pub fn resolve_package_req_for_deno_graph(
+  /// called by deno_graph's NpmResolver or for resolving packages in
+  /// a package.json
+  pub fn resolve_package_req_as_pending(
     &self,
     pkg_req: &NpmPackageReq,
   ) -> Result<NpmPackageNv, AnyError> {

cli/npm/resolvers/mod.rs

@@ -19,7 +19,6 @@ use deno_runtime::deno_node::RequireNpmResolver;
 use global::GlobalNpmPackageResolver;
 use serde::Deserialize;
 use serde::Serialize;
-use std::collections::BTreeMap;
 use std::path::Path;
 use std::path::PathBuf;
 use std::sync::Arc;
@@ -224,19 +223,6 @@ impl NpmPackageResolver {
     self.resolution.has_packages()
   }
 
-  /// Adds the package reqs from a package.json if they exist.
-  pub async fn add_package_json_deps(
-    &self,
-    maybe_package_json_deps: Option<&BTreeMap<String, NpmPackageReq>>,
-  ) -> Result<(), AnyError> {
-    if let Some(deps) = maybe_package_json_deps {
-      let mut package_reqs = deps.values().cloned().collect::<Vec<_>>();
-      package_reqs.sort(); // deterministic resolution
-      self.add_package_reqs(package_reqs).await?;
-    }
-    Ok(())
-  }
-
   /// Adds package requirements to the resolver and ensures everything is setup.
   pub async fn add_package_reqs(
     &self,

cli/proc_state.rs

@@ -27,6 +27,7 @@ use crate::node::NodeResolution;
 use crate::npm::NpmCache;
 use crate::npm::NpmPackageResolver;
 use crate::npm::NpmRegistryApi;
+use crate::npm::PackageJsonDepsInstaller;
 use crate::resolver::CliGraphResolver;
 use crate::tools::check;
 use crate::util::progress_bar::ProgressBar;
@@ -92,6 +93,7 @@ pub struct Inner {
   pub node_analysis_cache: NodeAnalysisCache,
   pub npm_cache: NpmCache,
   pub npm_resolver: NpmPackageResolver,
+  pub package_json_deps_installer: PackageJsonDepsInstaller,
   pub cjs_resolutions: Mutex<HashSet<ModuleSpecifier>>,
   progress_bar: ProgressBar,
 }
@@ -153,6 +155,7 @@ impl ProcState {
       node_analysis_cache: self.node_analysis_cache.clone(),
       npm_cache: self.npm_cache.clone(),
       npm_resolver: self.npm_resolver.clone(),
+      package_json_deps_installer: self.package_json_deps_installer.clone(),
       cjs_resolutions: Default::default(),
       progress_bar: self.progress_bar.clone(),
     });
@@ -228,6 +231,11 @@ impl ProcState {
       lockfile.as_ref().cloned(),
     )
     .await?;
+    let package_json_deps_installer = PackageJsonDepsInstaller::new(
+      npm_resolver.api().clone(),
+      npm_resolver.resolution().clone(),
+      cli_options.maybe_package_json_deps()?,
+    );
     let maybe_import_map = cli_options
       .resolve_import_map(&file_fetcher)
       .await?
@@ -235,18 +243,13 @@ impl ProcState {
     let maybe_inspector_server =
       cli_options.resolve_inspector_server().map(Arc::new);
 
-    let maybe_package_json_deps = cli_options.maybe_package_json_deps()?;
-    // resolve the package.json npm requirements ahead of time
-    npm_resolver
-      .add_package_json_deps(maybe_package_json_deps.as_ref())
-      .await?;
     let resolver = Arc::new(CliGraphResolver::new(
       cli_options.to_maybe_jsx_import_source_config(),
       maybe_import_map.clone(),
       cli_options.no_npm(),
       npm_resolver.api().clone(),
       npm_resolver.resolution().clone(),
-      maybe_package_json_deps,
+      package_json_deps_installer.clone(),
     ));
 
     let maybe_file_watcher_reporter =
@@ -298,6 +301,7 @@ impl ProcState {
       node_analysis_cache,
       npm_cache,
       npm_resolver,
+      package_json_deps_installer,
       cjs_resolutions: Default::default(),
       progress_bar,
     })))
@@ -514,8 +518,10 @@ impl ProcState {
               node::resolve_builtin_node_module(&module.module_name)
             }
             Some(Module::Esm(module)) => Ok(module.specifier.clone()),
-            Some(Module::External(module)) => Ok(module.specifier.clone()),
             Some(Module::Json(module)) => Ok(module.specifier.clone()),
+            Some(Module::External(module)) => {
+              Ok(node::resolve_specifier_into_node_modules(&module.specifier))
+            }
             None => Ok(specifier.clone()),
           };
         }
@@ -631,18 +637,13 @@ impl ProcState {
   ) -> Result<deno_graph::ModuleGraph, AnyError> {
     let maybe_imports = self.options.to_maybe_imports()?;
 
-    let maybe_package_json_deps = self.options.maybe_package_json_deps()?;
-    self
-      .npm_resolver
-      .add_package_json_deps(maybe_package_json_deps.as_ref())
-      .await?;
     let cli_resolver = CliGraphResolver::new(
       self.options.to_maybe_jsx_import_source_config(),
       self.maybe_import_map.clone(),
       self.options.no_npm(),
       self.npm_resolver.api().clone(),
       self.npm_resolver.resolution().clone(),
-      maybe_package_json_deps,
+      self.package_json_deps_installer.clone(),
     );
     let graph_resolver = cli_resolver.as_graph_resolver();
     let graph_npm_resolver = cli_resolver.as_graph_npm_resolver();

cli/resolver.rs

@@ -20,18 +20,19 @@ use std::sync::Arc;
 use crate::args::JsxImportSourceConfig;
 use crate::npm::NpmRegistryApi;
 use crate::npm::NpmResolution;
+use crate::npm::PackageJsonDepsInstaller;
 
 /// A resolver that takes care of resolution, taking into account loaded
 /// import map, JSX settings.
 #[derive(Debug, Clone)]
 pub struct CliGraphResolver {
   maybe_import_map: Option<Arc<ImportMap>>,
-  maybe_package_json_deps: Option<BTreeMap<String, NpmPackageReq>>,
   maybe_default_jsx_import_source: Option<String>,
   maybe_jsx_import_source_module: Option<String>,
   no_npm: bool,
   npm_registry_api: NpmRegistryApi,
   npm_resolution: NpmResolution,
+  package_json_deps_installer: PackageJsonDepsInstaller,
   sync_download_semaphore: Option<Arc<tokio::sync::Semaphore>>,
 }
 
@@ -49,7 +50,7 @@ impl Default for CliGraphResolver {
       no_npm: false,
       npm_registry_api,
       npm_resolution,
-      maybe_package_json_deps: Default::default(),
+      package_json_deps_installer: Default::default(),
       sync_download_semaphore: Self::create_sync_download_semaphore(),
     }
   }
@@ -62,7 +63,7 @@ impl CliGraphResolver {
     no_npm: bool,
     npm_registry_api: NpmRegistryApi,
     npm_resolution: NpmResolution,
-    maybe_package_json_deps: Option<BTreeMap<String, NpmPackageReq>>,
+    package_json_deps_installer: PackageJsonDepsInstaller,
   ) -> Self {
     Self {
       maybe_import_map,
@@ -74,7 +75,7 @@ impl CliGraphResolver {
       no_npm,
       npm_registry_api,
       npm_resolution,
-      maybe_package_json_deps,
+      package_json_deps_installer,
       sync_download_semaphore: Self::create_sync_download_semaphore(),
     }
   }
@@ -125,7 +126,8 @@ impl Resolver for CliGraphResolver {
     };
 
     // then with package.json
-    if let Some(deps) = self.maybe_package_json_deps.as_ref() {
+    if let Some(deps) = self.package_json_deps_installer.package_deps().as_ref()
+    {
       if let Some(specifier) = resolve_package_json_dep(specifier, deps)? {
         return Ok(specifier);
       }
@@ -187,17 +189,37 @@ impl NpmResolver for CliGraphResolver {
     // this will internally cache the package information
     let package_name = package_name.to_string();
     let api = self.npm_registry_api.clone();
-    let mut maybe_sync_download_semaphore =
-      self.sync_download_semaphore.clone();
+    let deps_installer = self.package_json_deps_installer.clone();
+    let maybe_sync_download_semaphore = self.sync_download_semaphore.clone();
     async move {
-      let result = if let Some(semaphore) = maybe_sync_download_semaphore.take()
-      {
-        let _permit = semaphore.acquire().await.unwrap();
-        api.package_info(&package_name).await
+      let permit = if let Some(semaphore) = &maybe_sync_download_semaphore {
+        Some(semaphore.acquire().await.unwrap())
       } else {
-        api.package_info(&package_name).await
+        None
       };
-      result.map(|_| ()).map_err(|err| format!("{err:#}"))
+
+      // trigger an npm install if the package name matches
+      // a package in the package.json
+      //
+      // todo(dsherret): ideally this would only download if a bare
+      // specifiy matched in the package.json, but deno_graph only
+      // calls this once per package name and we might resolve an
+      // npm specifier first which calls this, then a bare specifier
+      // second and that would cause this not to occur.
+      if deps_installer.has_package_name(&package_name) {
+        deps_installer
+          .ensure_top_level_install()
+          .await
+          .map_err(|err| format!("{err:#}"))?;
+      }
+
+      let result = api
+        .package_info(&package_name)
+        .await
+        .map(|_| ())
+        .map_err(|err| format!("{err:#}"));
+      drop(permit);
+      result
     }
     .boxed()
   }
@@ -211,7 +233,7 @@ impl NpmResolver for CliGraphResolver {
     }
     self
       .npm_resolution
-      .resolve_package_req_for_deno_graph(package_req)
+      .resolve_package_req_as_pending(package_req)
   }
 }
 

cli/standalone.rs

@@ -243,7 +243,7 @@ pub async fn run(
           false,
           ps.npm_resolver.api().clone(),
           ps.npm_resolver.resolution().clone(),
-          None,
+          ps.package_json_deps_installer.clone(),
         )
       },
     ),

cli/tests/integration/npm_tests.rs

@@ -1569,10 +1569,10 @@ itest!(create_require {
 itest!(node_modules_import_run {
   args: "run --quiet main.ts",
   output: "npm/node_modules_import/main.out",
-  envs: env_vars_for_npm_tests(),
   http_server: true,
-  cwd: Some("npm/node_modules_import/"),
   copy_temp_dir: Some("npm/node_modules_import/"),
+  cwd: Some("npm/node_modules_import/"),
+  envs: env_vars_for_npm_tests(),
   exit_code: 0,
 });
 

cli/tests/integration/run_tests.rs

@@ -2819,6 +2819,14 @@ itest!(package_json_auto_discovered_for_npm_binary {
   http_server: true,
 });
 
+itest!(package_json_auto_discovered_no_package_json_imports {
+  // this should not use --quiet because we should ensure no package.json install occurs
+  args: "run -A no_package_json_imports.ts",
+  output: "run/with_package_json/no_deno_json/no_package_json_imports.out",
+  cwd: Some("run/with_package_json/no_deno_json"),
+  copy_temp_dir: Some("run/with_package_json/no_deno_json"),
+});
+
 itest!(package_json_with_deno_json {
   args: "run --quiet -A main.ts",
   output: "package_json/deno_json/main.out",

cli/tests/testdata/run/with_package_json/no_deno_json/no_package_json_imports.out

@@ -0,0 +1 @@
+5

cli/tests/testdata/run/with_package_json/no_deno_json/no_package_json_imports.ts

@@ -0,0 +1 @@
+console.log(5);

cli/tests/testdata/run/with_package_json/no_deno_json/sub_dir/main.js

@@ -1,2 +1,3 @@
+import "chalk";
 console.log(Deno.cwd());
 console.log(Deno.statSync("../node_modules"));

cli/tests/testdata/task/both/deno_selected.out

@@ -1,4 +1,2 @@
-Download http://localhost:4545/npm/registry/@denotest/bin
-Download http://localhost:4545/npm/registry/@denotest/bin/1.0.0.tgz
 Task other deno eval 'console.log(2)'
 2

cli/tests/testdata/task/both/prefers_deno.out

@@ -1,4 +1,2 @@
-Download http://localhost:4545/npm/registry/@denotest/bin
-Download http://localhost:4545/npm/registry/@denotest/bin/1.0.0.tgz
 Task output deno eval 'console.log(1)' "some" "text"
 1

cli/tools/task.rs

@@ -60,6 +60,11 @@ pub async fn execute_script(
         .await;
     Ok(exit_code)
   } else if let Some(script) = package_json_scripts.get(task_name) {
+    ps.package_json_deps_installer
+      .ensure_top_level_install()
+      .await?;
+    ps.npm_resolver.resolve_pending().await?;
+
     let cwd = match task_flags.cwd {
       Some(path) => canonicalize_path(&PathBuf::from(path))?,
       None => maybe_package_json

cli/tools/vendor/test.rs

@@ -22,6 +22,7 @@ use import_map::ImportMap;
 use crate::cache::ParsedSourceCache;
 use crate::npm::NpmRegistryApi;
 use crate::npm::NpmResolution;
+use crate::npm::PackageJsonDepsInstaller;
 use crate::resolver::CliGraphResolver;
 
 use super::build::VendorEnvironment;
@@ -266,13 +267,18 @@ async fn build_test_graph(
     let npm_registry_api = NpmRegistryApi::new_uninitialized();
     let npm_resolution =
       NpmResolution::new(npm_registry_api.clone(), None, None);
+    let deps_installer = PackageJsonDepsInstaller::new(
+      npm_registry_api.clone(),
+      npm_resolution.clone(),
+      None,
+    );
     CliGraphResolver::new(
       None,
       Some(Arc::new(m)),
       false,
       npm_registry_api,
       npm_resolution,
-      None,
+      deps_installer,
     )
   });
   let mut graph = ModuleGraph::default();

cli/tsc/mod.rs

@@ -559,7 +559,9 @@ fn op_load(state: &mut OpState, args: Value) -> Result<Value, AnyError> {
         Module::Npm(_) | Module::Node(_) => None,
         Module::External(module) => {
           // means it's Deno code importing an npm module
-          media_type = MediaType::from(&module.specifier);
+          let specifier =
+            node::resolve_specifier_into_node_modules(&module.specifier);
+          media_type = MediaType::from(&specifier);
           let file_path = specifier.to_file_path().unwrap();
           let code =
             std::fs::read_to_string(&file_path).with_context(|| {
@@ -731,9 +733,10 @@ fn resolve_graph_specifier_types(
     Some(Module::External(module)) => {
       // we currently only use "External" for when the module is in an npm package
       Ok(state.maybe_npm_resolver.as_ref().map(|npm_resolver| {
+        let specifier =
+          node::resolve_specifier_into_node_modules(&module.specifier);
         NodeResolution::into_specifier_and_media_type(
-          node::url_to_node_resolution(module.specifier.clone(), npm_resolver)
-            .ok(),
+          node::url_to_node_resolution(specifier, npm_resolver).ok(),
         )
       }))
     }