fix(node): resolve types via package.json for directory import (denol…

…and#22878) Does a package resolve when resolving types for a directory (copying the behaviour that typescript does).
dsherret · Mar 15, 2024 · 0773ab9 · 0773ab9
1 parent c016a12
commit 0773ab9
Show file tree

Hide file tree

Showing 9 changed files with 226 additions and 81 deletions.
diff --git a/ext/node/polyfills/01_require.js b/ext/node/polyfills/01_require.js
@@ -491,31 491,6 @@ Module.globalPaths = modulePaths;
 
 const CHAR_FORWARD_SLASH = 47;
 const TRAILING_SLASH_REGEX = /(?:^|\/)\.?\.$/;
-const encodedSepRegEx = //|,/i;
-
-function finalizeEsmResolution(
- resolved,
- parentPath,
- pkgPath,
-) {
- if (RegExpPrototypeTest(encodedSepRegEx, resolved)) {
- throw new ERR_INVALID_MODULE_SPECIFIER(
- resolved,
- 'must not include encoded "/" or "\\" characters',
- parentPath,
- );
- }
- // const filename = fileURLToPath(resolved);
- const filename = resolved;
- const actual = tryFile(filename, false);
- if (actual) {
- return actual;
- }
- throw new ERR_MODULE_NOT_FOUND(
- filename,
- path.resolve(pkgPath, "package.json"),
- );
-}
 
 // This only applies to requests of a specific form:
 // 1. name/.*

diff --git a/ext/node/resolution.rs b/ext/node/resolution.rs
@@ -203,8 203,14 @@ impl NodeResolver {
  let path = url.to_file_path().unwrap();
  // todo(16370): the module kind is not correct here. I think we need
  // typescript to tell us if the referrer is esm or cjs
- match self.path_to_declaration_path(path, NodeModuleKind::Esm) {
- Some(path) => to_file_specifier(&path),
  let maybe_decl_url = self.path_to_declaration_url(
  path,
  referrer,
  NodeModuleKind::Esm,
  permissions,
  )?;
  match maybe_decl_url {
  Some(url) => url,
  None => return Ok(None),
  }
  }
@@ -231,10 237,12 @@ impl NodeResolver {
  let file_path = to_file_path(&resolved_specifier);
  // todo(dsherret): the node module kind is not correct and we
  // should use the value provided by typescript instead
- let declaration_path =
- self.path_to_declaration_path(file_path, NodeModuleKind::Esm);
- declaration_path
- .map(|declaration_path| to_file_specifier(&declaration_path))
  self.path_to_declaration_url(
  file_path,
  referrer,
  NodeModuleKind::Esm,
  permissions,
  )?
  } else {
  Some(resolved_specifier)
  }
@@ -363,8 371,13 @@ impl NodeResolver {
  NodeResolutionMode::Types => {
  if resolved_url.scheme() == "file" {
  let path = resolved_url.to_file_path().unwrap();
- match self.path_to_declaration_path(path, node_module_kind) {
- Some(path) => to_file_specifier(&path),
  match self.path_to_declaration_url(
  path,
  referrer,
  node_module_kind,
  permissions,
  )? {
  Some(url) => url,
  None => return Ok(None),
  }
  } else {
@@ -448,11 461,13 @@ impl NodeResolver {
  }
 
  /// Checks if the resolved file has a corresponding declaration file.
- fn path_to_declaration_path(
  fn path_to_declaration_url(
  &self,
  path: PathBuf,
  referrer: &ModuleSpecifier,
  referrer_kind: NodeModuleKind,
- ) -> Option<PathBuf> {
  permissions: &dyn NodePermissions,
  ) -> Result<Option<ModuleSpecifier>, AnyError> {
  fn probe_extensions(
  fs: &dyn deno_fs::FileSystem,
  path: &Path,
@@ -502,29 517,49 @@ impl NodeResolver {
  || lowercase_path.ends_with(".d.cts")
  || lowercase_path.ends_with(".d.mts")
  {
- return Some(path);
  return Ok(Some(to_file_specifier(&path)));
  }
  if let Some(path) =
  probe_extensions(&*self.fs, &path, &lowercase_path, referrer_kind)
  {
- return Some(path);
  return Ok(Some(to_file_specifier(&path)));
  }
  if self.fs.is_dir_sync(&path) {
  let package_json_path = path.join("package.json");
  if let Ok(pkg_json) =
  self.load_package_json(permissions, package_json_path)
  {
  let maybe_resolution = self.resolve_package_subpath(
  &pkg_json,
  /* sub path */ ".",
  referrer,
  referrer_kind,
  match referrer_kind {
  NodeModuleKind::Esm => DEFAULT_CONDITIONS,
  NodeModuleKind::Cjs => REQUIRE_CONDITIONS,
  },
  NodeResolutionMode::Types,
  permissions,
  )?;
  if let Some(resolution) = maybe_resolution {
  return Ok(Some(resolution));
  }
  }
  let index_path = path.join("index.js");
  if let Some(path) = probe_extensions(
  &*self.fs,
  &index_path,
  &index_path.to_string_lossy().to_lowercase(),
  referrer_kind,
  ) {
- return Some(path);
  return Ok(Some(to_file_specifier(&path)));
  }
  }
  // allow resolving .css files for types resolution
  if lowercase_path.ends_with(".css") {
- return Some(path);
  return Ok(Some(to_file_specifier(&path)));
  }
- None
  Ok(None)
  }
 
  #[allow(clippy::too_many_arguments)]
@@ -771,30 806,30 @@ impl NodeResolver {
  permissions: &dyn NodePermissions,
  ) -> Result<Option<ModuleSpecifier>, AnyError> {
  if let Some(target) = target.as_str() {
- return self
- .resolve_package_target_string(
- target,
- subpath,
- package_subpath,
- package_json_path,
  let url = self.resolve_package_target_string(
  target,
  subpath,
  package_subpath,
  package_json_path,
  referrer,
  referrer_kind,
  pattern,
  internal,
  conditions,
  mode,
  permissions,
  )?;
  if mode.is_types() && url.scheme() == "file" {
  let path = url.to_file_path().unwrap();
  return self.path_to_declaration_url(
  path,
  referrer,
  referrer_kind,
- pattern,
- internal,
- conditions,
- mode,
  permissions,
- )
- .map(|url| {
- if mode.is_types() && url.scheme() == "file" {
- let path = url.to_file_path().unwrap();
- self
- .path_to_declaration_path(path, referrer_kind)
- .map(|path| to_file_specifier(&path))
- } else {
- Some(url)
- }
- });
  );
  } else {
  return Ok(Some(url));
  }
  } else if let Some(target_arr) = target.as_array() {
  if target_arr.is_empty() {
  return Ok(None);
@@ -1090,29 1125,36 @@ impl NodeResolver {
  Ok(found) => return Ok(Some(found)),
  Err(exports_err) => {
  if mode.is_types() && package_subpath == "." {
- if let Ok(Some(path)) =
- self.legacy_main_resolve(package_json, referrer_kind, mode)
- {
- return Ok(Some(to_file_specifier(&path)));
- } else {
- return Ok(None);
- }
  return self.legacy_main_resolve(
  package_json,
   referrer,
  referrer_kind,
   mode,
  permissions,
  );
  }
  return Err(exports_err);
  }
  }
  }
  if package_subpath == "." {
- return self
- .legacy_main_resolve(package_json, referrer_kind, mode)
- .map(|maybe_resolved| maybe_resolved.map(|p| to_file_specifier(&p)));
  return self.legacy_main_resolve(
  package_json,
  referrer,
  referrer_kind,
  mode,
  permissions,
  );
  }
 
  let file_path = package_json.path.parent().unwrap().join(package_subpath);
  if mode.is_types() {
- let maybe_declaration_path =
- self.path_to_declaration_path(file_path, referrer_kind);
- Ok(maybe_declaration_path.map(|p| to_file_specifier(&p)))
  self.path_to_declaration_url(
  file_path,
  referrer,
  referrer_kind,
  permissions,
  )
  } else {
  Ok(Some(to_file_specifier(&file_path)))
  }
@@ -1183,9 1225,11 @@ impl NodeResolver {
  pub(super) fn legacy_main_resolve(
  &self,
  package_json: &PackageJson,
  referrer: &ModuleSpecifier,
  referrer_kind: NodeModuleKind,
  mode: NodeResolutionMode,
- ) -> Result<Option<PathBuf>, AnyError> {
  permissions: &dyn NodePermissions,
  ) -> Result<Option<ModuleSpecifier>, AnyError> {
  let maybe_main = if mode.is_types() {
  match package_json.types.as_ref() {
  Some(types) => Some(types),
@@ -1194,9 1238,13 @@ impl NodeResolver {
  // a corresponding declaration file
  if let Some(main) = package_json.main(referrer_kind) {
  let main = package_json.path.parent().unwrap().join(main).clean();
- if let Some(path) =
- self.path_to_declaration_path(main, referrer_kind)
- {
  let maybe_decl_url = self.path_to_declaration_url(
  main,
  referrer,
  referrer_kind,
  permissions,
  )?;
  if let Some(path) = maybe_decl_url {
  return Ok(Some(path));
  }
  }
@@ -1210,7 1258,7 @@ impl NodeResolver {
  if let Some(main) = maybe_main {
  let guess = package_json.path.parent().unwrap().join(main).clean();
  if self.fs.is_file_sync(&guess) {
- return Ok(Some(guess));
  return Ok(Some(to_file_specifier(&guess)));
  }
 
  // todo(dsherret): investigate exactly how node and typescript handles this
@@ -1240,7 1288,7 @@ impl NodeResolver {
  .clean();
  if self.fs.is_file_sync(&guess) {
  // TODO(bartlomieju): emitLegacyIndexDeprecation()
- return Ok(Some(guess));
  return Ok(Some(to_file_specifier(&guess)));
  }
  }
  }
@@ -1263,7 1311,7 @@ impl NodeResolver {
  .clean();
  if self.fs.is_file_sync(&guess) {
  // TODO(bartlomieju): emitLegacyIndexDeprecation()
- return Ok(Some(guess));
  return Ok(Some(to_file_specifier(&guess)));
  }
  }
 

diff --git a/tests/specs/npm/check_pkg_json_import/__test__.json b/tests/specs/npm/check_pkg_json_import/__test__.json
@@ -0,0 1,5 @@
 {
  "base": "npm",
  "args": "check --all main.ts",
  "output": "main.out"
 }
diff --git a/tests/specs/npm/check_pkg_json_import/main.out b/tests/specs/npm/check_pkg_json_import/main.out
@@ -0,0 1,3 @@
 Download http://localhost:4545/npm/registry/@denotest/types-pkg-json-import
 Download http://localhost:4545/npm/registry/@denotest/types-pkg-json-import/1.0.0.tgz
 Check file:///[WILDLINE]/main.ts
diff --git a/tests/specs/npm/check_pkg_json_import/main.ts b/tests/specs/npm/check_pkg_json_import/main.ts
@@ -0,0 1,18 @@
 import { createContext } from "npm:@denotest/types-pkg-json-import";
 import { useContext } from "npm:@denotest/types-pkg-json-import/hooks";
 
 export interface Foo {
  foo: string;
 }
 
 export const CTX = createContext<Foo | undefined>(undefined);
 
 function unwrap(foo: Foo) {}
 
 export function useCSP() {
  const foo = useContext(CTX);
  // previously this was erroring
  if (foo) {
  unwrap(foo);
  }
 }
diff --git a/tests/testdata/npm/registry/@denotest/types-pkg-json-import/1.0.0/hooks/src/index.d.ts b/tests/testdata/npm/registry/@denotest/types-pkg-json-import/1.0.0/hooks/src/index.d.ts
@@ -0,0 1,4 @@
 // this directory import was not working (it should resolve via the package.json)
 import { PreactContext } from '../..';
 
 export declare function useContext<T>(context: PreactContext<T>): T;
diff --git a/tests/testdata/npm/registry/@denotest/types-pkg-json-import/1.0.0/package.json b/tests/testdata/npm/registry/@denotest/types-pkg-json-import/1.0.0/package.json
@@ -0,0 1,14 @@
 {
  "name": "@denotest/types-directory-import",
  "version": "1.0.0",
  "exports": {
  ".": {
  "types": "./src/index.d.ts",
  "import": "./dist/preact.mjs"
  },
  "./hooks": {
  "types": "./hooks/src/index.d.ts",
  "import": "./hooks/dist/hooks.mjs"
  }
  }
 }