fix(npm): dependency types were sometimes not being resolved when package had no types entry (#16958)

Closes #16957

cli/lsp/documents.rs

@@ -943,7 +943,7 @@ impl Documents {
   /// tsc when type checking.
   pub fn resolve(
     &self,
-    specifiers: Vec<String>,
+    specifiers: &[String],
     referrer: &ModuleSpecifier,
     maybe_npm_resolver: Option<&NpmPackageResolver>,
   ) -> Option<Vec<Option<(ModuleSpecifier, MediaType)>>> {
@@ -955,7 +955,7 @@ impl Documents {
           // we're in an npm package, so use node resolution
           results.push(Some(NodeResolution::into_specifier_and_media_type(
             node::node_resolve(
-              &specifier,
+              specifier,
               referrer,
               NodeResolutionMode::Types,
               npm_resolver,
@@ -968,13 +968,13 @@ impl Documents {
       }
       // handle npm:<package> urls
       if specifier.starts_with("asset:") {
-        if let Ok(specifier) = ModuleSpecifier::parse(&specifier) {
+        if let Ok(specifier) = ModuleSpecifier::parse(specifier) {
           let media_type = MediaType::from(&specifier);
           results.push(Some((specifier, media_type)));
         } else {
           results.push(None);
         }
-      } else if let Some(dep) = dependencies.deps.get(&specifier) {
+      } else if let Some(dep) = dependencies.deps.get(specifier) {
         if let Resolved::Ok { specifier, .. } = &dep.maybe_type {
           results.push(self.resolve_dependency(specifier, maybe_npm_resolver));
         } else if let Resolved::Ok { specifier, .. } = &dep.maybe_code {
@@ -983,12 +983,12 @@ impl Documents {
           results.push(None);
         }
       } else if let Some(Resolved::Ok { specifier, .. }) =
-        self.resolve_imports_dependency(&specifier)
+        self.resolve_imports_dependency(specifier)
       {
         // clone here to avoid double borrow of self
         let specifier = specifier.clone();
         results.push(self.resolve_dependency(&specifier, maybe_npm_resolver));
-      } else if let Ok(npm_ref) = NpmPackageReference::from_str(&specifier) {
+      } else if let Ok(npm_ref) = NpmPackageReference::from_str(specifier) {
         results.push(maybe_npm_resolver.map(|npm_resolver| {
           NodeResolution::into_specifier_and_media_type(
             node_resolve_npm_reference(

cli/lsp/tsc.rs

@@ -2726,7 +2726,7 @@ fn op_resolve(
   let referrer = state.normalize_specifier(&args.base)?;
 
   let result = if let Some(resolved) = state.state_snapshot.documents.resolve(
-    args.specifiers,
+    &args.specifiers,
     &referrer,
     state.state_snapshot.maybe_npm_resolver.as_ref(),
   ) {

cli/node/mod.rs

@@ -95,15 +95,11 @@ impl NodeResolution {
           },
         )
       }
-      maybe_response => {
-        let specifier = match maybe_response {
-          Some(response) => response.into_url(),
-          None => {
-            ModuleSpecifier::parse("deno:///missing_dependency.d.ts").unwrap()
-          }
-        };
-        (specifier, MediaType::Dts)
-      }
+      Some(resolution) => (resolution.into_url(), MediaType::Dts),
+      None => (
+        ModuleSpecifier::parse("deno:///missing_dependency.d.ts").unwrap(),
+        MediaType::Dts,
+      ),
     }
   }
 }

cli/npm/resolvers/global.rs

@@ -13,15 +13,16 @@ use deno_core::futures::future::BoxFuture;
 use deno_core::futures::FutureExt;
 use deno_core::url::Url;
 use deno_runtime::deno_node::NodeResolutionMode;
-use deno_runtime::deno_node::PackageJson;
 
 use crate::args::Lockfile;
+use crate::npm::cache::NpmPackageCacheFolderId;
 use crate::npm::resolution::NpmResolution;
 use crate::npm::resolution::NpmResolutionSnapshot;
 use crate::npm::resolvers::common::cache_packages;
 use crate::npm::NpmCache;
 use crate::npm::NpmPackageId;
 use crate::npm::NpmPackageReq;
+use crate::npm::NpmResolutionPackage;
 use crate::npm::RealNpmRegistryApi;
 
 use super::common::ensure_registry_read_permission;
@@ -61,6 +62,17 @@ impl GlobalNpmPackageResolver {
       .cache
       .package_folder_for_id(&folder_id, &self.registry_url)
   }
+
+  fn resolve_types_package(
+    &self,
+    package_name: &str,
+    referrer_pkg_id: &NpmPackageCacheFolderId,
+  ) -> Result<NpmResolutionPackage, AnyError> {
+    let types_name = types_package_name(package_name);
+    self
+      .resolution
+      .resolve_package_from_package(&types_name, referrer_pkg_id)
+  }
 }
 
 impl InnerNpmPackageResolver for GlobalNpmPackageResolver {
@@ -81,30 +93,20 @@ impl InnerNpmPackageResolver for GlobalNpmPackageResolver {
     let referrer_pkg_id = self
       .cache
       .resolve_package_folder_id_from_specifier(referrer, &self.registry_url)?;
-    let pkg_result = self
-      .resolution
-      .resolve_package_from_package(name, &referrer_pkg_id);
-    if mode.is_types() && !name.starts_with("@types/") {
-      // When doing types resolution, the package must contain a "types"
-      // entry, or else it will then search for a @types package
-      if let Ok(pkg) = pkg_result {
-        let package_folder = self.package_folder(&pkg.id);
-        let package_json = PackageJson::load_skip_read_permission(
-          package_folder.join("package.json"),
-        )?;
-        if package_json.types.is_some() {
-          return Ok(package_folder);
-        }
+    let pkg = if mode.is_types() && !name.starts_with("@types/") {
+      // attempt to resolve the types package first, then fallback to the regular package
+      match self.resolve_types_package(name, &referrer_pkg_id) {
+        Ok(pkg) => pkg,
+        Err(_) => self
+          .resolution
+          .resolve_package_from_package(name, &referrer_pkg_id)?,
       }
-
-      let name = types_package_name(name);
-      let pkg = self
-        .resolution
-        .resolve_package_from_package(&name, &referrer_pkg_id)?;
-      Ok(self.package_folder(&pkg.id))
     } else {
-      Ok(self.package_folder(&pkg_result?.id))
-    }
+      self
+        .resolution
+        .resolve_package_from_package(name, &referrer_pkg_id)?
+    };
+    Ok(self.package_folder(&pkg.id))
   }
 
   fn resolve_package_folder_from_specifier(

cli/npm/resolvers/mod.rs

@@ -189,7 +189,11 @@ impl NpmPackageResolver {
       .inner
       .resolve_package_folder_from_deno_module(pkg_req)?;
     let path = canonicalize_path_maybe_not_exists(&path)?;
-    log::debug!("Resolved {} to {}", pkg_req, path.display());
+    log::debug!(
+      "Resolved package folder of {} to {}",
+      pkg_req,
+      path.display()
+    );
     Ok(path)
   }
 
@@ -217,7 +221,11 @@ impl NpmPackageResolver {
     let path = self
       .inner
       .resolve_package_folder_from_specifier(specifier)?;
-    log::debug!("Resolved {} to {}", specifier, path.display());
+    log::debug!(
+      "Resolved package folder of {} to {}",
+      specifier,
+      path.display()
+    );
     Ok(path)
   }
 

cli/tests/testdata/npm/registry/@denotest/types-no-types-entry/1.0.0/dist/main.d.ts

@@ -1 +1 @@
-export function getValue(): 5;
+export { getValue } from "@denotest/types-entry-value-not-exists";

cli/tests/testdata/npm/registry/@denotest/types-no-types-entry/1.0.0/dist/main.js

@@ -1 +1 @@
-module.exports.getValue = () => 5;
+module.exports.getValue = require("@denotest/types-entry-value-not-exists").getValue;

cli/tests/testdata/npm/registry/@denotest/types-no-types-entry/1.0.0/package.json

@@ -1,5 +1,8 @@
 {
   "name": "@denotest/types-no-types-entry",
   "version": "1.0.0",
-  "main": "./dist/main.js"
+  "main": "./dist/main.js",
+  "dependencies": {
+    "@denotest/types-entry-value-not-exists": "^1.0"
+  }
 }

cli/tests/testdata/npm/types_no_types_entry/main.out

@@ -1,4 +1,6 @@
 Download http://localhost:4545/npm/registry/@denotest/types-no-types-entry
+Download http://localhost:4545/npm/registry/@denotest/types-entry-value-not-exists
+Download http://localhost:4545/npm/registry/@denotest/types-entry-value-not-exists/1.0.0.tgz
 Download http://localhost:4545/npm/registry/@denotest/types-no-types-entry/1.0.0.tgz
 Check file://[WILDCARD]/types_no_types_entry/main.ts
 error: TS2322 [ERROR]: Type 'number' is not assignable to type 'string'.

cli/tsc/mod.rs

@@ -542,7 +542,8 @@ fn op_resolve(
   args: ResolveArgs,
 ) -> Result<Vec<(String, String)>, AnyError> {
   let state = state.borrow_mut::<State>();
-  let mut resolved: Vec<(String, String)> = Vec::new();
+  let mut resolved: Vec<(String, String)> =
+    Vec::with_capacity(args.specifiers.len());
   let referrer = if let Some(remapped_specifier) =
     state.remapped_specifiers.get(&args.base)
   {
@@ -661,6 +662,7 @@ fn op_resolve(
           ".d.ts".to_string(),
         ),
       };
+      log::debug!("Resolved {} to {:?}", specifier, result);
       resolved.push(result);
     }
   }