fix(npm): resolve dynamic npm imports individually (#24170)

* https://github.com/denoland/deno_npm/pull/57
* https://github.com/denoland/deno_graph/pull/498

Closes https://github.com/denoland/deno/issues/17802

Cargo.lock

@@ -1481,9 +1481,9 @@ dependencies = [
 
 [[package]]
 name = "deno_graph"
-version = "0.78.0"
+version = "0.78.1"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "c4ccd2755a805983f96aeccd211c1f7585b6bfec77471f502c47227abe375682"
+checksum = "d13080829a06062a14e41e190f64a3407e4a0f63cf7db5dcecbc3cf500445df3"
 dependencies = [
  "anyhow",
  "async-trait",
@@ -1740,9 +1740,9 @@ dependencies = [
 
 [[package]]
 name = "deno_npm"
-version = "0.21.3"
+version = "0.21.4"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "a78b95f0daab64d3cfb28b13be2b40d73c8b9563bbce3aa50fc322a7325ce0b9"
+checksum = "9812c781ff6b2e0e45c32ccba9983bce84ecccf6f6a7006b750f8c5c9ac15e30"
 dependencies = [
  "anyhow",
  "async-trait",

cli/Cargo.toml

@@ -69,10 +69,10 @@ deno_config = "=0.16.4"
 deno_core = { workspace = true, features = ["include_js_files_for_snapshotting"] }
 deno_doc = { version = "=0.139.0", features = ["html", "syntect"] }
 deno_emit = "=0.42.0"
-deno_graph = { version = "=0.78.0", features = ["tokio_executor"] }
+deno_graph = { version = "=0.78.1", features = ["tokio_executor"] }
 deno_lint = { version = "=0.60.0", features = ["docs"] }
 deno_lockfile.workspace = true
-deno_npm = "=0.21.3"
+deno_npm = "=0.21.4"
 deno_runtime = { workspace = true, features = ["include_js_files_for_snapshotting"] }
 deno_semver = "=0.5.4"
 deno_task_shell = "=0.16.1"

cli/factory.rs

@@ -35,7 +35,6 @@ use crate::npm::CliNpmResolver;
 use crate::npm::CliNpmResolverByonmCreateOptions;
 use crate::npm::CliNpmResolverCreateOptions;
 use crate::npm::CliNpmResolverManagedCreateOptions;
-use crate::npm::CliNpmResolverManagedPackageJsonInstallerOption;
 use crate::npm::CliNpmResolverManagedSnapshotOption;
 use crate::resolver::CjsResolutionStore;
 use crate::resolver::CliGraphResolver;
@@ -441,10 +440,8 @@ impl CliFactory {
             cache_setting: self.options.cache_setting(),
             text_only_progress_bar: self.text_only_progress_bar().clone(),
             maybe_node_modules_path: self.options.node_modules_dir_path().cloned(),
-            package_json_installer:
+            package_json_deps_provider:
-              CliNpmResolverManagedPackageJsonInstallerOption::ConditionalInstall(
                 self.package_json_deps_provider().clone(),
-              ),
             npm_system_info: self.options.npm_system_info(),
             npmrc: self.options.npmrc().clone()
           })

cli/graph_util.rs

@@ -160,6 +160,10 @@ pub fn graph_valid(
   if let Some(error) = errors.next() {
     Err(error)
   } else {
+    // finally surface the npm resolution result
+    if let Err(err) = &graph.npm_dep_graph_result {
+      return Err(custom_error(get_error_class_name(err), format!("{}", err)));
+    }
     Ok(())
   }
 }
@@ -562,30 +566,9 @@ impl ModuleGraphBuilder {
     let initial_redirects_len = graph.redirects.len();
     let initial_package_deps_len = graph.packages.package_deps_sum();
     let initial_package_mappings_len = graph.packages.mappings().len();
-    let initial_npm_packages = graph.npm_packages.len();
 
     graph.build(roots, loader, options).await;
 
-    let has_npm_packages_changed =
-      graph.npm_packages.len() != initial_npm_packages;
-    // skip installing npm packages if we don't have to
-    if is_first_execution
-      && self.npm_resolver.root_node_modules_path().is_some()
-      || has_npm_packages_changed
-    {
-      if let Some(npm_resolver) = self.npm_resolver.as_managed() {
-        // ensure that the top level package.json is installed if a
-        // specifier was matched in the package.json
-        if self.resolver.found_package_json_dep() {
-          npm_resolver.ensure_top_level_package_json_install().await?;
-        }
-
-        // resolve the dependencies of any pending dependencies
-        // that were inserted by building the graph
-        npm_resolver.resolve_pending().await?;
-      }
-    }
-
     let has_redirects_changed = graph.redirects.len() != initial_redirects_len;
     let has_jsr_package_deps_changed =
       graph.packages.package_deps_sum() != initial_package_deps_len;

cli/lsp/resolver.rs

@@ -12,7 +12,6 @@ use crate::npm::CliNpmResolver;
 use crate::npm::CliNpmResolverByonmCreateOptions;
 use crate::npm::CliNpmResolverCreateOptions;
 use crate::npm::CliNpmResolverManagedCreateOptions;
-use crate::npm::CliNpmResolverManagedPackageJsonInstallerOption;
 use crate::npm::CliNpmResolverManagedSnapshotOption;
 use crate::npm::ManagedCliNpmResolver;
 use crate::resolver::CliGraphResolver;
@@ -347,9 +346,11 @@ async fn create_npm_resolver(
       cache_setting: CacheSetting::Only,
       text_only_progress_bar: ProgressBar::new(ProgressBarStyle::TextOnly),
       maybe_node_modules_path: config_data.node_modules_dir.clone(),
-      // do not install while resolving in the lsp—leave that to the cache command
+      package_json_deps_provider: Arc::new(PackageJsonDepsProvider::new(
-      package_json_installer:
+        config_data.package_json.as_ref().map(|package_json| {
-        CliNpmResolverManagedPackageJsonInstallerOption::NoInstall,
+          package_json::get_local_package_json_version_reqs(package_json)
+        }),
+      )),
       npmrc: config_data
         .npmrc
         .clone()

cli/module_loader.rs

@@ -75,7 +75,6 @@ pub async fn load_top_level_deps(factory: &CliFactory) -> Result<(), AnyError> {
   let npm_resolver = factory.npm_resolver().await?;
   if let Some(npm_resolver) = npm_resolver.as_managed() {
     npm_resolver.ensure_top_level_package_json_install().await?;
-    npm_resolver.resolve_pending().await?;
   }
   // cache as many entries in the import map as we can
   if let Some(import_map) = factory.maybe_import_map().await? {

cli/npm/managed/installer.rs

@@ -1,123 +0,0 @@
-// Copyright 2018-2024 the Deno authors. All rights reserved. MIT license.
-
-use std::future::Future;
-use std::sync::Arc;
-
-use deno_core::error::AnyError;
-use deno_core::futures::stream::FuturesOrdered;
-use deno_core::futures::StreamExt;
-use deno_npm::registry::NpmRegistryApi;
-use deno_npm::registry::NpmRegistryPackageInfoLoadError;
-use deno_semver::package::PackageReq;
-
-use crate::args::PackageJsonDepsProvider;
-use crate::util::sync::AtomicFlag;
-
-use super::CliNpmRegistryApi;
-use super::NpmResolution;
-
-#[derive(Debug)]
-struct PackageJsonDepsInstallerInner {
-  deps_provider: Arc<PackageJsonDepsProvider>,
-  has_installed_flag: AtomicFlag,
-  npm_registry_api: Arc<CliNpmRegistryApi>,
-  npm_resolution: Arc<NpmResolution>,
-}
-
-impl PackageJsonDepsInstallerInner {
-  pub fn reqs_with_info_futures<'a>(
-    &self,
-    reqs: &'a [&'a PackageReq],
-  ) -> FuturesOrdered<
-    impl Future<
-      Output = Result<
-        (&'a PackageReq, Arc<deno_npm::registry::NpmPackageInfo>),
-        NpmRegistryPackageInfoLoadError,
-      >,
-    >,
-  > {
-    FuturesOrdered::from_iter(reqs.iter().map(|req| {
-      let api = self.npm_registry_api.clone();
-      async move {
-        let info = api.package_info(&req.name).await?;
-        Ok::<_, NpmRegistryPackageInfoLoadError>((*req, info))
-      }
-    }))
-  }
-}
-
-/// Holds and controls installing dependencies from package.json.
-#[derive(Debug, Default)]
-pub struct PackageJsonDepsInstaller(Option<PackageJsonDepsInstallerInner>);
-
-impl PackageJsonDepsInstaller {
-  pub fn new(
-    deps_provider: Arc<PackageJsonDepsProvider>,
-    npm_registry_api: Arc<CliNpmRegistryApi>,
-    npm_resolution: Arc<NpmResolution>,
-  ) -> Self {
-    Self(Some(PackageJsonDepsInstallerInner {
-      deps_provider,
-      has_installed_flag: Default::default(),
-      npm_registry_api,
-      npm_resolution,
-    }))
-  }
-
-  /// Creates an installer that never installs local packages during
-  /// resolution. A top level install will be a no-op.
-  pub fn no_op() -> Self {
-    Self(None)
-  }
-
-  /// 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> {
-    let inner = match &self.0 {
-      Some(inner) => inner,
-      None => return Ok(()),
-    };
-
-    if !inner.has_installed_flag.raise() {
-      return Ok(()); // already installed by something else
-    }
-
-    let package_reqs = inner.deps_provider.reqs().unwrap_or_default();
-
-    // check if something needs resolving before bothering to load all
-    // the package information (which is slow)
-    if package_reqs.iter().all(|req| {
-      inner
-        .npm_resolution
-        .resolve_pkg_id_from_pkg_req(req)
-        .is_ok()
-    }) {
-      log::debug!(
-        "All package.json deps resolvable. Skipping top level install."
-      );
-      return Ok(()); // everything is already resolvable
-    }
-
-    let mut reqs_with_info_futures =
-      inner.reqs_with_info_futures(&package_reqs);
-
-    while let Some(result) = reqs_with_info_futures.next().await {
-      let (req, info) = result?;
-      let result = inner
-        .npm_resolution
-        .resolve_pkg_req_as_pending_with_info(req, &info)
-        .await;
-      if let Err(err) = result {
-        if inner.npm_registry_api.mark_force_reload() {
-          log::debug!("Failed to resolve package. Retrying. Error: {err:#}");
-          // re-initialize
-          reqs_with_info_futures = inner.reqs_with_info_futures(&package_reqs);
-        } else {
-          return Err(err.into());
-        }
-      }
-    }
-
-    Ok(())
-  }
-}

cli/npm/managed/mod.rs

@@ -11,7 +11,6 @@ use deno_core::anyhow::Context;
 use deno_core::error::AnyError;
 use deno_core::parking_lot::Mutex;
 use deno_core::serde_json;
-use deno_graph::NpmPackageReqsResolution;
 use deno_npm::npm_rc::ResolvedNpmRc;
 use deno_npm::registry::NpmPackageInfo;
 use deno_npm::registry::NpmRegistryApi;
@@ -26,6 +25,7 @@ use deno_runtime::deno_node::NodePermissions;
 use deno_runtime::deno_node::NpmResolver;
 use deno_semver::package::PackageNv;
 use deno_semver::package::PackageReq;
+use resolution::AddPkgReqsResult;
 
 use crate::args::Lockfile;
 use crate::args::NpmProcessState;
@@ -35,9 +35,9 @@ use crate::cache::FastInsecureHasher;
 use crate::http_util::HttpClientProvider;
 use crate::util::fs::canonicalize_path_maybe_not_exists_with_fs;
 use crate::util::progress_bar::ProgressBar;
+use crate::util::sync::AtomicFlag;
 
 use self::cache::NpmCache;
-use self::installer::PackageJsonDepsInstaller;
 use self::registry::CliNpmRegistryApi;
 use self::resolution::NpmResolution;
 use self::resolvers::create_npm_fs_resolver;
@@ -48,7 +48,6 @@ use super::InnerCliNpmResolverRef;
 use super::NpmCacheDir;
 
 mod cache;
-mod installer;
 mod registry;
 mod resolution;
 mod resolvers;
@@ -58,11 +57,6 @@ pub enum CliNpmResolverManagedSnapshotOption {
   Specified(Option<ValidSerializedNpmResolutionSnapshot>),
 }
 
-pub enum CliNpmResolverManagedPackageJsonInstallerOption {
-  ConditionalInstall(Arc<PackageJsonDepsProvider>),
-  NoInstall,
-}
-
 pub struct CliNpmResolverManagedCreateOptions {
   pub snapshot: CliNpmResolverManagedSnapshotOption,
   pub maybe_lockfile: Option<Arc<Mutex<Lockfile>>>,
@@ -73,7 +67,7 @@ pub struct CliNpmResolverManagedCreateOptions {
   pub text_only_progress_bar: crate::util::progress_bar::ProgressBar,
   pub maybe_node_modules_path: Option<PathBuf>,
   pub npm_system_info: NpmSystemInfo,
-  pub package_json_installer: CliNpmResolverManagedPackageJsonInstallerOption,
+  pub package_json_deps_provider: Arc<PackageJsonDepsProvider>,
   pub npmrc: Arc<ResolvedNpmRc>,
 }
 
@@ -98,7 +92,7 @@ pub async fn create_managed_npm_resolver_for_lsp(
       npm_api,
       npm_cache,
       options.npmrc,
-      options.package_json_installer,
+      options.package_json_deps_provider,
       options.text_only_progress_bar,
       options.maybe_node_modules_path,
       options.npm_system_info,
@@ -122,7 +116,7 @@ pub async fn create_managed_npm_resolver(
     npm_api,
     npm_cache,
     options.npmrc,
-    options.package_json_installer,
+    options.package_json_deps_provider,
     options.text_only_progress_bar,
     options.maybe_node_modules_path,
     options.npm_system_info,
@@ -138,7 +132,7 @@ fn create_inner(
   npm_api: Arc<CliNpmRegistryApi>,
   npm_cache: Arc<NpmCache>,
   npm_rc: Arc<ResolvedNpmRc>,
-  package_json_installer: CliNpmResolverManagedPackageJsonInstallerOption,
+  package_json_deps_provider: Arc<PackageJsonDepsProvider>,
   text_only_progress_bar: crate::util::progress_bar::ProgressBar,
   node_modules_dir_path: Option<PathBuf>,
   npm_system_info: NpmSystemInfo,
@@ -165,25 +159,13 @@ fn create_inner(
     node_modules_dir_path,
     npm_system_info.clone(),
   );
-  let package_json_deps_installer = match package_json_installer {
-    CliNpmResolverManagedPackageJsonInstallerOption::ConditionalInstall(
-      provider,
-    ) => Arc::new(PackageJsonDepsInstaller::new(
-      provider,
-      npm_api.clone(),
-      resolution.clone(),
-    )),
-    CliNpmResolverManagedPackageJsonInstallerOption::NoInstall => {
-      Arc::new(PackageJsonDepsInstaller::no_op())
-    }
-  };
   Arc::new(ManagedCliNpmResolver::new(
     fs,
     fs_resolver,
     maybe_lockfile,
     npm_api,
     npm_cache,
-    package_json_deps_installer,
+    package_json_deps_provider,
     resolution,
     tarball_cache,
     text_only_progress_bar,
@@ -271,11 +253,12 @@ pub struct ManagedCliNpmResolver {
   maybe_lockfile: Option<Arc<Mutex<Lockfile>>>,
   npm_api: Arc<CliNpmRegistryApi>,
   npm_cache: Arc<NpmCache>,
-  package_json_deps_installer: Arc<PackageJsonDepsInstaller>,
+  package_json_deps_provider: Arc<PackageJsonDepsProvider>,
   resolution: Arc<NpmResolution>,
   tarball_cache: Arc<TarballCache>,
   text_only_progress_bar: ProgressBar,
   npm_system_info: NpmSystemInfo,
+  top_level_install_flag: AtomicFlag,
 }
 
 impl std::fmt::Debug for ManagedCliNpmResolver {
@@ -294,7 +277,7 @@ impl ManagedCliNpmResolver {
     maybe_lockfile: Option<Arc<Mutex<Lockfile>>>,
     npm_api: Arc<CliNpmRegistryApi>,
     npm_cache: Arc<NpmCache>,
-    package_json_deps_installer: Arc<PackageJsonDepsInstaller>,
+    package_json_deps_provider: Arc<PackageJsonDepsProvider>,
     resolution: Arc<NpmResolution>,
     tarball_cache: Arc<TarballCache>,
     text_only_progress_bar: ProgressBar,
@@ -306,11 +289,12 @@ impl ManagedCliNpmResolver {
       maybe_lockfile,
       npm_api,
       npm_cache,
-      package_json_deps_installer,
+      package_json_deps_provider,
       text_only_progress_bar,
       resolution,
       tarball_cache,
       npm_system_info,
+      top_level_install_flag: Default::default(),
     }
   }
 
@@ -385,14 +369,28 @@ impl ManagedCliNpmResolver {
     &self,
     packages: &[PackageReq],
   ) -> Result<(), AnyError> {
-    if packages.is_empty() {
+    let result = self.add_package_reqs_raw(packages).await;
-      return Ok(());
+    result.dependencies_result
   }
 
-    self.resolution.add_package_reqs(packages).await?;
+  pub async fn add_package_reqs_raw(
-    self.fs_resolver.cache_packages().await?;
+    &self,
+    packages: &[PackageReq],
+  ) -> AddPkgReqsResult {
+    if packages.is_empty() {
+      return AddPkgReqsResult {
+        dependencies_result: Ok(()),
+        results: vec![],
+      };
+    }
 
-    Ok(())
+    let mut result = self.resolution.add_package_reqs(packages).await;
+    if result.dependencies_result.is_ok() {
+      result.dependencies_result =
+        self.cache_packages().await.map_err(AnyError::from);
+    }
+
+    result
   }
 
   /// Sets package requirements to the resolver, removing old requirements and adding new ones.
@@ -422,51 +420,17 @@ impl ManagedCliNpmResolver {
     &self,
   ) -> Result<(), AnyError> {
     // add and ensure this isn't added to the lockfile
-    let package_reqs = vec![PackageReq::from_str("@types/node").unwrap()];
+    self
-    self.resolution.add_package_reqs(&package_reqs).await?;
+      .add_package_reqs(&[PackageReq::from_str("@types/node").unwrap()])
-    self.cache_packages().await?;
+      .await?;
 
     Ok(())
   }
 
-  pub async fn resolve_pending(&self) -> Result<(), AnyError> {
-    self.resolution.resolve_pending().await?;
-    self.cache_packages().await
-  }
-
   pub async fn cache_packages(&self) -> Result<(), AnyError> {
     self.fs_resolver.cache_packages().await
   }
 
-  /// Resolves package requirements for deno graph.
-  pub async fn resolve_npm_for_deno_graph(
-    &self,
-    reqs_with_pkg_infos: &[(&PackageReq, Arc<NpmPackageInfo>)],
-  ) -> NpmPackageReqsResolution {
-    let results = self
-      .resolution
-      .resolve_pkg_reqs_as_pending_with_info(reqs_with_pkg_infos)
-      .await;
-
-    let mut resolutions = Vec::with_capacity(results.len());
-    for result in results {
-      match result {
-        Ok(nv) => {
-          resolutions.push(Ok(nv));
-        }
-        Err(err) => {
-          if self.npm_api.mark_force_reload() {
-            log::debug!("Restarting npm specifier resolution to check for new registry information. Error: {:#}", err);
-            return NpmPackageReqsResolution::ReloadRegistryInfo;
-          } else {
-            resolutions.push(Err(Arc::new(err.into())));
-          }
-        }
-      }
-    }
-    NpmPackageReqsResolution::Resolutions(resolutions)
-  }
-
   pub fn resolve_pkg_folder_from_deno_module(
     &self,
     nv: &PackageNv,
@@ -485,10 +449,26 @@ impl ManagedCliNpmResolver {
   pub async fn ensure_top_level_package_json_install(
     &self,
   ) -> Result<(), AnyError> {
-    self
+    let Some(reqs) = self.package_json_deps_provider.reqs() else {
-      .package_json_deps_installer
+      return Ok(());
-      .ensure_top_level_install()
+    };
-      .await
+    if !self.top_level_install_flag.raise() {
+      return Ok(()); // already did this
+    }
+    // check if something needs resolving before bothering to load all
+    // the package information (which is slow)
+    if reqs
+      .iter()
+      .all(|req| self.resolution.resolve_pkg_id_from_pkg_req(req).is_ok())
+    {
+      log::debug!(
+        "All package.json deps resolvable. Skipping top level install."
+      );
+      return Ok(()); // everything is already resolvable
+    }
+
+    let reqs = reqs.into_iter().cloned().collect::<Vec<_>>();
+    self.add_package_reqs(&reqs).await
   }
 
   pub async fn cache_package_info(
@@ -582,7 +562,7 @@ impl CliNpmResolver for ManagedCliNpmResolver {
       self.maybe_lockfile.clone(),
       self.npm_api.clone(),
       self.npm_cache.clone(),
-      self.package_json_deps_installer.clone(),
+      self.package_json_deps_provider.clone(),
       npm_resolution,
       self.tarball_cache.clone(),
       self.text_only_progress_bar.clone(),

cli/npm/managed/resolution.rs

@@ -8,9 +8,7 @@ use deno_core::error::AnyError;
 use deno_core::parking_lot::Mutex;
 use deno_lockfile::NpmPackageDependencyLockfileInfo;
 use deno_lockfile::NpmPackageLockfileInfo;
-use deno_npm::registry::NpmPackageInfo;
 use deno_npm::registry::NpmRegistryApi;
-use deno_npm::resolution::NpmPackageVersionResolutionError;
 use deno_npm::resolution::NpmPackagesPartitioned;
 use deno_npm::resolution::NpmResolutionError;
 use deno_npm::resolution::NpmResolutionSnapshot;
@@ -34,6 +32,16 @@ use crate::util::sync::SyncReadAsyncWriteLock;
 
 use super::CliNpmRegistryApi;
 
+pub struct AddPkgReqsResult {
+  /// Results from adding the individual packages.
+  ///
+  /// The indexes of the results correspond to the indexes of the provided
+  /// package requirements.
+  pub results: Vec<Result<PackageNv, NpmResolutionError>>,
+  /// The final result of resolving and caching all the package requirements.
+  pub dependencies_result: Result<(), AnyError>,
+}
+
 /// Handles updating and storing npm resolution in memory where the underlying
 /// snapshot can be updated concurrently. Additionally handles updating the lockfile
 /// based on changes to the resolution.
@@ -80,20 +88,28 @@ impl NpmResolution {
   pub async fn add_package_reqs(
     &self,
     package_reqs: &[PackageReq],
-  ) -> Result<(), AnyError> {
+  ) -> AddPkgReqsResult {
     // only allow one thread in here at a time
     let snapshot_lock = self.snapshot.acquire().await;
-    let snapshot = add_package_reqs_to_snapshot(
+    let result = add_package_reqs_to_snapshot(
       &self.api,
       package_reqs,
       self.maybe_lockfile.clone(),
       || snapshot_lock.read().clone(),
     )
-    .await?;
+    .await;
 
+    AddPkgReqsResult {
+      results: result.results,
+      dependencies_result: match result.dep_graph_result {
+        Ok(snapshot) => {
           *snapshot_lock.write() = snapshot;
           Ok(())
         }
+        Err(err) => Err(err.into()),
+      },
+    }
+  }
 
   pub async fn set_package_reqs(
     &self,
@@ -121,24 +137,8 @@ impl NpmResolution {
         }
       },
     )
-    .await?;
+    .await
-
+    .into_result()?;
-    *snapshot_lock.write() = snapshot;
-
-    Ok(())
-  }
-
-  pub async fn resolve_pending(&self) -> Result<(), AnyError> {
-    // only allow one thread in here at a time
-    let snapshot_lock = self.snapshot.acquire().await;
-
-    let snapshot = add_package_reqs_to_snapshot(
-      &self.api,
-      &Vec::new(),
-      self.maybe_lockfile.clone(),
-      || snapshot_lock.read().clone(),
-    )
-    .await?;
 
     *snapshot_lock.write() = snapshot;
 
@@ -217,49 +217,6 @@ impl NpmResolution {
       .map(|pkg| pkg.id.clone())
   }
 
-  /// Resolves a package requirement for deno graph. This should only be
-  /// called by deno_graph's NpmResolver or for resolving packages in
-  /// a package.json
-  pub async fn resolve_pkg_req_as_pending_with_info(
-    &self,
-    pkg_req: &PackageReq,
-    pkg_info: &NpmPackageInfo,
-  ) -> Result<PackageNv, NpmPackageVersionResolutionError> {
-    debug_assert_eq!(pkg_req.name, pkg_info.name);
-    // only allow one thread in here at a time
-    let snapshot_lock = self.snapshot.acquire().await;
-
-    let mut snapshot = snapshot_lock.write();
-    let pending_resolver = get_npm_pending_resolver(&self.api);
-    let nv = pending_resolver.resolve_package_req_as_pending(
-      &mut snapshot,
-      pkg_req,
-      pkg_info,
-    )?;
-    Ok(nv)
-  }
-
-  pub async fn resolve_pkg_reqs_as_pending_with_info(
-    &self,
-    reqs_with_pkg_infos: &[(&PackageReq, Arc<NpmPackageInfo>)],
-  ) -> Vec<Result<PackageNv, NpmPackageVersionResolutionError>> {
-    // only allow one thread in here at a time
-    let snapshot_lock = self.snapshot.acquire().await;
-
-    let mut snapshot = snapshot_lock.write();
-    let pending_resolver = get_npm_pending_resolver(&self.api);
-    let mut results = Vec::with_capacity(reqs_with_pkg_infos.len());
-    for (pkg_req, pkg_info) in reqs_with_pkg_infos {
-      debug_assert_eq!(pkg_req.name, pkg_info.name);
-      results.push(pending_resolver.resolve_package_req_as_pending(
-        &mut snapshot,
-        pkg_req,
-        pkg_info,
-      ));
-    }
-    results
-  }
-
   pub fn package_reqs(&self) -> HashMap<PackageReq, PackageNv> {
     self.snapshot.read().package_reqs().clone()
   }
@@ -307,52 +264,50 @@ async fn add_package_reqs_to_snapshot(
   package_reqs: &[PackageReq],
   maybe_lockfile: Option<Arc<Mutex<Lockfile>>>,
   get_new_snapshot: impl Fn() -> NpmResolutionSnapshot,
-) -> Result<NpmResolutionSnapshot, AnyError> {
+) -> deno_npm::resolution::AddPkgReqsResult {
   let snapshot = get_new_snapshot();
-  let snapshot = if !snapshot.has_pending()
+  if package_reqs
-    && package_reqs
     .iter()
     .all(|req| snapshot.package_reqs().contains_key(req))
   {
+    log::debug!("Snapshot already up to date. Skipping npm resolution.");
+    return deno_npm::resolution::AddPkgReqsResult {
+      results: package_reqs
+        .iter()
+        .map(|req| Ok(snapshot.package_reqs().get(req).unwrap().clone()))
+        .collect(),
+      dep_graph_result: Ok(snapshot),
+    };
+  }
   log::debug!(
-      "Snapshot already up to date. Skipping pending npm resolution."
+    /* this string is used in tests */
-    );
+    "Running npm resolution."
-    snapshot
-  } else {
-    log::debug!(
-      /* this string is used in tests!! */
-      "Running pending npm resolution."
   );
   let pending_resolver = get_npm_pending_resolver(api);
-    let result = pending_resolver
+  let result = pending_resolver.add_pkg_reqs(snapshot, package_reqs).await;
-      .resolve_pending(snapshot, package_reqs)
-      .await;
   api.clear_memory_cache();
-    match result {
+  let result = match &result.dep_graph_result {
-      Ok(snapshot) => snapshot,
     Err(NpmResolutionError::Resolution(err)) if api.mark_force_reload() => {
       log::debug!("{err:#}");
       log::debug!("npm resolution failed. Trying again...");
 
       // try again
       let snapshot = get_new_snapshot();
-        let result = pending_resolver
+      let result = pending_resolver.add_pkg_reqs(snapshot, package_reqs).await;
-          .resolve_pending(snapshot, package_reqs)
-          .await;
       api.clear_memory_cache();
-        // now surface the result after clearing the cache
+      result
-        result?
-      }
-      Err(err) => return Err(err.into()),
     }
+    _ => result,
   };
 
+  if let Ok(snapshot) = &result.dep_graph_result {
     if let Some(lockfile_mutex) = maybe_lockfile {
       let mut lockfile = lockfile_mutex.lock();
-    populate_lockfile_from_snapshot(&mut lockfile, &snapshot);
+      populate_lockfile_from_snapshot(&mut lockfile, snapshot);
+    }
   }
 
-  Ok(snapshot)
+  result
 }
 
 fn get_npm_pending_resolver(
@@ -374,7 +329,6 @@ fn populate_lockfile_from_snapshot(
   lockfile: &mut Lockfile,
   snapshot: &NpmResolutionSnapshot,
 ) {
-  assert!(!snapshot.has_pending());
   for (package_req, nv) in snapshot.package_reqs() {
     lockfile.insert_package_specifier(
       format!("npm:{}", package_req),

cli/npm/mod.rs

@@ -25,7 +25,6 @@ pub use self::byonm::ByonmCliNpmResolver;
 pub use self::byonm::CliNpmResolverByonmCreateOptions;
 pub use self::cache_dir::NpmCacheDir;
 pub use self::managed::CliNpmResolverManagedCreateOptions;
-pub use self::managed::CliNpmResolverManagedPackageJsonInstallerOption;
 pub use self::managed::CliNpmResolverManagedSnapshotOption;
 pub use self::managed::ManagedCliNpmResolver;
 

cli/resolver.rs

@@ -7,9 +7,6 @@ use deno_ast::MediaType;
 use deno_core::anyhow::anyhow;
 use deno_core::anyhow::Context;
 use deno_core::error::AnyError;
-use deno_core::futures::future;
-use deno_core::futures::future::LocalBoxFuture;
-use deno_core::futures::FutureExt;
 use deno_core::ModuleSourceCode;
 use deno_core::ModuleSpecifier;
 use deno_graph::source::ResolutionMode;
@@ -17,8 +14,9 @@ use deno_graph::source::ResolveError;
 use deno_graph::source::Resolver;
 use deno_graph::source::UnknownBuiltInNodeModuleError;
 use deno_graph::source::DEFAULT_JSX_IMPORT_SOURCE_MODULE;
-use deno_graph::NpmPackageReqsResolution;
+use deno_graph::NpmLoadError;
-use deno_npm::registry::NpmPackageInfo;
+use deno_graph::NpmResolvePkgReqsResult;
+use deno_npm::resolution::NpmResolutionError;
 use deno_runtime::deno_fs;
 use deno_runtime::deno_fs::FileSystem;
 use deno_runtime::deno_node::is_builtin_node_module;
@@ -34,8 +32,6 @@ use deno_semver::npm::NpmPackageReqReference;
 use deno_semver::package::PackageReq;
 use import_map::ImportMap;
 use std::borrow::Cow;
-use std::cell::RefCell;
-use std::collections::HashMap;
 use std::path::Path;
 use std::path::PathBuf;
 use std::rc::Rc;
@@ -440,7 +436,7 @@ pub struct CliGraphResolver {
   maybe_vendor_specifier: Option<ModuleSpecifier>,
   node_resolver: Option<Arc<CliNodeResolver>>,
   npm_resolver: Option<Arc<dyn CliNpmResolver>>,
-  found_package_json_dep_flag: Arc<AtomicFlag>,
+  found_package_json_dep_flag: AtomicFlag,
   bare_node_builtins_enabled: bool,
 }
 
@@ -501,15 +497,11 @@ impl CliGraphResolver {
   pub fn create_graph_npm_resolver(&self) -> WorkerCliNpmGraphResolver {
     WorkerCliNpmGraphResolver {
       npm_resolver: self.npm_resolver.as_ref(),
-      memory_cache: Default::default(),
+      found_package_json_dep_flag: &self.found_package_json_dep_flag,
       bare_node_builtins_enabled: self.bare_node_builtins_enabled,
     }
   }
 
-  pub fn found_package_json_dep(&self) -> bool {
-    self.found_package_json_dep_flag.is_raised()
-  }
-
   fn check_surface_byonm_node_error(
     &self,
     specifier: &str,
@@ -771,9 +763,7 @@ fn resolve_package_json_dep(
 #[derive(Debug)]
 pub struct WorkerCliNpmGraphResolver<'a> {
   npm_resolver: Option<&'a Arc<dyn CliNpmResolver>>,
-  // use a cache per worker so that another worker doesn't clear the
+  found_package_json_dep_flag: &'a AtomicFlag,
-  // cache of another worker
-  memory_cache: Rc<RefCell<HashMap<String, Arc<NpmPackageInfo>>>>,
   bare_node_builtins_enabled: bool,
 }
 
@@ -810,36 +800,25 @@ impl<'a> deno_graph::source::NpmResolver for WorkerCliNpmGraphResolver<'a> {
     }
   }
 
-  fn load_and_cache_npm_package_info(
+  fn load_and_cache_npm_package_info(&self, package_name: &str) {
-    &self,
-    package_name: &str,
-  ) -> LocalBoxFuture<'static, Result<(), AnyError>> {
     match self.npm_resolver {
       Some(npm_resolver) if npm_resolver.as_managed().is_some() => {
         let npm_resolver = npm_resolver.clone();
         let package_name = package_name.to_string();
-        let memory_cache = self.memory_cache.clone();
+        deno_core::unsync::spawn(async move {
-        async move {
           if let Some(managed) = npm_resolver.as_managed() {
-            let package_info =
+            let _ignore = managed.cache_package_info(&package_name).await;
-              managed.cache_package_info(&package_name).await?;
-            memory_cache.borrow_mut().insert(package_name, package_info);
           }
-          Ok(())
+        });
-        }
-        .boxed_local()
-      }
-      _ => {
-        // return it succeeded and error at the import site below
-        Box::pin(future::ready(Ok(())))
       }
+      _ => {}
     }
   }
 
   async fn resolve_pkg_reqs(
     &self,
-    package_reqs: &[&PackageReq],
+    package_reqs: &[PackageReq],
-  ) -> NpmPackageReqsResolution {
+  ) -> NpmResolvePkgReqsResult {
     match &self.npm_resolver {
       Some(npm_resolver) => {
         let npm_resolver = match npm_resolver.as_inner() {
@@ -849,37 +828,50 @@ impl<'a> deno_graph::source::NpmResolver for WorkerCliNpmGraphResolver<'a> {
           InnerCliNpmResolverRef::Byonm(_) => unreachable!(),
         };
 
-        let reqs_with_package_infos = {
+        let top_level_result = if self.found_package_json_dep_flag.is_raised() {
-          let memory_cache = self.memory_cache.borrow();
+          npm_resolver.ensure_top_level_package_json_install().await
-          package_reqs
+        } else {
-            .iter()
+          Ok(())
-            .map(|package_req| {
-              let package_info = memory_cache
-                .get(&package_req.name)
-                .unwrap() // ok because deno_graph would have called load_and_cache_npm_package_info
-                .clone();
-              (*package_req, package_info)
-            })
-            .collect::<Vec<_>>()
         };
-        let result = npm_resolver
+
-          .resolve_npm_for_deno_graph(&reqs_with_package_infos)
+        let result = npm_resolver.add_package_reqs_raw(package_reqs).await;
-          .await;
+
-        if matches!(result, NpmPackageReqsResolution::ReloadRegistryInfo) {
+        NpmResolvePkgReqsResult {
-          self.memory_cache.borrow_mut().clear();
+          results: result
+            .results
+            .into_iter()
+            .map(|r| {
+              r.map_err(|err| match err {
+                NpmResolutionError::Registry(e) => {
+                  NpmLoadError::RegistryInfo(Arc::new(e.into()))
                 }
-        result
+                NpmResolutionError::Resolution(e) => {
+                  NpmLoadError::PackageReqResolution(Arc::new(e.into()))
                 }
-      None => NpmPackageReqsResolution::Resolutions(
+                NpmResolutionError::DependencyEntry(e) => {
-        package_reqs
+                  NpmLoadError::PackageReqResolution(Arc::new(e.into()))
-          .iter()
+                }
-          .map(|_| {
+              })
-            Err(Arc::new(anyhow!(
-              "npm specifiers were requested; but --no-npm is specified"
-            )))
             })
             .collect(),
-      ),
+          dep_graph_result: match top_level_result {
+            Ok(()) => result.dependencies_result.map_err(Arc::new),
+            Err(err) => Err(Arc::new(err)),
+          },
+        }
+      }
+      None => {
+        let err = Arc::new(anyhow!(
+          "npm specifiers were requested; but --no-npm is specified"
+        ));
+        NpmResolvePkgReqsResult {
+          results: package_reqs
+            .iter()
+            .map(|_| Err(NpmLoadError::RegistryInfo(err.clone())))
+            .collect(),
+          dep_graph_result: Err(err),
+        }
+      }
     }
   }
 

cli/standalone/mod.rs

@@ -21,7 +21,6 @@ use crate::npm::create_cli_npm_resolver;
 use crate::npm::CliNpmResolverByonmCreateOptions;
 use crate::npm::CliNpmResolverCreateOptions;
 use crate::npm::CliNpmResolverManagedCreateOptions;
-use crate::npm::CliNpmResolverManagedPackageJsonInstallerOption;
 use crate::npm::CliNpmResolverManagedSnapshotOption;
 use crate::npm::NpmCacheDir;
 use crate::resolver::CjsResolutionStore;
@@ -373,9 +372,12 @@ pub async fn run(
           ));
         let fs = Arc::new(DenoCompileFileSystem::new(vfs))
           as Arc<dyn deno_fs::FileSystem>;
-        let npm_resolver = create_cli_npm_resolver(
+        let npm_resolver =
-          CliNpmResolverCreateOptions::Managed(CliNpmResolverManagedCreateOptions {
+          create_cli_npm_resolver(CliNpmResolverCreateOptions::Managed(
-            snapshot: CliNpmResolverManagedSnapshotOption::Specified(Some(snapshot)),
+            CliNpmResolverManagedCreateOptions {
+              snapshot: CliNpmResolverManagedSnapshotOption::Specified(Some(
+                snapshot,
+              )),
               maybe_lockfile: None,
               fs: fs.clone(),
               http_client_provider: http_client_provider.clone(),
@@ -383,16 +385,13 @@ pub async fn run(
               cache_setting,
               text_only_progress_bar: progress_bar,
               maybe_node_modules_path,
-            package_json_installer:
+              package_json_deps_provider: package_json_deps_provider.clone(),
-              CliNpmResolverManagedPackageJsonInstallerOption::ConditionalInstall(
-                package_json_deps_provider.clone(),
-              ),
               npm_system_info: Default::default(),
               // Packages from different registries are already inlined in the ESZip,
               // so no need to create actual `.npmrc` configuration.
               npmrc: create_default_npmrc(),
-          }),
+            },
-        )
+          ))
           .await?;
         (
           package_json_deps_provider,
@@ -431,8 +430,9 @@ pub async fn run(
         let package_json_deps_provider =
           Arc::new(PackageJsonDepsProvider::new(None));
         let fs = Arc::new(deno_fs::RealFs) as Arc<dyn deno_fs::FileSystem>;
-        let npm_resolver = create_cli_npm_resolver(
+        let npm_resolver =
-          CliNpmResolverCreateOptions::Managed(CliNpmResolverManagedCreateOptions {
+          create_cli_npm_resolver(CliNpmResolverCreateOptions::Managed(
+            CliNpmResolverManagedCreateOptions {
               snapshot: CliNpmResolverManagedSnapshotOption::Specified(None),
               maybe_lockfile: None,
               fs: fs.clone(),
@@ -441,16 +441,13 @@ pub async fn run(
               cache_setting,
               text_only_progress_bar: progress_bar,
               maybe_node_modules_path: None,
-            package_json_installer:
+              package_json_deps_provider: package_json_deps_provider.clone(),
-              CliNpmResolverManagedPackageJsonInstallerOption::ConditionalInstall(
-                package_json_deps_provider.clone(),
-              ),
               npm_system_info: Default::default(),
               // Packages from different registries are already inlined in the ESZip,
               // so no need to create actual `.npmrc` configuration.
               npmrc: create_default_npmrc(),
-          }),
+            },
-        )
+          ))
           .await?;
         (package_json_deps_provider, fs, npm_resolver, None)
       }

cli/tools/task.rs

@@ -122,7 +122,6 @@ pub async fn execute_script(
     if cli_options.has_node_modules_dir() {
       if let Some(npm_resolver) = npm_resolver.as_managed() {
         npm_resolver.ensure_top_level_package_json_install().await?;
-        npm_resolver.resolve_pending().await?;
       }
     }
 

tests/integration/lsp_tests.rs

@@ -12868,14 +12868,10 @@ fn lsp_uses_lockfile_for_npm_initialization() {
   client.initialize_default();
   let mut skipping_count = 0;
   client.wait_until_stderr_line(|line| {
-    if line.contains("Skipping pending npm resolution.") {
+    if line.contains("Skipping npm resolution.") {
       skipping_count += 1;
     }
-    assert!(
+    assert!(!line.contains("Running npm resolution."), "Line: {}", line);
-      !line.contains("Running pending npm resolution."),
-      "Line: {}",
-      line
-    );
     line.contains("Server ready.")
   });
   assert_eq!(skipping_count, 1);

tests/integration/npm_tests.rs

@@ -1822,6 +1822,7 @@ fn reload_info_not_found_cache_but_exists_remote() {
       .run();
     output.assert_matches_text(concat!(
       "error: Could not find npm package '@denotest/esm-import-cjs-default' matching '1.0.0'.\n",
+      "    at file:///[WILDCARD]/main.ts:1:8\n",
     ));
     output.assert_exit_code(1);
 

tests/registry/npm/@denotest/dep-cannot-parse/1.0.0/package.json

@@ -0,0 +1,7 @@
+{
+  "name": "@denotest/dep-cannot-parse",
+  "version": "1.0.0",
+  "dependencies": {
+    "@denotest/esm-basic": "unknown-scheme:unknown"
+  }
+}

tests/specs/npm/dynamic_npm_resolution_failure/__test__.jsonc

@@ -0,0 +1,4 @@
+{
+  "args": "run -A main.ts",
+  "output": "main.out"
+}

tests/specs/npm/dynamic_npm_resolution_failure/main.out

@@ -0,0 +1,15 @@
+[UNORDERED_START]
+Download http://localhost:4260/chalk
+Download http://localhost:4260/@denotest/dep-cannot-parse
+[UNORDERED_END]
+Download http://localhost:4260/chalk/chalk-5.0.1.tgz
+Hi
+TypeError: Error in @denotest/dep-cannot-parse@1.0.0 parsing version requirement for dependency: @denotest/esm-basic@unknown-scheme:unknown
+
+Invalid npm version requirement. Unexpected character.
+  unknown-scheme:unknown
+  ~
+    at async file:///[WILDLINE]main.ts:5:3 {
+  code: "ERR_MODULE_NOT_FOUND"
+}
+Bye

tests/specs/npm/dynamic_npm_resolution_failure/main.ts

@@ -0,0 +1,9 @@
+import chalk from "npm:chalk";
+
+console.log(chalk.green("Hi"));
+try {
+  await import("npm:@denotest/dep-cannot-parse");
+} catch (err) {
+  console.log(err);
+}
+console.log(chalk.green("Bye"));