Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(node): resolve types via package.json for directory import #22878

Merged
merged 6 commits into from
Mar 14, 2024
Merged

fix(node): resolve types via package.json for directory import #22878

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift click to select a range
7f79018
Start working on directory resolution issue
dsherret Mar 12, 2024
60c0f10
Update
dsherret Mar 12, 2024
f404ce7
Merge branch 'main' into fix_directory_resolution_error
dsherret Mar 13, 2024
52d93c8
Fix.
dsherret Mar 13, 2024
a0adbce
Merge branch 'main' into fix_directory_resolution_error
dsherret Mar 13, 2024
e2f3c87
Ignore specs folder from linting
dsherret Mar 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 0 additions & 25 deletions ext/node/polyfills/01_require.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
);
}
Comment on lines -514 to -518
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC this was quite an important bit and some packages were checking for this ERR_MODULE_NOT_FOUND code property. Does this value still match after this change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code was not used so I removed it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, let's land as is, but just a warning for the future. I'm not 100% sure this was the exact code path that was checking for this code.


// This only applies to requests of a specific form:
// 1. name/.*
Expand Down
160 changes: 104 additions & 56 deletions ext/node/resolution.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
}
}
Expand All @@ -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)
}
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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));
}
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into it and this seems to be done first before the index.d.ts probing.

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)]
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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)))
}
Expand Down Expand Up @@ -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),
Expand All @@ -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));
}
}
Expand All @@ -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
Expand Down Expand Up @@ -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)));
}
}
}
Expand All @@ -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)));
}
}

Expand Down
5 changes: 5 additions & 0 deletions tests/specs/npm/check_pkg_json_import/__test__.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 1,5 @@
{
"base": "npm",
"args": "check --all main.ts",
"output": "main.out"
}
3 changes: 3 additions & 0 deletions tests/specs/npm/check_pkg_json_import/main.out
View file
Open in desktop
dsherret marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -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
18 changes: 18 additions & 0 deletions tests/specs/npm/check_pkg_json_import/main.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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);
}
}
4 changes: 4 additions & 0 deletions tests/testdata/npm/registry/@denotest/types-pkg-json-import/1.0.0/hooks/src/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
14 changes: 14 additions & 0 deletions tests/testdata/npm/registry/@denotest/types-pkg-json-import/1.0.0/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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"
}
}
}
Loading
Loading