fix(check): do not add @types/node@* to lockfile if a @types/node req already exists (denoland#29270)

Author: dsherret

Cargo.lock

@@ -65,7 +65,7 @@ deno_lint = "=0.75.0"
 deno_lockfile = "=0.28.0"
 deno_media_type = { version = "=0.2.8", features = ["module_specifier"] }
 deno_native_certs = "0.3.0"
-deno_npm = "=0.33.2"
+deno_npm = "=0.33.3"
 deno_package_json = { version = "=0.6.0", default-features = false }
 deno_path_util = "=0.3.2"
 deno_semver = "=0.7.1"

Cargo.toml

@@ -973,7 +973,6 @@ impl CliFactory {
             cli_options.clone(),
             self.module_graph_builder().await?.clone(),
             self.node_resolver().await?.clone(),
-            self.npm_installer_if_managed().await?.cloned(),
             self.npm_resolver().await?.clone(),
             self.sys(),
             self.tsconfig_resolver()?.clone(),
@@ -1034,7 +1033,6 @@ impl CliFactory {
           let cli_options = self.cli_options()?;
           Ok(Arc::new(ModuleGraphCreator::new(
             cli_options.clone(),
-            self.npm_installer_if_managed().await?.cloned(),
             self.module_graph_builder().await?.clone(),
             self.type_checker().await?.clone(),
           )))

cli/factory.rs

@@ -364,21 +364,18 @@ pub struct CreateGraphOptions<'a> {
 
 pub struct ModuleGraphCreator {
   options: Arc<CliOptions>,
-  npm_installer: Option<Arc<NpmInstaller>>,
   module_graph_builder: Arc<ModuleGraphBuilder>,
   type_checker: Arc<TypeChecker>,
 }
 
 impl ModuleGraphCreator {
   pub fn new(
     options: Arc<CliOptions>,
-    npm_installer: Option<Arc<NpmInstaller>>,
     module_graph_builder: Arc<ModuleGraphBuilder>,
     type_checker: Arc<TypeChecker>,
   ) -> Self {
     Self {
       options,
-      npm_installer,
       module_graph_builder,
       type_checker,
     }
@@ -469,7 +466,7 @@ impl ModuleGraphCreator {
     if self.options.type_check_mode().is_true()
       && !graph_has_external_remote(&graph)
     {
-      self.type_check_graph(graph.clone()).await?;
+      self.type_check_graph(graph.clone())?;
     }
 
     if build_fast_check_graph {
@@ -501,12 +498,6 @@ impl ModuleGraphCreator {
       .build_graph_with_npm_resolution(&mut graph, options)
       .await?;
 
-    if let Some(npm_installer) = &self.npm_installer {
-      if graph.has_node_specifier && self.options.type_check_mode().is_true() {
-        npm_installer.inject_synthetic_types_node_package().await?;
-      }
-    }
-
     Ok(graph)
   }
 
@@ -530,7 +521,7 @@ impl ModuleGraphCreator {
 
     if self.options.type_check_mode().is_true() {
       // provide the graph to the type checker, then get it back after it's done
-      let graph = self.type_check_graph(graph).await?;
+      let graph = self.type_check_graph(graph)?;
       Ok(graph)
     } else {
       Ok(Arc::new(graph))
@@ -541,22 +532,19 @@ impl ModuleGraphCreator {
     self.module_graph_builder.graph_valid(graph)
   }
 
-  async fn type_check_graph(
+  fn type_check_graph(
     &self,
     graph: ModuleGraph,
   ) -> Result<Arc<ModuleGraph>, CheckError> {
-    self
-      .type_checker
-      .check(
-        graph,
-        CheckOptions {
-          build_fast_check_graph: true,
-          lib: self.options.ts_type_lib_window(),
-          reload: self.options.reload_flag(),
-          type_check_mode: self.options.type_check_mode(),
-        },
-      )
-      .await
+    self.type_checker.check(
+      graph,
+      CheckOptions {
+        build_fast_check_graph: true,
+        lib: self.options.ts_type_lib_window(),
+        reload: self.options.reload_flag(),
+        type_check_mode: self.options.type_check_mode(),
+      },
+    )
   }
 }
 
@@ -785,7 +773,15 @@ impl ModuleGraphBuilder {
         },
         options.npm_caching,
       )
-      .await
+      .await?;
+
+    if let Some(npm_installer) = &self.npm_installer {
+      if graph.has_node_specifier && options.graph_kind.include_types() {
+        npm_installer.inject_synthetic_types_node_package().await?;
+      }
+    }
+
+    Ok(())
   }
 
   async fn build_graph_with_npm_resolution_and_build_options<'a>(

cli/graph_util.rs

@@ -224,22 +224,19 @@ impl ModuleLoadPreparer {
 
     // type check if necessary
     if self.options.type_check_mode().is_true() && !has_type_checked {
-      self
-        .type_checker
-        .check(
-          // todo(perf): since this is only done the first time the graph is
-          // created, we could avoid the clone of the graph here by providing
-          // the actual graph on the first run and then getting the Arc<ModuleGraph>
-          // back from the return value.
-          graph.clone(),
-          CheckOptions {
-            build_fast_check_graph: true,
-            lib,
-            reload: self.options.reload_flag(),
-            type_check_mode: self.options.type_check_mode(),
-          },
-        )
-        .await?;
+      self.type_checker.check(
+        // todo(perf): since this is only done the first time the graph is
+        // created, we could avoid the clone of the graph here by providing
+        // the actual graph on the first run and then getting the Arc<ModuleGraph>
+        // back from the return value.
+        graph.clone(),
+        CheckOptions {
+          build_fast_check_graph: true,
+          lib,
+          reload: self.options.reload_flag(),
+          type_check_mode: self.options.type_check_mode(),
+        },
+      )?;
     }
 
     // write the lockfile if there is one and do so after type checking

cli/module_loader.rs

@@ -206,8 +206,16 @@ impl NpmInstaller {
     &self,
   ) -> Result<(), JsErrorBox> {
     self.npm_resolution_initializer.ensure_initialized().await?;
+
+    // don't inject this if it's already been added
+    if self
+      .npm_resolution
+      .any_top_level_package(|id| id.nv.name == "@types/node")
+    {
+      return Ok(());
+    }
+
     let reqs = &[PackageReq::from_str("@types/node").unwrap()];
-    // add and ensure this isn't added to the lockfile
     self
       .add_package_reqs(reqs, PackageCaching::Only(reqs.into()))
       .await?;

cli/npm/installer/mod.rs

@@ -357,18 +357,15 @@ impl PublishPreparer {
       } else {
         // fast check passed, type check the output as a temporary measure
         // until we know that it's reliable and stable
-        let mut diagnostics_by_folder = self
-          .type_checker
-          .check_diagnostics(
-            graph,
-            CheckOptions {
-              build_fast_check_graph: false, // already built
-              lib: self.cli_options.ts_type_lib_window(),
-              reload: self.cli_options.reload_flag(),
-              type_check_mode: self.cli_options.type_check_mode(),
-            },
-          )
-          .await?;
+        let mut diagnostics_by_folder = self.type_checker.check_diagnostics(
+          graph,
+          CheckOptions {
+            build_fast_check_graph: false, // already built
+            lib: self.cli_options.ts_type_lib_window(),
+            reload: self.cli_options.reload_flag(),
+            type_check_mode: self.cli_options.type_check_mode(),
+          },
+        )?;
         // ignore unused parameter diagnostics that may occur due to fast check
         // not having function body implementations
         for result in diagnostics_by_folder.by_ref() {

cli/tools/publish/mod.rs

@@ -37,7 +37,6 @@ use crate::graph_util::resolution_error_for_tsc_diagnostic;
 use crate::graph_util::BuildFastCheckGraphOptions;
 use crate::graph_util::ModuleGraphBuilder;
 use crate::node::CliNodeResolver;
-use crate::npm::installer::NpmInstaller;
 use crate::npm::CliNpmResolver;
 use crate::sys::CliSys;
 use crate::tsc;
@@ -104,7 +103,6 @@ pub struct TypeChecker {
   cjs_tracker: Arc<TypeCheckingCjsTracker>,
   cli_options: Arc<CliOptions>,
   module_graph_builder: Arc<ModuleGraphBuilder>,
-  npm_installer: Option<Arc<NpmInstaller>>,
   node_resolver: Arc<CliNodeResolver>,
   npm_resolver: CliNpmResolver,
   sys: CliSys,
@@ -120,7 +118,6 @@ impl TypeChecker {
     cli_options: Arc<CliOptions>,
     module_graph_builder: Arc<ModuleGraphBuilder>,
     node_resolver: Arc<CliNodeResolver>,
-    npm_installer: Option<Arc<NpmInstaller>>,
     npm_resolver: CliNpmResolver,
     sys: CliSys,
     tsconfig_resolver: Arc<TsConfigResolver>,
@@ -132,7 +129,6 @@ impl TypeChecker {
       cli_options,
       module_graph_builder,
       node_resolver,
-      npm_installer,
       npm_resolver,
       sys,
       tsconfig_resolver,
@@ -144,12 +140,12 @@ impl TypeChecker {
   ///
   /// It is expected that it is determined if a check and/or emit is validated
   /// before the function is called.
-  pub async fn check(
+  pub fn check(
     &self,
     graph: ModuleGraph,
     options: CheckOptions,
   ) -> Result<Arc<ModuleGraph>, CheckError> {
-    let mut diagnostics = self.check_diagnostics(graph, options).await?;
+    let mut diagnostics = self.check_diagnostics(graph, options)?;
     let mut failed = false;
     for result in diagnostics.by_ref() {
       let mut diagnostics = result?;
@@ -178,7 +174,7 @@ impl TypeChecker {
   ///
   /// It is expected that it is determined if a check and/or emit is validated
   /// before the function is called.
-  pub async fn check_diagnostics(
+  pub fn check_diagnostics(
     &self,
     mut graph: ModuleGraph,
     options: CheckOptions,
@@ -212,15 +208,6 @@ impl TypeChecker {
       ));
     }
 
-    // node built-in specifiers use the @types/node package to determine
-    // types, so inject that now (the caller should do this after the lockfile
-    // has been written)
-    if let Some(npm_installer) = &self.npm_installer {
-      if graph.has_node_specifier {
-        npm_installer.inject_synthetic_types_node_package().await?;
-      }
-    }
-
     log::debug!("Type checking");
 
     // add fast check to the graph before getting the roots

cli/type_checker.rs

@@ -141,6 +141,13 @@ impl NpmResolutionCell {
       .collect::<Vec<_>>()
   }
 
+  pub fn any_top_level_package(
+    &self,
+    check: impl Fn(&NpmPackageId) -> bool,
+  ) -> bool {
+    self.snapshot.read().top_level_packages().any(check)
+  }
+
   pub fn all_system_packages(
     &self,
     system_info: &NpmSystemInfo,

resolvers/deno/npm/managed/resolution.rs

@@ -1,16 +1,31 @@
 {
   "tempDir": true,
-  "steps": [{
-    "args": "check --frozen=true",
-    "output": "[WILDCARD]The lockfile is out of date.[WILDCARD]",
-    "exitCode": 1
-  }, {
-    "args": "check --frozen=false",
-    "output": "[WILDCARD]",
-    "exitCode": 0
-  }, {
-    "args": "check --frozen=true",
-    "output": "[WILDCARD]",
-    "exitCode": 0
-  }]
+  "tests": {
+    "check": {
+      "steps": [{
+        "args": "check --frozen=true",
+        "output": "[WILDCARD]The lockfile is out of date.[WILDCARD]",
+        "exitCode": 1
+      }, {
+        "args": "check --frozen=false",
+        "output": "[WILDCARD]",
+        "exitCode": 0
+      }, {
+        "args": "check --frozen=true",
+        "output": "[WILDCARD]",
+        "exitCode": 0
+      }]
+    },
+    "install": {
+      "steps": [{
+        "args": "install --frozen=false --entrypoint main.ts",
+        "output": "[WILDCARD]",
+        "exitCode": 0
+      }, {
+        "args": "check --frozen=true",
+        "output": "[WILDCARD]",
+        "exitCode": 0
+      }]
+    }
+  }
 }