diff options
25 files changed, 149 insertions, 123 deletions
diff --git a/cli/lsp/analysis.rs b/cli/lsp/analysis.rs index f8ace060a..e62294012 100644 --- a/cli/lsp/analysis.rs +++ b/cli/lsp/analysis.rs @@ -8,6 +8,7 @@ use super::tsc; use crate::npm::CliNpmResolver; use crate::tools::lint::create_linter; +use crate::util::path::specifier_to_file_path; use deno_ast::SourceRange; use deno_ast::SourceRangedForSpanned; @@ -20,9 +21,10 @@ use deno_core::serde_json; use deno_core::serde_json::json; use deno_core::ModuleSpecifier; use deno_lint::rules::LintRule; +use deno_runtime::deno_node::NodeResolver; use deno_runtime::deno_node::NpmResolver; -use deno_runtime::deno_node::PackageJson; use deno_runtime::deno_node::PathClean; +use deno_runtime::permissions::PermissionsContainer; use deno_semver::package::PackageReq; use import_map::ImportMap; use once_cell::sync::Lazy; @@ -161,6 +163,7 @@ fn code_as_string(code: &Option<lsp::NumberOrString>) -> String { pub struct TsResponseImportMapper<'a> { documents: &'a Documents, maybe_import_map: Option<&'a ImportMap>, + node_resolver: Option<&'a NodeResolver>, npm_resolver: Option<&'a dyn CliNpmResolver>, } @@ -168,11 +171,13 @@ impl<'a> TsResponseImportMapper<'a> { pub fn new( documents: &'a Documents, maybe_import_map: Option<&'a ImportMap>, + node_resolver: Option<&'a NodeResolver>, npm_resolver: Option<&'a dyn CliNpmResolver>, ) -> Self { Self { documents, maybe_import_map, + node_resolver, npm_resolver, } } @@ -249,18 +254,14 @@ impl<'a> TsResponseImportMapper<'a> { &self, specifier: &ModuleSpecifier, ) -> Option<String> { - let specifier_path = specifier.to_file_path().ok()?; - let root_folder = self - .npm_resolver - .as_ref() - .and_then(|r| r.resolve_package_folder_from_path(specifier).ok()) + let node_resolver = self.node_resolver?; + let package_json = node_resolver + .get_closest_package_json(specifier, &PermissionsContainer::allow_all()) + .ok() .flatten()?; - let package_json_path = root_folder.join("package.json"); - let package_json_text = std::fs::read_to_string(&package_json_path).ok()?; - let package_json = - PackageJson::load_from_string(package_json_path, package_json_text) - .ok()?; + let root_folder = package_json.path.parent()?; + let specifier_path = specifier_to_file_path(specifier).ok()?; let mut search_paths = vec![specifier_path.clone()]; // TypeScript will provide a .js extension for quick fixes, so do // a search for the .d.ts file instead @@ -271,7 +272,7 @@ impl<'a> TsResponseImportMapper<'a> { for search_path in search_paths { if let Some(exports) = &package_json.exports { if let Some(result) = try_reverse_map_package_json_exports( - &root_folder, + root_folder, &search_path, exports, ) { diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index 8b275a650..435daee90 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -2127,6 +2127,7 @@ impl Inner { TsResponseImportMapper::new( &self.documents, self.maybe_import_map.as_deref(), + self.npm.node_resolver.as_deref(), self.npm.resolver.as_deref(), ) } diff --git a/cli/npm/byonm.rs b/cli/npm/byonm.rs index 6ddac42e4..da36e3b53 100644 --- a/cli/npm/byonm.rs +++ b/cli/npm/byonm.rs @@ -91,16 +91,16 @@ impl NpmResolver for ByonmCliNpmResolver { fn inner( fs: &dyn FileSystem, name: &str, - package_root_path: &Path, 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 = package_root_path; + let mut current_folder = referrer_file.parent().unwrap(); loop { let node_modules_folder = if current_folder.ends_with("node_modules") { Cow::Borrowed(current_folder) @@ -135,48 +135,10 @@ impl NpmResolver for ByonmCliNpmResolver { ); } - let package_root_path = - self.resolve_package_folder_from_path(referrer)?.unwrap(); // todo(byonm): don't unwrap - let path = inner(&*self.fs, name, &package_root_path, referrer, mode)?; + let path = inner(&*self.fs, name, referrer, mode)?; Ok(self.fs.realpath_sync(&path)?) } - fn resolve_package_folder_from_path( - &self, - specifier: &deno_core::ModuleSpecifier, - ) -> Result<Option<PathBuf>, AnyError> { - let path = specifier.to_file_path().unwrap(); // todo(byonm): don't unwrap - let path = self.fs.realpath_sync(&path)?; - if self.in_npm_package(specifier) { - let mut path = path.as_path(); - while let Some(parent) = path.parent() { - if parent - .file_name() - .and_then(|f| f.to_str()) - .map(|s| s.to_ascii_lowercase()) - .as_deref() - == Some("node_modules") - { - return Ok(Some(path.to_path_buf())); - } else { - path = parent; - } - } - } else { - // find the folder with a package.json - // todo(dsherret): not exactly correct, but good enough for a first pass - let mut path = path.as_path(); - while let Some(parent) = path.parent() { - if self.fs.exists_sync(&parent.join("package.json")) { - return Ok(Some(parent.to_path_buf())); - } else { - path = parent; - } - } - } - Ok(None) - } - fn in_npm_package(&self, specifier: &ModuleSpecifier) -> bool { specifier.scheme() == "file" && specifier diff --git a/cli/npm/managed/mod.rs b/cli/npm/managed/mod.rs index 68b5c2134..d59b81912 100644 --- a/cli/npm/managed/mod.rs +++ b/cli/npm/managed/mod.rs @@ -505,24 +505,6 @@ impl NpmResolver for ManagedCliNpmResolver { Ok(path) } - fn resolve_package_folder_from_path( - &self, - specifier: &ModuleSpecifier, - ) -> Result<Option<PathBuf>, AnyError> { - let Some(path) = self - .fs_resolver - .resolve_package_folder_from_specifier(specifier)? - else { - return Ok(None); - }; - log::debug!( - "Resolved package folder of {} to {}", - specifier, - path.display() - ); - Ok(Some(path)) - } - fn in_npm_package(&self, specifier: &ModuleSpecifier) -> bool { let root_dir_url = self.fs_resolver.root_dir_url(); debug_assert!(root_dir_url.as_str().ends_with('/')); diff --git a/cli/tests/integration/npm_tests.rs b/cli/tests/integration/npm_tests.rs index 5104036e3..927e83475 100644 --- a/cli/tests/integration/npm_tests.rs +++ b/cli/tests/integration/npm_tests.rs @@ -2505,3 +2505,28 @@ console.log(add(1, 2)); .run(); output.assert_matches_text("Check file:///[WILDCARD]/project-b/main.ts\n"); } + +itest!(imports_package_json { + args: "run --node-modules-dir=false npm/imports_package_json/main.js", + output: "npm/imports_package_json/main.out", + envs: env_vars_for_npm_tests(), + http_server: true, +}); + +itest!(imports_package_json_import_not_defined { + args: + "run --node-modules-dir=false npm/imports_package_json/import_not_defined.js", + output: "npm/imports_package_json/import_not_defined.out", + envs: env_vars_for_npm_tests(), + exit_code: 1, + http_server: true, +}); + +itest!(imports_package_json_sub_path_import_not_defined { + args: + "run --node-modules-dir=false npm/imports_package_json/sub_path_import_not_defined.js", + output: "npm/imports_package_json/sub_path_import_not_defined.out", + envs: env_vars_for_npm_tests(), + exit_code: 1, + http_server: true, +}); diff --git a/cli/tests/testdata/npm/imports_package_json/import_not_defined.js b/cli/tests/testdata/npm/imports_package_json/import_not_defined.js new file mode 100644 index 000000000..dc4d2df16 --- /dev/null +++ b/cli/tests/testdata/npm/imports_package_json/import_not_defined.js @@ -0,0 +1,3 @@ +import data from "@denotest/imports-package-json/import-not-defined"; + +console.log(data); diff --git a/cli/tests/testdata/npm/imports_package_json/import_not_defined.out b/cli/tests/testdata/npm/imports_package_json/import_not_defined.out new file mode 100644 index 000000000..3580d9007 --- /dev/null +++ b/cli/tests/testdata/npm/imports_package_json/import_not_defined.out @@ -0,0 +1,6 @@ +Download http://localhost:4545/npm/registry/@denotest/imports-package-json +Download http://localhost:4545/npm/registry/@denotest/imports-package-json/1.0.0.tgz +error: Could not resolve '#not-defined' from 'file:///[WILDCARD]/@denotest/imports-package-json/1.0.0/import_not_defined.js'. + +Caused by: + [ERR_PACKAGE_IMPORT_NOT_DEFINED] Package import specifier "#not-defined" is not defined in package [WILDCARD]package.json imported from [WILDCARD]import_not_defined.js diff --git a/cli/tests/testdata/npm/imports_package_json/main.js b/cli/tests/testdata/npm/imports_package_json/main.js new file mode 100644 index 000000000..cd7bbf8e9 --- /dev/null +++ b/cli/tests/testdata/npm/imports_package_json/main.js @@ -0,0 +1,4 @@ +import data from "@denotest/imports-package-json"; + +console.log(data.hi); +console.log(data.bye); diff --git a/cli/tests/testdata/npm/imports_package_json/main.out b/cli/tests/testdata/npm/imports_package_json/main.out new file mode 100644 index 000000000..f006f18d5 --- /dev/null +++ b/cli/tests/testdata/npm/imports_package_json/main.out @@ -0,0 +1,4 @@ +Download http://localhost:4545/npm/registry/@denotest/imports-package-json +Download http://localhost:4545/npm/registry/@denotest/imports-package-json/1.0.0.tgz +hi +bye diff --git a/cli/tests/testdata/npm/imports_package_json/package.json b/cli/tests/testdata/npm/imports_package_json/package.json new file mode 100644 index 000000000..cb6a08d1a --- /dev/null +++ b/cli/tests/testdata/npm/imports_package_json/package.json @@ -0,0 +1,6 @@ +{ + "name": "my-test", + "dependencies": { + "@denotest/imports-package-json": "1.0.0" + } +} diff --git a/cli/tests/testdata/npm/imports_package_json/sub_path_import_not_defined.js b/cli/tests/testdata/npm/imports_package_json/sub_path_import_not_defined.js new file mode 100644 index 000000000..f1097aa06 --- /dev/null +++ b/cli/tests/testdata/npm/imports_package_json/sub_path_import_not_defined.js @@ -0,0 +1,3 @@ +import data from "@denotest/imports-package-json/sub-path-import-not-defined"; + +console.log(data); diff --git a/cli/tests/testdata/npm/imports_package_json/sub_path_import_not_defined.out b/cli/tests/testdata/npm/imports_package_json/sub_path_import_not_defined.out new file mode 100644 index 000000000..04a21c99e --- /dev/null +++ b/cli/tests/testdata/npm/imports_package_json/sub_path_import_not_defined.out @@ -0,0 +1,6 @@ +Download http://localhost:4545/npm/registry/@denotest/imports-package-json +Download http://localhost:4545/npm/registry/@denotest/imports-package-json/1.0.0.tgz +error: Could not resolve '#hi' from 'file:///[WILDCARD]/@denotest/imports-package-json/1.0.0/sub_path/import_not_defined.js'. + +Caused by: + [ERR_PACKAGE_IMPORT_NOT_DEFINED] Package import specifier "#hi" is not defined in package [WILDCARD]sub_path[WILDCARD]package.json imported from [WILDCARD]import_not_defined.js diff --git a/cli/tests/testdata/npm/registry/@denotest/imports-package-json/1.0.0/hi.js b/cli/tests/testdata/npm/registry/@denotest/imports-package-json/1.0.0/hi.js new file mode 100644 index 000000000..407090812 --- /dev/null +++ b/cli/tests/testdata/npm/registry/@denotest/imports-package-json/1.0.0/hi.js @@ -0,0 +1 @@ +export default "hi"; diff --git a/cli/tests/testdata/npm/registry/@denotest/imports-package-json/1.0.0/import_not_defined.js b/cli/tests/testdata/npm/registry/@denotest/imports-package-json/1.0.0/import_not_defined.js new file mode 100644 index 000000000..07864fd2c --- /dev/null +++ b/cli/tests/testdata/npm/registry/@denotest/imports-package-json/1.0.0/import_not_defined.js @@ -0,0 +1,3 @@ +import notDefined from "#not-defined"; + +export default notDefined; diff --git a/cli/tests/testdata/npm/registry/@denotest/imports-package-json/1.0.0/main.js b/cli/tests/testdata/npm/registry/@denotest/imports-package-json/1.0.0/main.js new file mode 100644 index 000000000..46625479e --- /dev/null +++ b/cli/tests/testdata/npm/registry/@denotest/imports-package-json/1.0.0/main.js @@ -0,0 +1,7 @@ +import hi from "#hi"; +import bye from "./sub_path/main.js"; + +export default { + hi, + bye, +}; diff --git a/cli/tests/testdata/npm/registry/@denotest/imports-package-json/1.0.0/package.json b/cli/tests/testdata/npm/registry/@denotest/imports-package-json/1.0.0/package.json new file mode 100644 index 000000000..a4fbc8889 --- /dev/null +++ b/cli/tests/testdata/npm/registry/@denotest/imports-package-json/1.0.0/package.json @@ -0,0 +1,16 @@ +{ + "name": "imports-package-json", + "type": "module", + "version": "1.0.0", + "description": "", + "license": "ISC", + "author": "", + "exports": { + ".": "./main.js", + "./import-not-defined": "./import_not_defined.js", + "./sub-path-import-not-defined": "./sub_path/import_not_defined.js" + }, + "imports": { + "#hi": "./hi.js" + } +} diff --git a/cli/tests/testdata/npm/registry/@denotest/imports-package-json/1.0.0/sub_path/bye.js b/cli/tests/testdata/npm/registry/@denotest/imports-package-json/1.0.0/sub_path/bye.js new file mode 100644 index 000000000..6fc719e48 --- /dev/null +++ b/cli/tests/testdata/npm/registry/@denotest/imports-package-json/1.0.0/sub_path/bye.js @@ -0,0 +1 @@ +export default "bye"; diff --git a/cli/tests/testdata/npm/registry/@denotest/imports-package-json/1.0.0/sub_path/import_not_defined.js b/cli/tests/testdata/npm/registry/@denotest/imports-package-json/1.0.0/sub_path/import_not_defined.js new file mode 100644 index 000000000..ffaa2b1ad --- /dev/null +++ b/cli/tests/testdata/npm/registry/@denotest/imports-package-json/1.0.0/sub_path/import_not_defined.js @@ -0,0 +1,4 @@ +// this won't be defined in the closest package.json and will fail +import hi from "#hi"; + +export default hi; diff --git a/cli/tests/testdata/npm/registry/@denotest/imports-package-json/1.0.0/sub_path/main.js b/cli/tests/testdata/npm/registry/@denotest/imports-package-json/1.0.0/sub_path/main.js new file mode 100644 index 000000000..260ca79ae --- /dev/null +++ b/cli/tests/testdata/npm/registry/@denotest/imports-package-json/1.0.0/sub_path/main.js @@ -0,0 +1,3 @@ +import bye from "#bye"; + +export default bye; diff --git a/cli/tests/testdata/npm/registry/@denotest/imports-package-json/1.0.0/sub_path/package.json b/cli/tests/testdata/npm/registry/@denotest/imports-package-json/1.0.0/sub_path/package.json new file mode 100644 index 000000000..3f2c2bbd8 --- /dev/null +++ b/cli/tests/testdata/npm/registry/@denotest/imports-package-json/1.0.0/sub_path/package.json @@ -0,0 +1,6 @@ +{ + "type": "module", + "imports": { + "#bye": "./bye.js" + } +} diff --git a/ext/node/errors.rs b/ext/node/errors.rs index e72cb8557..e07a8347a 100644 --- a/ext/node/errors.rs +++ b/ext/node/errors.rs @@ -138,11 +138,12 @@ pub fn err_package_import_not_defined( base: &str, ) -> AnyError { let mut msg = format!( - "[ERR_PACKAGE_IMPORT_NOT_DEFINED] Package import specifier \"{specifier}\" is not defined in" + "[ERR_PACKAGE_IMPORT_NOT_DEFINED] Package import specifier \"{specifier}\" is not defined" ); if let Some(package_path) = package_path { - msg = format!("{msg} in package {package_path}package.json"); + let pkg_json_path = PathBuf::from(package_path).join("package.json"); + msg = format!("{} in package {}", msg, pkg_json_path.display()); } msg = format!("{msg} imported from {base}"); diff --git a/ext/node/lib.rs b/ext/node/lib.rs index 730554f2d..7a43fc4d4 100644 --- a/ext/node/lib.rs +++ b/ext/node/lib.rs @@ -81,12 +81,6 @@ pub trait NpmResolver: std::fmt::Debug + MaybeSend + MaybeSync { mode: NodeResolutionMode, ) -> Result<PathBuf, AnyError>; - /// Resolves the npm package folder path from the specified path. - fn resolve_package_folder_from_path( - &self, - specifier: &ModuleSpecifier, - ) -> Result<Option<PathBuf>, AnyError>; - fn in_npm_package(&self, specifier: &ModuleSpecifier) -> bool; fn in_npm_package_at_path(&self, path: &Path) -> bool { diff --git a/ext/node/ops/require.rs b/ext/node/ops/require.rs index 2cf59b0a3..ceb771bf9 100644 --- a/ext/node/ops/require.rs +++ b/ext/node/ops/require.rs @@ -389,7 +389,7 @@ where let node_resolver = state.borrow::<Rc<NodeResolver>>(); let permissions = state.borrow::<P>(); let pkg = node_resolver - .get_package_scope_config( + .get_closest_package_json( &Url::from_file_path(parent_path.unwrap()).unwrap(), permissions, ) diff --git a/ext/node/polyfills/internal/errors.ts b/ext/node/polyfills/internal/errors.ts index 2ba7ec28e..7f7ce9d3a 100644 --- a/ext/node/polyfills/internal/errors.ts +++ b/ext/node/polyfills/internal/errors.ts @@ -2421,7 +2421,7 @@ export class ERR_INVALID_PACKAGE_TARGET extends NodeError { if (key === ".") { assert(isImport === false); msg = `Invalid "exports" main target ${JSON.stringify(target)} defined ` + - `in the package config ${pkgPath}package.json${ + `in the package config ${displayJoin(pkgPath, "package.json")}${ base ? ` imported from ${base}` : "" }${relError ? '; targets must start with "./"' : ""}`; } else { @@ -2429,9 +2429,11 @@ export class ERR_INVALID_PACKAGE_TARGET extends NodeError { JSON.stringify( target, ) - } defined for '${key}' in the package config ${pkgPath}package.json${ - base ? ` imported from ${base}` : "" - }${relError ? '; targets must start with "./"' : ""}`; + } defined for '${key}' in the package config ${ + displayJoin(pkgPath, "package.json") + }${base ? ` imported from ${base}` : ""}${ + relError ? '; targets must start with "./"' : "" + }`; } super("ERR_INVALID_PACKAGE_TARGET", msg); } @@ -2444,7 +2446,9 @@ export class ERR_PACKAGE_IMPORT_NOT_DEFINED extends NodeTypeError { base: string, ) { const msg = `Package import specifier "${specifier}" is not defined${ - packagePath ? ` in package ${packagePath}package.json` : "" + packagePath + ? ` in package ${displayJoin(packagePath, "package.json")}` + : "" } imported from ${base}`; super("ERR_PACKAGE_IMPORT_NOT_DEFINED", msg); @@ -2455,14 +2459,13 @@ export class ERR_PACKAGE_PATH_NOT_EXPORTED extends NodeError { constructor(subpath: string, pkgPath: string, basePath?: string) { let msg: string; if (subpath === ".") { - msg = `No "exports" main defined in ${pkgPath}package.json${ - basePath ? ` imported from ${basePath}` : "" - }`; + msg = `No "exports" main defined in ${ + displayJoin(pkgPath, "package.json") + }${basePath ? ` imported from ${basePath}` : ""}`; } else { - msg = - `Package subpath '${subpath}' is not defined by "exports" in ${pkgPath}package.json${ - basePath ? ` imported from ${basePath}` : "" - }`; + msg = `Package subpath '${subpath}' is not defined by "exports" in ${ + displayJoin(pkgPath, "package.json") + }${basePath ? ` imported from ${basePath}` : ""}`; } super("ERR_PACKAGE_PATH_NOT_EXPORTED", msg); @@ -2621,6 +2624,12 @@ function determineSpecificType(value: any) { return `type ${typeof value} (${inspected})`; } +// Non-robust path join +function displayJoin(dir: string, fileName: string) { + const sep = dir.includes("\\") ? "\\" : "/"; + return dir.endsWith(sep) ? dir + fileName : dir + sep + fileName; +} + export { codes, genericNodeError, hideStackFrames }; export default { diff --git a/ext/node/resolution.rs b/ext/node/resolution.rs index cb4fef405..65c2e1bcd 100644 --- a/ext/node/resolution.rs +++ b/ext/node/resolution.rs @@ -543,7 +543,7 @@ impl NodeResolver { let mut package_json_path = None; if let Some(package_config) = - self.get_package_scope_config(referrer, permissions)? + self.get_closest_package_json(referrer, permissions)? { if package_config.exists { package_json_path = Some(package_config.path.clone()); @@ -977,12 +977,12 @@ impl NodeResolver { let (package_name, package_subpath, _is_scoped) = parse_npm_pkg_name(specifier, referrer)?; - // ResolveSelf let Some(package_config) = - self.get_package_scope_config(referrer, permissions)? + self.get_closest_package_json(referrer, permissions)? else { return Ok(None); }; + // ResolveSelf if package_config.exists && package_config.name.as_ref() == Some(&package_name) { @@ -1086,24 +1086,7 @@ impl NodeResolver { } } - pub(super) fn get_package_scope_config( - &self, - referrer: &ModuleSpecifier, - permissions: &dyn NodePermissions, - ) -> Result<Option<PackageJson>, AnyError> { - let Some(root_folder) = self - .npm_resolver - .resolve_package_folder_from_path(referrer)? - else { - return Ok(None); - }; - let package_json_path = root_folder.join("package.json"); - self - .load_package_json(permissions, package_json_path) - .map(Some) - } - - pub(super) fn get_closest_package_json( + pub fn get_closest_package_json( &self, url: &ModuleSpecifier, permissions: &dyn NodePermissions, @@ -1121,7 +1104,9 @@ impl NodeResolver { &self, url: &ModuleSpecifier, ) -> Result<Option<PathBuf>, AnyError> { - let file_path = url.to_file_path().unwrap(); + let Ok(file_path) = url.to_file_path() else { + return Ok(None); + }; let current_dir = deno_core::strip_unc_prefix( self.fs.realpath_sync(file_path.parent().unwrap())?, ); @@ -1130,15 +1115,8 @@ impl NodeResolver { if self.fs.exists_sync(&package_json_path) { return Ok(Some(package_json_path)); } - let Some(root_pkg_folder) = - self.npm_resolver.resolve_package_folder_from_path( - &ModuleSpecifier::from_directory_path(current_dir).unwrap(), - )? - else { - return Ok(None); - }; - while current_dir.starts_with(&root_pkg_folder) { - current_dir = current_dir.parent().unwrap(); + while let Some(parent) = current_dir.parent() { + current_dir = parent; let package_json_path = current_dir.join("package.json"); if self.fs.exists_sync(&package_json_path) { return Ok(Some(package_json_path)); |