From 1416713cb3af8a952b1ae9952091706e2540341c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartek=20Iwa=C5=84czuk?= Date: Wed, 16 Nov 2022 20:41:27 +0100 Subject: [PATCH] fix(npm): using types for packages with subpath (#16656) For CommonJS packages we were not trying different extensions for files specified as subpath of the package ([package_name]/[subpath]). This commit fixes that. --- cli/node/mod.rs | 29 +---------- cli/tests/integration/npm_tests.rs | 8 +++ .../registry/@denotest/types/1.0.0/index.d.ts | 4 ++ .../@denotest/types/1.0.0/package.json | 5 ++ .../@denotest/types_imported/1.0.0/index.d.ts | 4 ++ .../types_imported/1.0.0/package.json | 5 ++ .../types_imported/1.0.0/subpath.d.ts | 4 ++ cli/tests/testdata/npm/types/main.out | 51 +++++++++++++++++++ cli/tests/testdata/npm/types/main.ts | 18 +++++++ ext/node/lib.rs | 1 + ext/node/resolution.rs | 39 ++++++++++++-- 11 files changed, 137 insertions(+), 31 deletions(-) create mode 100644 cli/tests/testdata/npm/registry/@denotest/types/1.0.0/index.d.ts create mode 100644 cli/tests/testdata/npm/registry/@denotest/types/1.0.0/package.json create mode 100644 cli/tests/testdata/npm/registry/@denotest/types_imported/1.0.0/index.d.ts create mode 100644 cli/tests/testdata/npm/registry/@denotest/types_imported/1.0.0/package.json create mode 100644 cli/tests/testdata/npm/registry/@denotest/types_imported/1.0.0/subpath.d.ts create mode 100644 cli/tests/testdata/npm/types/main.out create mode 100644 cli/tests/testdata/npm/types/main.ts diff --git a/cli/node/mod.rs b/cli/node/mod.rs index 7bb28d9843..9ab4574ef3 100644 --- a/cli/node/mod.rs +++ b/cli/node/mod.rs @@ -25,6 +25,7 @@ use deno_runtime::deno_node::legacy_main_resolve; use deno_runtime::deno_node::package_exports_resolve; use deno_runtime::deno_node::package_imports_resolve; use deno_runtime::deno_node::package_resolve; +use deno_runtime::deno_node::path_to_declaration_path; use deno_runtime::deno_node::NodeModuleKind; use deno_runtime::deno_node::PackageJson; use deno_runtime::deno_node::PathClean; @@ -548,34 +549,6 @@ fn mode_conditions(mode: NodeResolutionMode) -> &'static [&'static str] { } } -/// Checks if the resolved file has a corresponding declaration file. -fn path_to_declaration_path( - path: PathBuf, - referrer_kind: NodeModuleKind, -) -> PathBuf { - let lowercase_path = path.to_string_lossy().to_lowercase(); - if lowercase_path.ends_with(".d.ts") - || lowercase_path.ends_with(".d.cts") - || lowercase_path.ends_with(".d.ts") - { - return path; - } - let specific_dts_path = match referrer_kind { - NodeModuleKind::Cjs => path.with_extension("d.cts"), - NodeModuleKind::Esm => path.with_extension("d.mts"), - }; - if specific_dts_path.exists() { - specific_dts_path - } else { - let dts_path = path.with_extension("d.ts"); - if dts_path.exists() { - dts_path - } else { - path - } - } -} - pub fn node_resolve_binary_export( pkg_req: &NpmPackageReq, bin_name: Option<&str>, diff --git a/cli/tests/integration/npm_tests.rs b/cli/tests/integration/npm_tests.rs index e6990f0e0d..e29c1452e5 100644 --- a/cli/tests/integration/npm_tests.rs +++ b/cli/tests/integration/npm_tests.rs @@ -266,6 +266,14 @@ itest!(check_local { exit_code: 1, }); +itest!(types { + args: "check --quiet npm/types/main.ts", + output: "npm/types/main.out", + envs: env_vars(), + http_server: true, + exit_code: 1, +}); + itest!(types_ambient_module { args: "check --quiet npm/types_ambient_module/main.ts", output: "npm/types_ambient_module/main.out", diff --git a/cli/tests/testdata/npm/registry/@denotest/types/1.0.0/index.d.ts b/cli/tests/testdata/npm/registry/@denotest/types/1.0.0/index.d.ts new file mode 100644 index 0000000000..afe876c4d8 --- /dev/null +++ b/cli/tests/testdata/npm/registry/@denotest/types/1.0.0/index.d.ts @@ -0,0 +1,4 @@ +export interface Fizzbuzz { + fizz: string; + buzz: string; +} \ No newline at end of file diff --git a/cli/tests/testdata/npm/registry/@denotest/types/1.0.0/package.json b/cli/tests/testdata/npm/registry/@denotest/types/1.0.0/package.json new file mode 100644 index 0000000000..ef927cbe35 --- /dev/null +++ b/cli/tests/testdata/npm/registry/@denotest/types/1.0.0/package.json @@ -0,0 +1,5 @@ +{ + "name": "@denotest/types-ambient", + "version": "1.0.0", + "types": "./index.d.ts" +} diff --git a/cli/tests/testdata/npm/registry/@denotest/types_imported/1.0.0/index.d.ts b/cli/tests/testdata/npm/registry/@denotest/types_imported/1.0.0/index.d.ts new file mode 100644 index 0000000000..559cdb2ecb --- /dev/null +++ b/cli/tests/testdata/npm/registry/@denotest/types_imported/1.0.0/index.d.ts @@ -0,0 +1,4 @@ +export interface SomeInterface { + prop: string; + prop2: number; +} \ No newline at end of file diff --git a/cli/tests/testdata/npm/registry/@denotest/types_imported/1.0.0/package.json b/cli/tests/testdata/npm/registry/@denotest/types_imported/1.0.0/package.json new file mode 100644 index 0000000000..ef927cbe35 --- /dev/null +++ b/cli/tests/testdata/npm/registry/@denotest/types_imported/1.0.0/package.json @@ -0,0 +1,5 @@ +{ + "name": "@denotest/types-ambient", + "version": "1.0.0", + "types": "./index.d.ts" +} diff --git a/cli/tests/testdata/npm/registry/@denotest/types_imported/1.0.0/subpath.d.ts b/cli/tests/testdata/npm/registry/@denotest/types_imported/1.0.0/subpath.d.ts new file mode 100644 index 0000000000..883cf037a2 --- /dev/null +++ b/cli/tests/testdata/npm/registry/@denotest/types_imported/1.0.0/subpath.d.ts @@ -0,0 +1,4 @@ +export interface Foobar { + foo: string; + bar: string; +} \ No newline at end of file diff --git a/cli/tests/testdata/npm/types/main.out b/cli/tests/testdata/npm/types/main.out new file mode 100644 index 0000000000..c439234784 --- /dev/null +++ b/cli/tests/testdata/npm/types/main.out @@ -0,0 +1,51 @@ +error: TS2322 [ERROR]: Type 'number' is not assignable to type 'string'. + bar: 1, + ~~~ + at [WILDCARD]/npm/types/main.ts:7:3 + + The expected type comes from property 'bar' which is declared here on type 'Foobar' + bar: string; + ~~~ + at [WILDCARD]/@denotest/types_imported/1.0.0/subpath.d.ts:3:5 + +TS2322 [ERROR]: Type 'number' is not assignable to type 'string'. + prop: 1, + ~~~~ + at [WILDCARD]/npm/types/main.ts:11:3 + + The expected type comes from property 'prop' which is declared here on type 'SomeInterface' + prop: string; + ~~~~ + at [WILDCARD]/@denotest/types_imported/1.0.0/index.d.ts:2:5 + +TS2322 [ERROR]: Type 'string' is not assignable to type 'number'. + prop2: "asdf", + ~~~~~ + at [WILDCARD]/npm/types/main.ts:12:3 + + The expected type comes from property 'prop2' which is declared here on type 'SomeInterface' + prop2: number; + ~~~~~ + at [WILDCARD]/@denotest/types_imported/1.0.0/index.d.ts:3:5 + +TS2322 [ERROR]: Type 'number' is not assignable to type 'string'. + fizz: 1, + ~~~~ + at [WILDCARD]/npm/types/main.ts:16:3 + + The expected type comes from property 'fizz' which is declared here on type 'Fizzbuzz' + fizz: string; + ~~~~ + at [WILDCARD]/@denotest/types/1.0.0/index.d.ts:2:3 + +TS2322 [ERROR]: Type 'number' is not assignable to type 'string'. + buzz: 2, + ~~~~ + at [WILDCARD]/npm/types/main.ts:17:3 + + The expected type comes from property 'buzz' which is declared here on type 'Fizzbuzz' + buzz: string; + ~~~~ + at [WILDCARD]/@denotest/types/1.0.0/index.d.ts:3:3 + +Found 5 errors. diff --git a/cli/tests/testdata/npm/types/main.ts b/cli/tests/testdata/npm/types/main.ts new file mode 100644 index 0000000000..324ed723f7 --- /dev/null +++ b/cli/tests/testdata/npm/types/main.ts @@ -0,0 +1,18 @@ +import type { Fizzbuzz } from "npm:@denotest/types"; +import type { SomeInterface } from "npm:@denotest/types_imported"; +import type { Foobar as FooInterface } from "npm:@denotest/types_imported/subpath"; + +const foobar: FooInterface = { + foo: "foo", + bar: 1, +}; + +const i: SomeInterface = { + prop: 1, + prop2: "asdf", +}; + +const fizzbuzz: Fizzbuzz = { + fizz: 1, + buzz: 2, +}; diff --git a/ext/node/lib.rs b/ext/node/lib.rs index b0d5485b47..d69d3b6fe5 100644 --- a/ext/node/lib.rs +++ b/ext/node/lib.rs @@ -27,6 +27,7 @@ pub use resolution::legacy_main_resolve; pub use resolution::package_exports_resolve; pub use resolution::package_imports_resolve; pub use resolution::package_resolve; +pub use resolution::path_to_declaration_path; pub use resolution::NodeModuleKind; pub use resolution::DEFAULT_CONDITIONS; pub use resolution::TYPES_CONDITIONS; diff --git a/ext/node/resolution.rs b/ext/node/resolution.rs index de8f6e87b2..9a6bc3aebe 100644 --- a/ext/node/resolution.rs +++ b/ext/node/resolution.rs @@ -27,6 +27,34 @@ pub enum NodeModuleKind { Cjs, } +/// Checks if the resolved file has a corresponding declaration file. +pub fn path_to_declaration_path( + path: PathBuf, + referrer_kind: NodeModuleKind, +) -> PathBuf { + let lowercase_path = path.to_string_lossy().to_lowercase(); + if lowercase_path.ends_with(".d.ts") + || lowercase_path.ends_with(".d.cts") + || lowercase_path.ends_with(".d.ts") + { + return path; + } + let specific_dts_path = match referrer_kind { + NodeModuleKind::Cjs => path.with_extension("d.cts"), + NodeModuleKind::Esm => path.with_extension("d.mts"), + }; + if specific_dts_path.exists() { + specific_dts_path + } else { + let dts_path = path.with_extension("d.ts"); + if dts_path.exists() { + dts_path + } else { + path + } + } +} + fn to_specifier_display_string(url: &ModuleSpecifier) -> String { if let Ok(path) = url.to_file_path() { path.display().to_string() @@ -659,9 +687,14 @@ pub fn package_resolve( return legacy_main_resolve(&package_json, referrer_kind, conditions); } - Ok(Some( - package_json.path.parent().unwrap().join(&package_subpath), - )) + let file_path = package_json.path.parent().unwrap().join(&package_subpath); + + if conditions == TYPES_CONDITIONS { + let declaration_path = path_to_declaration_path(file_path, referrer_kind); + Ok(Some(declaration_path)) + } else { + Ok(Some(file_path)) + } } pub fn get_package_scope_config(