refactor: use deno_graph for npm specifiers (#17858)

This changes npm specifiers to be handled by deno_graph and resolved to
an npm package name and version when the specifier is encountered. It
also slightly changes how npm specifier resolution occurs—previously it
would collect all the npm specifiers and resolve them all at once, but
now it resolves them on the fly as they are encountered in the module
graph.

https://github.com/denoland/deno_graph/pull/232

---------

Co-authored-by: Bartek Iwańczuk <biwanczuk@gmail.com>

1 files changed, 110 insertions, 141 deletions

diff --git a/cli/proc_state.rs b/cli/proc_state.rs
index edd59427c..86af11ec3 100644
--- a/cli/proc_state.rs
+++ b/cli/proc_state.rs
@@ -17,12 +17,12 @@ use crate::cache::ParsedSourceCache;
 use crate::cache::TypeCheckCache;
 use crate::emit::emit_parsed_source;
 use crate::file_fetcher::FileFetcher;
+use crate::graph_util::build_graph_with_npm_resolution;
 use crate::graph_util::graph_lock_or_exit;
 use crate::graph_util::graph_valid_with_cli_options;
 use crate::http_util::HttpClient;
 use crate::node;
 use crate::node::NodeResolution;
-use crate::npm::resolve_graph_npm_info;
 use crate::npm::NpmCache;
 use crate::npm::NpmPackageResolver;
 use crate::npm::NpmRegistryApi;
@@ -43,12 +43,11 @@ use deno_core::resolve_url_or_path;
 use deno_core::CompiledWasmModuleStore;
 use deno_core::ModuleSpecifier;
 use deno_core::SharedArrayBufferStore;
-use deno_graph::npm::NpmPackageReq;
 use deno_graph::npm::NpmPackageReqReference;
 use deno_graph::source::Loader;
 use deno_graph::source::Resolver;
+use deno_graph::Module;
 use deno_graph::ModuleGraph;
-use deno_graph::ModuleKind;
 use deno_graph::Resolution;
 use deno_runtime::deno_broadcast_channel::InMemoryBroadcastChannel;
 use deno_runtime::deno_node::NodeResolutionMode;
@@ -209,6 +208,29 @@ impl ProcState {
 
     let lockfile = cli_options.maybe_lock_file();
 
+    let registry_url = NpmRegistryApi::default_url().to_owned();
+    let npm_cache = NpmCache::from_deno_dir(
+      &dir,
+      cli_options.cache_setting(),
+      http_client.clone(),
+      progress_bar.clone(),
+    );
+    let api = NpmRegistryApi::new(
+      registry_url,
+      npm_cache.clone(),
+      http_client.clone(),
+      progress_bar.clone(),
+    );
+    let npm_resolver = NpmPackageResolver::new_with_maybe_lockfile(
+      npm_cache.clone(),
+      api,
+      cli_options
+        .resolve_local_node_modules_folder()
+        .with_context(|| "Resolving local node_modules folder.")?,
+      cli_options.get_npm_resolution_snapshot(),
+      lockfile.as_ref().cloned(),
+    )
+    .await?;
     let maybe_import_map = cli_options
       .resolve_import_map(&file_fetcher)
       .await?
@@ -217,16 +239,18 @@ impl ProcState {
       cli_options.resolve_inspector_server().map(Arc::new);
 
     let maybe_package_json_deps = cli_options.maybe_package_json_deps()?;
-    let package_json_reqs = if let Some(deps) = &maybe_package_json_deps {
+    if let Some(deps) = &maybe_package_json_deps {
+      // resolve the package.json npm requirements ahead of time
       let mut package_reqs = deps.values().cloned().collect::<Vec<_>>();
       package_reqs.sort(); // deterministic resolution
-      package_reqs
-    } else {
-      Vec::new()
-    };
+      npm_resolver.add_package_reqs(package_reqs).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,
     ));
 
@@ -244,31 +268,12 @@ impl ProcState {
     let emit_cache = EmitCache::new(dir.gen_cache.clone());
     let parsed_source_cache =
       ParsedSourceCache::new(Some(dir.dep_analysis_db_file_path()));
-    let registry_url = NpmRegistryApi::default_url();
     let npm_cache = NpmCache::from_deno_dir(
       &dir,
       cli_options.cache_setting(),
       http_client.clone(),
       progress_bar.clone(),
     );
-    let api = NpmRegistryApi::new(
-      registry_url.clone(),
-      npm_cache.clone(),
-      http_client.clone(),
-      progress_bar.clone(),
-    );
-    let npm_resolver = NpmPackageResolver::new_with_maybe_lockfile(
-      npm_cache.clone(),
-      api,
-      cli_options.no_npm(),
-      cli_options
-        .resolve_local_node_modules_folder()
-        .with_context(|| "Resolving local node_modules folder.")?,
-      cli_options.get_npm_resolution_snapshot(),
-      lockfile.as_ref().cloned(),
-    )
-    .await?;
-    npm_resolver.add_package_reqs(package_json_reqs).await?;
     let node_analysis_cache =
       NodeAnalysisCache::new(Some(dir.node_analysis_db_file_path()));
 
@@ -326,7 +331,8 @@ impl ProcState {
       dynamic_permissions,
     );
     let maybe_imports = self.options.to_maybe_imports()?;
-    let resolver = self.resolver.as_graph_resolver();
+    let graph_resolver = self.resolver.as_graph_resolver();
+    let graph_npm_resolver = self.resolver.as_graph_npm_resolver();
     let maybe_file_watcher_reporter: Option<&dyn deno_graph::source::Reporter> =
       if let Some(reporter) = &self.maybe_file_watcher_reporter {
         Some(reporter)
@@ -344,40 +350,35 @@ impl ProcState {
     let reload_exclusions: HashSet<ModuleSpecifier> =
       graph.specifiers().map(|(s, _)| s.clone()).collect();
 
-    graph
-      .build(
-        roots.clone(),
-        &mut cache,
-        deno_graph::BuildOptions {
-          is_dynamic,
-          imports: maybe_imports,
-          resolver: Some(resolver),
-          module_analyzer: Some(&*analyzer),
-          reporter: maybe_file_watcher_reporter,
-        },
-      )
-      .await;
+    build_graph_with_npm_resolution(
+      &mut graph,
+      &self.npm_resolver,
+      roots.clone(),
+      &mut cache,
+      deno_graph::BuildOptions {
+        is_dynamic,
+        imports: maybe_imports,
+        resolver: Some(graph_resolver),
+        npm_resolver: Some(graph_npm_resolver),
+        module_analyzer: Some(&*analyzer),
+        reporter: maybe_file_watcher_reporter,
+      },
+    )
+    .await?;
 
     // If there is a lockfile, validate the integrity of all the modules.
     if let Some(lockfile) = &self.lockfile {
       graph_lock_or_exit(&graph, &mut lockfile.lock());
     }
 
-    let (npm_package_reqs, has_node_builtin_specifier) = {
+    let graph = {
       graph_valid_with_cli_options(&graph, &roots, &self.options)?;
       let mut graph_data = self.graph_data.write();
       graph_data.update_graph(Arc::new(graph));
-      (
-        graph_data.npm_packages.clone(),
-        graph_data.has_node_builtin_specifier,
-      )
+      graph_data.graph.clone()
     };
 
-    if !npm_package_reqs.is_empty() {
-      self.npm_resolver.add_package_reqs(npm_package_reqs).await?;
-    }
-
-    if has_node_builtin_specifier
+    if graph.has_node_specifier
       && self.options.type_check_mode() != TypeCheckMode::None
     {
       self
@@ -394,12 +395,9 @@ impl ProcState {
     {
       log::debug!("Type checking.");
       let maybe_config_specifier = self.options.maybe_config_file_specifier();
-      let (graph, has_node_builtin_specifier) = {
+      let graph = {
         let graph_data = self.graph_data.read();
-        (
-          Arc::new(graph_data.graph.segment(&roots)),
-          graph_data.has_node_builtin_specifier,
-        )
+        Arc::new(graph_data.graph.segment(&roots))
       };
       let options = check::CheckOptions {
         type_check_mode: self.options.type_check_mode(),
@@ -412,7 +410,6 @@ impl ProcState {
         log_checks: true,
         reload: self.options.reload_flag()
           && !roots.iter().all(|r| reload_exclusions.contains(r)),
-        has_node_builtin_specifier,
       };
       let check_cache =
         TypeCheckCache::new(&self.dir.type_checking_cache_db_file_path());
@@ -501,7 +498,7 @@ impl ProcState {
       let graph_data = self.graph_data.read();
       let graph = &graph_data.graph;
       let maybe_resolved = match graph.get(&referrer) {
-        Some(module) => module
+        Some(Module::Esm(module)) => module
           .dependencies
           .get(specifier)
           .map(|d| (&module.specifier, &d.maybe_code)),
@@ -512,33 +509,36 @@ impl ProcState {
         Some((found_referrer, Resolution::Ok(resolved))) => {
           let specifier = &resolved.specifier;
 
-          if specifier.scheme() == "node" {
-            return node::resolve_builtin_node_module(specifier.path());
-          }
-
-          if let Ok(reference) =
-            NpmPackageReqReference::from_specifier(specifier)
-          {
-            if !self.options.unstable()
-              && matches!(found_referrer.scheme(), "http" | "https")
-            {
-              return Err(custom_error(
+          return match graph.get(specifier) {
+            Some(Module::Npm(module)) => {
+              if !self.options.unstable()
+                && matches!(found_referrer.scheme(), "http" | "https")
+              {
+                return Err(custom_error(
                 "NotSupported",
                 format!("importing npm specifiers in remote modules requires the --unstable flag (referrer: {found_referrer})"),
               ));
+              }
+
+              self
+                .handle_node_resolve_result(node::node_resolve_npm_reference(
+                  &module.nv_reference,
+                  NodeResolutionMode::Execution,
+                  &self.npm_resolver,
+                  permissions,
+                ))
+                .with_context(|| {
+                  format!("Could not resolve '{}'.", module.nv_reference)
+                })
             }
-
-            return self
-              .handle_node_resolve_result(node::node_resolve_npm_reference(
-                &reference,
-                NodeResolutionMode::Execution,
-                &self.npm_resolver,
-                permissions,
-              ))
-              .with_context(|| format!("Could not resolve '{reference}'."));
-          } else {
-            return Ok(specifier.clone());
-          }
+            Some(Module::Node(module)) => {
+              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()),
+            None => Ok(specifier.clone()),
+          };
         }
         Some((_, Resolution::Err(err))) => {
           return Err(custom_error(
@@ -579,6 +579,10 @@ impl ProcState {
         if let Ok(reference) =
           NpmPackageReqReference::from_specifier(&specifier)
         {
+          let reference = self
+            .npm_resolver
+            .resolution()
+            .pkg_req_ref_to_nv_ref(reference)?;
           return self
             .handle_node_resolve_result(node::node_resolve_npm_reference(
               &reference,
@@ -597,8 +601,8 @@ impl ProcState {
   pub fn cache_module_emits(&self) -> Result<(), AnyError> {
     let graph = self.graph();
     for module in graph.modules() {
-      let is_emittable = module.kind != ModuleKind::External
-        && matches!(
+      if let Module::Esm(module) = module {
+        let is_emittable = matches!(
           module.media_type,
           MediaType::TypeScript
             | MediaType::Mts
@@ -606,14 +610,13 @@ impl ProcState {
             | MediaType::Jsx
             | MediaType::Tsx
         );
-      if is_emittable {
-        if let Some(code) = &module.maybe_source {
+        if is_emittable {
           emit_parsed_source(
             &self.emit_cache,
             &self.parsed_source_cache,
             &module.specifier,
             module.media_type,
-            code,
+            &module.source,
             &self.emit_options,
             self.emit_options_hash,
           )?;
@@ -651,36 +654,34 @@ impl ProcState {
     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(),
       // TODO(bartlomieju): this should use dependencies from `package.json`?
       None,
     );
     let graph_resolver = cli_resolver.as_graph_resolver();
+    let graph_npm_resolver = cli_resolver.as_graph_npm_resolver();
     let analyzer = self.parsed_source_cache.as_analyzer();
 
     let mut graph = ModuleGraph::default();
-    graph
-      .build(
-        roots,
-        loader,
-        deno_graph::BuildOptions {
-          is_dynamic: false,
-          imports: maybe_imports,
-          resolver: Some(graph_resolver),
-          module_analyzer: Some(&*analyzer),
-          reporter: None,
-        },
-      )
-      .await;
+    build_graph_with_npm_resolution(
+      &mut graph,
+      &self.npm_resolver,
+      roots,
+      loader,
+      deno_graph::BuildOptions {
+        is_dynamic: false,
+        imports: maybe_imports,
+        resolver: Some(graph_resolver),
+        npm_resolver: Some(graph_npm_resolver),
+        module_analyzer: Some(&*analyzer),
+        reporter: None,
+      },
+    )
+    .await?;
 
-    // add the found npm package requirements to the npm resolver and cache them
-    let graph_npm_info = resolve_graph_npm_info(&graph);
-    if !graph_npm_info.package_reqs.is_empty() {
-      self
-        .npm_resolver
-        .add_package_reqs(graph_npm_info.package_reqs)
-        .await?;
-    }
-    if graph_npm_info.has_node_builtin_specifier
+    if graph.has_node_specifier
       && self.options.type_check_mode() != TypeCheckMode::None
     {
       self
@@ -697,7 +698,7 @@ impl ProcState {
   }
 
   pub fn has_node_builtin_specifier(&self) -> bool {
-    self.graph_data.read().has_node_builtin_specifier
+    self.graph_data.read().graph.has_node_specifier
   }
 }
 
@@ -728,44 +729,12 @@ impl deno_graph::source::Reporter for FileWatcherReporter {
 #[derive(Debug, Default)]
 struct GraphData {
   graph: Arc<ModuleGraph>,
-  /// The npm package requirements from all the encountered graphs
-  /// in the order that they should be resolved.
-  npm_packages: Vec<NpmPackageReq>,
-  /// If the graph had a "node:" specifier.
-  has_node_builtin_specifier: bool,
   checked_libs: HashMap<TsTypeLib, HashSet<ModuleSpecifier>>,
 }
 
 impl GraphData {
   /// Store data from `graph` into `self`.
   pub fn update_graph(&mut self, graph: Arc<ModuleGraph>) {
-    let mut has_npm_specifier_in_graph = false;
-
-    for (specifier, _) in graph.specifiers() {
-      match specifier.scheme() {
-        "node" => {
-          // We don't ever set this back to false because once it's
-          // on then it's on globally.
-          self.has_node_builtin_specifier = true;
-        }
-        "npm" => {
-          if !has_npm_specifier_in_graph
-            && NpmPackageReqReference::from_specifier(specifier).is_ok()
-          {
-            has_npm_specifier_in_graph = true;
-          }
-        }
-        _ => {}
-      }
-
-      if has_npm_specifier_in_graph && self.has_node_builtin_specifier {
-        break; // exit early
-      }
-    }
-
-    if has_npm_specifier_in_graph {
-      self.npm_packages = resolve_graph_npm_info(&graph).package_reqs;
-    }
     self.graph = graph;
   }