fix(check): ensure @types/node is cached when already in resolution (#31235)

Author: dsherret

cli/graph_util.rs

@@ -792,7 +792,9 @@ impl ModuleGraphBuilder {
       && graph.has_node_specifier
       && graph.graph_kind().include_types()
     {
-      npm_installer.inject_synthetic_types_node_package().await?;
+      npm_installer
+        .inject_synthetic_types_node_package(options.npm_caching)
+        .await?;
     }
 
     Ok(())

cli/tools/repl/session.rs

@@ -40,6 +40,7 @@ use deno_graph::Position;
 use deno_graph::PositionRange;
 use deno_graph::analysis::SpecifierWithRange;
 use deno_lib::util::result::any_and_jserrorbox_downcast_ref;
+use deno_npm_installer::graph::NpmCachingStrategy;
 use deno_resolver::deno_json::CompilerOptionsResolver;
 use deno_runtime::worker::MainWorker;
 use deno_semver::npm::NpmPackageReqReference;
@@ -896,7 +897,9 @@ impl ReplSession {
 
       // prevent messages in the repl about @types/node not being cached
       if has_node_specifier {
-        npm_installer.inject_synthetic_types_node_package().await?;
+        npm_installer
+          .inject_synthetic_types_node_package(NpmCachingStrategy::Eager)
+          .await?;
       }
     }
     Ok(())

libs/npm_installer/graph.rs

@@ -1,5 +1,6 @@
 // Copyright 2018-2025 the Deno authors. MIT license.
 
+use std::borrow::Cow;
 use std::sync::Arc;
 
 use deno_error::JsErrorBox;
@@ -21,6 +22,21 @@ pub enum NpmCachingStrategy {
   Manual,
 }
 
+impl NpmCachingStrategy {
+  pub fn as_package_caching<'a>(
+    &self,
+    package_reqs: &'a [PackageReq],
+  ) -> Option<PackageCaching<'a>> {
+    match self {
+      NpmCachingStrategy::Eager => Some(PackageCaching::All),
+      NpmCachingStrategy::Lazy => {
+        Some(PackageCaching::Only(Cow::Borrowed(package_reqs)))
+      }
+      NpmCachingStrategy::Manual => None,
+    }
+  }
+}
+
 #[derive(Debug)]
 pub struct NpmDenoGraphResolver<
   TNpmCacheHttpClient: NpmCacheHttpClient,
@@ -88,13 +104,7 @@ impl<TNpmCacheHttpClient: NpmCacheHttpClient, TSys: NpmInstallerSys>
         let result = npm_installer
           .add_package_reqs_raw(
             package_reqs,
-            match self.npm_caching {
-              NpmCachingStrategy::Eager => Some(PackageCaching::All),
-              NpmCachingStrategy::Lazy => {
-                Some(PackageCaching::Only(package_reqs.into()))
-              }
-              NpmCachingStrategy::Manual => None,
-            },
+            self.npm_caching.as_package_caching(package_reqs),
           )
           .await;
 

libs/npm_installer/lib.rs

@@ -298,50 +298,65 @@ impl<TNpmCacheHttpClient: NpmCacheHttpClient, TSys: NpmInstallerSys>
     if result.dependencies_result.is_ok()
       && let Some(caching) = caching
     {
-      // the async mutex is unfortunate, but needed to handle the edge case where two workers
-      // try to cache the same package at the same time. we need to hold the lock while we cache
-      // and since that crosses an await point, we need the async mutex.
-      //
-      // should have a negligible perf impact because acquiring the lock is still in the order of nanoseconds
-      // while caching typically takes micro or milli seconds.
-      let _permit = self.install_queue.acquire().await;
-      let uncached = {
-        let cached_reqs = self.cached_reqs.lock();
-        packages
-          .iter()
-          .filter(|req| !cached_reqs.contains(req))
-          .collect::<Vec<_>>()
-      };
+      result.dependencies_result =
+        self.maybe_cache_packages(packages, caching).await;
+    }
 
-      if !uncached.is_empty() {
-        result.dependencies_result =
-          self.fs_installer.cache_packages(caching).await;
-        if result.dependencies_result.is_ok() {
-          let mut cached_reqs = self.cached_reqs.lock();
-          for req in uncached {
-            cached_reqs.insert(req.clone());
-          }
-        }
+    result
+  }
+
+  async fn maybe_cache_packages(
+    &self,
+    packages: &[PackageReq],
+    caching: PackageCaching<'_>,
+  ) -> Result<(), JsErrorBox> {
+    // the async mutex is unfortunate, but needed to handle the edge case where two workers
+    // try to cache the same package at the same time. we need to hold the lock while we cache
+    // and since that crosses an await point, we need the async mutex.
+    //
+    // should have a negligible perf impact because acquiring the lock is still in the order of nanoseconds
+    // while caching typically takes micro or milli seconds.
+    let _permit = self.install_queue.acquire().await;
+    let uncached = {
+      let cached_reqs = self.cached_reqs.lock();
+      packages
+        .iter()
+        .filter(|req| !cached_reqs.contains(req))
+        .collect::<Vec<_>>()
+    };
+
+    if uncached.is_empty() {
+      return Ok(());
+    }
+    let result = self.fs_installer.cache_packages(caching).await;
+    if result.is_ok() {
+      let mut cached_reqs = self.cached_reqs.lock();
+      for req in uncached {
+        cached_reqs.insert(req.clone());
       }
     }
-
     result
   }
 
   pub async fn inject_synthetic_types_node_package(
     &self,
+    cache_strategy: graph::NpmCachingStrategy,
   ) -> Result<(), JsErrorBox> {
     self.npm_resolution_initializer.ensure_initialized().await?;
 
     // don't inject this if it's already been added
+    let reqs = &[PackageReq::from_str("@types/node").unwrap()];
     if self
       .npm_resolution
       .any_top_level_package(|id| id.nv.name == "@types/node")
     {
+      // ensure it's cached though
+      if let Some(caching) = cache_strategy.as_package_caching(reqs) {
+        self.maybe_cache_packages(reqs, caching).await?;
+      }
       return Ok(());
     }
 
-    let reqs = &[PackageReq::from_str("@types/node").unwrap()];
     self
       .add_package_reqs(reqs, PackageCaching::Only(reqs.into()))
       .await?;

tests/specs/check/types_node_in_lockfile_not_cached/__test__.jsonc

@@ -0,0 +1,15 @@
+{
+  "tempDir": true,
+  "steps": [{
+    // will create a deno.lock file
+    "args": "check",
+    "output": "[WILDCARD]"
+  }, {
+    "args": "clean",
+    "output": "[WILDCARD]"
+  }, {
+    // this should ensure the package gets cached
+    "args": "check",
+    "output": "[WILDCARD]"
+  }]
+}

tests/specs/check/types_node_in_lockfile_not_cached/deno.json

@@ -0,0 +1,15 @@
+import * as cp from "node:child_process";
+
+const child = cp.spawn(
+  "cmd.exe",
+  ["/d", "/s", "/c"],
+  {
+    cwd: "test",
+  },
+);
+child.on("exit", (code) => {
+  console.log(code);
+});
+child.on("error", (err) => {
+  console.log(err);
+});