fix(check): attempt to resolve types from pkg before @types pkg (de…

…noland#24152)

I've been meaning to fix this for ages, but I finally ran into it here:


https://github.com/dsherret/ts-ast-viewer/actions/runs/9432038675/job/25981325408

We need to resolve the `@types` package as a fallback instead of eagerly
resolving it.

cli/npm/byonm.rs

@@ -12,7 +12,6 @@ use deno_core::error::AnyError;
 use deno_core::serde_json;
 use deno_runtime::deno_fs::FileSystem;
 use deno_runtime::deno_node::NodePermissions;
-use deno_runtime::deno_node::NodeResolutionMode;
 use deno_runtime::deno_node::NpmResolver;
 use deno_runtime::deno_node::PackageJson;
 use deno_semver::package::PackageReq;
@@ -23,7 +22,6 @@ use crate::args::NpmProcessStateKind;
 use crate::util::fs::canonicalize_path_maybe_not_exists_with_fs;
 use deno_runtime::fs_util::specifier_to_file_path;
 
-use super::common::types_package_name;
 use super::CliNpmResolver;
 use super::InnerCliNpmResolverRef;
 
@@ -97,20 +95,13 @@ impl NpmResolver for ByonmCliNpmResolver {
  &self,
  name: &str,
  referrer: &ModuleSpecifier,
- mode: NodeResolutionMode,
  ) -> Result<PathBuf, AnyError> {
  fn inner(
  fs: &dyn FileSystem,
  name: &str,
  referrer: &ModuleSpecifier,
- mode: NodeResolutionMode,
  ) -> Result<PathBuf, AnyError> {
  let referrer_file = specifier_to_file_path(referrer)?;
- let types_pkg_name = if mode.is_types() && !name.starts_with("@types/") {
- Some(types_package_name(name))
- } else {
- None
- };
  let mut current_folder = referrer_file.parent().unwrap();
  loop {
  let node_modules_folder = if current_folder.ends_with("node_modules") {
@@ -119,14 +110,6 @@ impl NpmResolver for ByonmCliNpmResolver {
  Cow::Owned(current_folder.join("node_modules"))
  };
 
- // attempt to resolve the types package first, then fallback to the regular package
- if let Some(types_pkg_name) = &types_pkg_name {
- let sub_dir = join_package_name(&node_modules_folder, types_pkg_name);
- if fs.is_dir_sync(&sub_dir) {
- return Ok(sub_dir);
- }
- }
-
  let sub_dir = join_package_name(&node_modules_folder, name);
  if fs.is_dir_sync(&sub_dir) {
  return Ok(sub_dir);
@@ -146,7 +129,7 @@ impl NpmResolver for ByonmCliNpmResolver {
  );
  }
 
- let path = inner(&*self.fs, name, referrer, mode)?;
+ let path = inner(&*self.fs, name, referrer)?;
  Ok(self.fs.realpath_sync(&path)?)
  }
 

cli/npm/common.rs

@@ -3,14 +3,6 @@
 use deno_npm::npm_rc::RegistryConfig;
 use reqwest::header;
 
-/// Gets the corresponding @types package for the provided package name.
-pub fn types_package_name(package_name: &str) -> String {
- debug_assert!(!package_name.starts_with("@types/"));
- // Scoped packages will get two underscores for each slash
- // https://github.com/DefinitelyTyped/DefinitelyTyped/tree/15f1ece08f7b498f4b9a2147c2a46e94416ca777#what-about-scoped-packages
- format!("@types/{}", package_name.replace('/', "__"))
-}
-
 // TODO(bartlomieju): support more auth methods besides token and basic auth
 pub fn maybe_auth_header_for_npm_registry(
  registry_config: &RegistryConfig,
@@ -31,17 +23,3 @@ pub fn maybe_auth_header_for_npm_registry(
 
  None
 }
-
-#[cfg(test)]
-mod test {
- use super::types_package_name;
-
- #[test]
- fn test_types_package_name() {
- assert_eq!(types_package_name("name"), "@types/name");
- assert_eq!(
- types_package_name("@scoped/package"),
- "@types/@scoped__package"
- );
- }
-}

cli/npm/managed/mod.rs

@@ -23,7 +23,6 @@ use deno_npm::NpmResolutionPackage;
 use deno_npm::NpmSystemInfo;
 use deno_runtime::deno_fs::FileSystem;
 use deno_runtime::deno_node::NodePermissions;
-use deno_runtime::deno_node::NodeResolutionMode;
 use deno_runtime::deno_node::NpmResolver;
 use deno_semver::package::PackageNv;
 use deno_semver::package::PackageReq;
@@ -531,11 +530,10 @@ impl NpmResolver for ManagedCliNpmResolver {
  &self,
  name: &str,
  referrer: &ModuleSpecifier,
- mode: NodeResolutionMode,
  ) -> Result<PathBuf, AnyError> {
  let path = self
  .fs_resolver
- .resolve_package_folder_from_package(name, referrer, mode)?;
+ .resolve_package_folder_from_package(name, referrer)?;
  let path =
  canonicalize_path_maybe_not_exists_with_fs(&path, self.fs.as_ref())?;
  log::debug!("Resolved {} from {} to {}", name, referrer, path.display());

cli/npm/managed/resolvers/common.rs

@@ -19,7 +19,6 @@ use deno_npm::NpmPackageId;
 use deno_npm::NpmResolutionPackage;
 use deno_runtime::deno_fs::FileSystem;
 use deno_runtime::deno_node::NodePermissions;
-use deno_runtime::deno_node::NodeResolutionMode;
 
 use crate::npm::managed::cache::TarballCache;
 
@@ -41,7 +40,6 @@ pub trait NpmPackageFsResolver: Send + Sync {
  &self,
  name: &str,
  referrer: &ModuleSpecifier,
- mode: NodeResolutionMode,
  ) -> Result<PathBuf, AnyError>;
 
  fn resolve_package_cache_folder_id_from_specifier(

cli/npm/managed/resolvers/global.rs

@@ -11,16 +11,12 @@ use deno_ast::ModuleSpecifier;
 use deno_core::anyhow::bail;
 use deno_core::error::AnyError;
 use deno_core::url::Url;
-use deno_npm::resolution::PackageNotFoundFromReferrerError;
 use deno_npm::NpmPackageCacheFolderId;
 use deno_npm::NpmPackageId;
-use deno_npm::NpmResolutionPackage;
 use deno_npm::NpmSystemInfo;
 use deno_runtime::deno_fs::FileSystem;
 use deno_runtime::deno_node::NodePermissions;
-use deno_runtime::deno_node::NodeResolutionMode;
 
-use super::super::super::common::types_package_name;
 use super::super::cache::NpmCache;
 use super::super::cache::TarballCache;
 use super::super::resolution::NpmResolution;
@@ -57,17 +53,6 @@ impl GlobalNpmPackageResolver {
  system_info,
  }
  }
-
- fn resolve_types_package(
- &self,
- package_name: &str,
- referrer_pkg_id: &NpmPackageCacheFolderId,
- ) -> Result<NpmResolutionPackage, Box<PackageNotFoundFromReferrerError>> {
- let types_name = types_package_name(package_name);
- self
- .resolution
- .resolve_package_from_package(&types_name, referrer_pkg_id)
- }
 }
 
 #[async_trait(?Send)]
@@ -92,27 +77,16 @@ impl NpmPackageFsResolver for GlobalNpmPackageResolver {
  &self,
  name: &str,
  referrer: &ModuleSpecifier,
- mode: NodeResolutionMode,
  ) -> Result<PathBuf, AnyError> {
  let Some(referrer_pkg_id) = self
  .cache
  .resolve_package_folder_id_from_specifier(referrer)
  else {
  bail!("could not find npm package for '{}'", referrer);
  };
- 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)?,
- }
- } else {
- self
- .resolution
- .resolve_package_from_package(name, &referrer_pkg_id)?
- };
+ let pkg = self
+ .resolution
+ .resolve_package_from_package(name, &referrer_pkg_id)?;
  self.package_folder(&pkg.id)
  }
 

cli/npm/managed/resolvers/local.rs

@@ -39,14 +39,12 @@ use deno_npm::NpmResolutionPackage;
 use deno_npm::NpmSystemInfo;
 use deno_runtime::deno_fs;
 use deno_runtime::deno_node::NodePermissions;
-use deno_runtime::deno_node::NodeResolutionMode;
 use deno_semver::package::PackageNv;
 use serde::Deserialize;
 use serde::Serialize;
 
 use crate::npm::cache_dir::mixed_case_package_name_encode;
 
-use super::super::super::common::types_package_name;
 use super::super::cache::NpmCache;
 use super::super::cache::TarballCache;
 use super::super::resolution::NpmResolution;
@@ -175,7 +173,6 @@ impl NpmPackageFsResolver for LocalNpmPackageResolver {
  &self,
  name: &str,
  referrer: &ModuleSpecifier,
- mode: NodeResolutionMode,
  ) -> Result<PathBuf, AnyError> {
  let Some(local_path) = self.resolve_folder_for_specifier(referrer)? else {
  bail!("could not find npm package for '{}'", referrer);
@@ -190,15 +187,6 @@ impl NpmPackageFsResolver for LocalNpmPackageResolver {
  Cow::Owned(current_folder.join("node_modules"))
  };
 
- // attempt to resolve the types package first, then fallback to the regular package
- if mode.is_types() && !name.starts_with("@types/") {
- let sub_dir =
- join_package_name(&node_modules_folder, &types_package_name(name));
- if self.fs.is_dir_sync(&sub_dir) {
- return Ok(sub_dir);
- }
- }
-
  let sub_dir = join_package_name(&node_modules_folder, name);
  if self.fs.is_dir_sync(&sub_dir) {
  return Ok(sub_dir);

cli/resolver.rs

@@ -514,14 +514,11 @@ impl CliGraphResolver {
  &self,
  specifier: &str,
  referrer: &ModuleSpecifier,
- mode: NodeResolutionMode,
  original_err: AnyError,
  resolver: &ByonmCliNpmResolver,
  ) -> Result<(), AnyError> {
  if let Ok((pkg_name, _, _)) = parse_npm_pkg_name(specifier, referrer) {
- match resolver
- .resolve_package_folder_from_package(&pkg_name, referrer, mode)
- {
+ match resolver.resolve_package_folder_from_package(&pkg_name, referrer) {
  Ok(_) => {
  return Err(original_err);
  }
@@ -650,7 +647,6 @@ impl Resolver for CliGraphResolver {
  .check_surface_byonm_node_error(
  specifier,
  referrer,
- to_node_mode(mode),
  anyhow!("Cannot find \"{}\"", specifier),
  resolver,
  )
@@ -659,11 +655,7 @@ impl Resolver for CliGraphResolver {
  Err(err) => {
  self
  .check_surface_byonm_node_error(
- specifier,
- referrer,
- to_node_mode(mode),
- err,
- resolver,
+ specifier, referrer, err, resolver,
  )
  .map_err(ResolveError::Other)?;
  }

ext/node/analyze.rs

@@ -307,7 +307,6 @@ impl<TCjsCodeAnalyzer: CjsCodeAnalyzer> NodeCodeTranslator<TCjsCodeAnalyzer> {
  let module_dir = self.npm_resolver.resolve_package_folder_from_package(
  package_specifier.as_str(),
  referrer,
- mode,
  )?;
 
  let package_json_path = module_dir.join("package.json");

ext/node/lib.rs

@@ -162,7 +162,6 @@ pub trait NpmResolver: std::fmt::Debug + MaybeSend + MaybeSync {
  &self,
  specifier: &str,
  referrer: &ModuleSpecifier,
- mode: NodeResolutionMode,
  ) -> Result<PathBuf, AnyError>;
 
  fn in_npm_package(&self, specifier: &ModuleSpecifier) -> bool;

ext/node/ops/require.rs

@@ -198,7 +198,6 @@ pub fn op_require_resolve_deno_dir(
  &ModuleSpecifier::from_file_path(&parent_filename).unwrap_or_else(|_| {
  panic!("Url::from_file_path: [{:?}]", parent_filename)
  }),
- NodeResolutionMode::Execution,
  )
  .ok()
  .map(|p| p.to_string_lossy().to_string())

ext/node/resolution.rs

@@ -1037,9 +1037,45 @@ impl NodeResolver {
  }
  }
 
+ let result = self.resolve_package_subpath_for_package(
+ &package_name,
+ &package_subpath,
+ referrer,
+ referrer_kind,
+ conditions,
+ mode,
+ );
+ if mode.is_types() && !matches!(result, Ok(Some(_))) {
+ // try to resolve with the @types/node package
+ let package_name = types_package_name(&package_name);
+ if let Ok(Some(result)) = self.resolve_package_subpath_for_package(
+ &package_name,
+ &package_subpath,
+ referrer,
+ referrer_kind,
+ conditions,
+ mode,
+ ) {
+ return Ok(Some(result));
+ }
+ }
+
+ result
+ }
+
+ #[allow(clippy::too_many_arguments)]
+ fn resolve_package_subpath_for_package(
+ &self,
+ package_name: &str,
+ package_subpath: &str,
+ referrer: &ModuleSpecifier,
+ referrer_kind: NodeModuleKind,
+ conditions: &[&str],
+ mode: NodeResolutionMode,
+ ) -> Result<Option<ModuleSpecifier>, AnyError> {
  let package_dir_path = self
  .npm_resolver
- .resolve_package_folder_from_package(&package_name, referrer, mode)?;
+ .resolve_package_folder_from_package(package_name, referrer)?;
  let package_json_path = package_dir_path.join("package.json");
 
  // todo: error with this instead when can't find package
@@ -1060,7 +1096,7 @@ impl NodeResolver {
  .load_package_json(&mut AllowAllNodePermissions, package_json_path)?;
  self.resolve_package_subpath(
  &package_json,
- &package_subpath,
+ package_subpath,
  referrer,
  referrer_kind,
  conditions,
@@ -1600,6 +1636,14 @@ fn pattern_key_compare(a: &str, b: &str) -> i32 {
  0
 }
 
+/// Gets the corresponding @types package for the provided package name.
+fn types_package_name(package_name: &str) -> String {
+ debug_assert!(!package_name.starts_with("@types/"));
+ // Scoped packages will get two underscores for each slash
+ // https://github.com/DefinitelyTyped/DefinitelyTyped/tree/15f1ece08f7b498f4b9a2147c2a46e94416ca777#what-about-scoped-packages
+ format!("@types/{}", package_name.replace('/', "__"))
+}
+
 #[cfg(test)]
 mod tests {
  use deno_core::serde_json::json;
@@ -1780,4 +1824,13 @@ mod tests {
  assert_eq!(actual.to_string_lossy(), *expected);
  }
  }
+
+ #[test]
+ fn test_types_package_name() {
+ assert_eq!(types_package_name("name"), "@types/name");
+ assert_eq!(
+ types_package_name("@scoped/package"),
+ "@types/@scoped__package"
+ );
+ }
 }

tests/specs/npm/check_prefers_non_types_node_pkg/__test__.jsonc

@@ -0,0 +1,7 @@
+{
+ "envs": {
+ "DENO_FUTURE": "1"
+ },
+ "args": "check --quiet main.ts",
+ "output": ""
+}

tests/specs/npm/check_prefers_non_types_node_pkg/main.ts

@@ -0,0 +1,3 @@
+import { compressToEncodedURIComponent } from "lz-string";
+
+console.log(compressToEncodedURIComponent("Hello, World!"));