From bc782cee98096d2562d87c7a0b15051b6d0d6628 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Wed, 13 Mar 2024 22:37:56 -0400 Subject: [PATCH] fix(node): resolve types via package.json for directory import (#22878) Does a package resolve when resolving types for a directory (copying the behaviour that typescript does). --- ext/node/polyfills/01_require.js | 25 --- ext/node/resolution.rs | 160 ++++++++++++------ .../npm/check_pkg_json_import/__test__.json | 5 + .../specs/npm/check_pkg_json_import/main.out | 3 + tests/specs/npm/check_pkg_json_import/main.ts | 18 ++ .../1.0.0/hooks/src/index.d.ts | 4 + .../types-pkg-json-import/1.0.0/package.json | 14 ++ .../1.0.0/src/index.d.ts | 76 +++++++++ tools/lint.js | 2 + 9 files changed, 226 insertions(+), 81 deletions(-) create mode 100644 tests/specs/npm/check_pkg_json_import/__test__.json create mode 100644 tests/specs/npm/check_pkg_json_import/main.out create mode 100644 tests/specs/npm/check_pkg_json_import/main.ts create mode 100644 tests/testdata/npm/registry/@denotest/types-pkg-json-import/1.0.0/hooks/src/index.d.ts create mode 100644 tests/testdata/npm/registry/@denotest/types-pkg-json-import/1.0.0/package.json create mode 100644 tests/testdata/npm/registry/@denotest/types-pkg-json-import/1.0.0/src/index.d.ts diff --git a/ext/node/polyfills/01_require.js b/ext/node/polyfills/01_require.js index 8136bc68e6..8d34a51b1b 100644 --- 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 = /%2F|%2C/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 index a1ee0e9ef7..d878a50594 100644 --- 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 { + permissions: &dyn NodePermissions, + ) -> Result, AnyError> { fn probe_extensions( fs: &dyn deno_fs::FileSystem, path: &Path, @@ -502,14 +517,34 @@ 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, @@ -517,14 +552,14 @@ impl NodeResolver { &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, 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, AnyError> { + permissions: &dyn NodePermissions, + ) -> Result, 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 new file mode 100644 index 0000000000..ce8e532802 --- /dev/null +++ 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 new file mode 100644 index 0000000000..1f436e6131 --- /dev/null +++ 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 new file mode 100644 index 0000000000..72b8877352 --- /dev/null +++ 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(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 new file mode 100644 index 0000000000..a0fe33a3d3 --- /dev/null +++ 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(context: PreactContext): 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 new file mode 100644 index 0000000000..3f9792f225 --- /dev/null +++ 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" + } + } +} \ No newline at end of file diff --git a/tests/testdata/npm/registry/@denotest/types-pkg-json-import/1.0.0/src/index.d.ts b/tests/testdata/npm/registry/@denotest/types-pkg-json-import/1.0.0/src/index.d.ts new file mode 100644 index 0000000000..94e6b25721 --- /dev/null +++ b/tests/testdata/npm/registry/@denotest/types-pkg-json-import/1.0.0/src/index.d.ts @@ -0,0 +1,76 @@ +export as namespace preact; + +export interface VNode

{ + type: any | string; + props: P & { children: ComponentChildren }; + key: Key; + /** + * ref is not guaranteed by React.ReactElement, for compatibility reasons + * with popular react libs we define it as optional too + */ + ref?: Ref | null; + /** + * The time this `vnode` started rendering. Will only be set when + * the devtools are attached. + * Default value: `0` + */ + startTime?: number; + /** + * The time that the rendering of this `vnode` was completed. Will only be + * set when the devtools are attached. + * Default value: `-1` + */ + endTime?: number; +} + +export type Key = string | number | any; + +export type RefObject = { current: T | null }; +export type RefCallback = (instance: T | null) => void; +export type Ref = RefObject | RefCallback | null; + +export type ComponentChild = + | VNode + | object + | string + | number + | bigint + | boolean + | null + | undefined; +export type ComponentChildren = ComponentChild[] | ComponentChild; + +export interface FunctionComponent

{ + (props: any, context?: any): VNode | null; + displayName?: string; + defaultProps?: Partial

| undefined; +} +export interface FunctionalComponent

extends FunctionComponent

{} + +// +// Context +// ----------------------------------- +export interface Consumer + extends FunctionComponent<{ + children: (value: T) => ComponentChildren; + }> {} +export interface PreactConsumer extends Consumer {} + +export interface Provider + extends FunctionComponent<{ + value: T; + children?: ComponentChildren; + }> {} +export interface PreactProvider extends Provider {} +export type ContextType> = C extends Context + ? T + : never; + +export interface Context { + Consumer: Consumer; + Provider: Provider; + displayName?: string; +} +export interface PreactContext extends Context {} + +export function createContext(defaultValue: T): Context; diff --git a/tools/lint.js b/tools/lint.js index 6cda0cef06..f48f3e8502 100755 --- a/tools/lint.js +++ b/tools/lint.js @@ -50,6 +50,8 @@ async function dlint() { ":!:cli/bench/testdata/react-dom.js", ":!:cli/compilers/wasm_wrap.js", ":!:cli/tsc/dts/**", + ":!:target/**", + ":!:tests/specs/**", ":!:tests/testdata/encoding/**", ":!:tests/testdata/error_syntax.js", ":!:tests/testdata/file_extensions/ts_with_js_extension.js",