diff --git a/cli/npm/byonm.rs b/cli/npm/byonm.rs index 7aba839159..6ddac42e4b 100644 --- a/cli/npm/byonm.rs +++ b/cli/npm/byonm.rs @@ -46,6 +46,41 @@ pub struct ByonmCliNpmResolver { root_node_modules_dir: PathBuf, } +impl ByonmCliNpmResolver { + /// Finds the ancestor package.json that contains the specified dependency. + pub fn find_ancestor_package_json_with_dep( + &self, + dep_name: &str, + referrer: &ModuleSpecifier, + ) -> Option { + let referrer_path = referrer.to_file_path().ok()?; + let mut current_folder = referrer_path.parent()?; + loop { + let pkg_json_path = current_folder.join("package.json"); + if let Ok(pkg_json) = + PackageJson::load_skip_read_permission(self.fs.as_ref(), pkg_json_path) + { + if let Some(deps) = &pkg_json.dependencies { + if deps.contains_key(dep_name) { + return Some(pkg_json); + } + } + if let Some(deps) = &pkg_json.dev_dependencies { + if deps.contains_key(dep_name) { + return Some(pkg_json); + } + } + } + + if let Some(parent) = current_folder.parent() { + current_folder = parent; + } else { + return None; + } + } + } +} + impl NpmResolver for ByonmCliNpmResolver { fn resolve_package_folder_from_package( &self, @@ -60,6 +95,11 @@ impl NpmResolver for ByonmCliNpmResolver { referrer: &ModuleSpecifier, mode: NodeResolutionMode, ) -> Result { + 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; loop { let node_modules_folder = if current_folder.ends_with("node_modules") { @@ -69,9 +109,8 @@ impl NpmResolver for ByonmCliNpmResolver { }; // attempt to resolve the types package first, then fallback to the regular package - if mode.is_types() && !name.starts_with("@types/") { - let sub_dir = - join_package_name(&node_modules_folder, &types_package_name(name)); + if let Some(types_pkg_name) = &types_pkg_name { + let sub_dir = join_package_name(&node_modules_folder, types_pkg_name); if fs.is_dir_sync(&sub_dir) { return Ok(sub_dir); } @@ -128,7 +167,7 @@ impl NpmResolver for ByonmCliNpmResolver { // 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 parent.join("package.json").exists() { + if self.fs.exists_sync(&parent.join("package.json")) { return Ok(Some(parent.to_path_buf())); } else { path = parent; diff --git a/cli/npm/mod.rs b/cli/npm/mod.rs index 761c99dba4..c9f261ccd3 100644 --- a/cli/npm/mod.rs +++ b/cli/npm/mod.rs @@ -13,6 +13,7 @@ use deno_core::error::AnyError; use deno_runtime::deno_node::NpmResolver; use deno_semver::package::PackageReq; +pub use self::byonm::ByonmCliNpmResolver; pub use self::byonm::CliNpmResolverByonmCreateOptions; pub use self::cache_dir::NpmCacheDir; pub use self::managed::CliNpmResolverManagedCreateOptions; @@ -20,8 +21,6 @@ pub use self::managed::CliNpmResolverManagedPackageJsonInstallerOption; pub use self::managed::CliNpmResolverManagedSnapshotOption; pub use self::managed::ManagedCliNpmResolver; -use self::byonm::ByonmCliNpmResolver; - pub enum CliNpmResolverCreateOptions { Managed(CliNpmResolverManagedCreateOptions), Byonm(CliNpmResolverByonmCreateOptions), diff --git a/cli/resolver.rs b/cli/resolver.rs index c4037a75a4..44de3aa3f8 100644 --- a/cli/resolver.rs +++ b/cli/resolver.rs @@ -15,9 +15,11 @@ use deno_graph::source::UnknownBuiltInNodeModuleError; use deno_graph::source::DEFAULT_JSX_IMPORT_SOURCE_MODULE; use deno_runtime::deno_fs::FileSystem; use deno_runtime::deno_node::is_builtin_node_module; +use deno_runtime::deno_node::parse_npm_pkg_name; use deno_runtime::deno_node::NodeResolution; use deno_runtime::deno_node::NodeResolutionMode; use deno_runtime::deno_node::NodeResolver; +use deno_runtime::deno_node::NpmResolver as DenoNodeNpmResolver; use deno_runtime::permissions::PermissionsContainer; use deno_semver::npm::NpmPackageReqReference; use deno_semver::package::PackageReq; @@ -29,6 +31,7 @@ use crate::args::package_json::PackageJsonDeps; use crate::args::JsxImportSourceConfig; use crate::args::PackageJsonDepsProvider; use crate::module_loader::CjsResolutionStore; +use crate::npm::ByonmCliNpmResolver; use crate::npm::CliNpmResolver; use crate::npm::InnerCliNpmResolverRef; use crate::util::sync::AtomicFlag; @@ -177,6 +180,41 @@ impl CliGraphResolver { pub fn found_package_json_dep(&self) -> bool { self.found_package_json_dep_flag.is_raised() } + + fn check_surface_byonm_node_error( + &self, + specifier: &str, + referrer: &ModuleSpecifier, + mode: NodeResolutionMode, + original_err: AnyError, + resolver: &ByonmCliNpmResolver, + ) -> Result<(), AnyError> { + if let Ok((pkg_name, _, _)) = parse_npm_pkg_name(specifier, referrer) { + match resolver + .resolve_package_folder_from_package(&pkg_name, referrer, mode) + { + Ok(_) => { + return Err(original_err); + } + Err(_) => { + if resolver + .find_ancestor_package_json_with_dep(&pkg_name, referrer) + .is_some() + { + return Err(anyhow!( + concat!( + "Could not resolve \"{}\", but found it in a package.json. ", + "Deno expects the node_modules/ directory to be up to date. ", + "Did you forget to run `npm install`?" + ), + specifier + )); + } + } + } + } + Ok(()) + } } impl Resolver for CliGraphResolver { @@ -248,7 +286,7 @@ impl Resolver for CliGraphResolver { let package_json_path = package_folder.join("package.json"); if !self.fs.exists_sync(&package_json_path) { return Err(ResolveError::Other(anyhow!( - "Could not find '{}'. Maybe run `npm install`?", + "Could not find '{}'. Deno expects the node_modules/ directory to be up to date. Did you forget to run `npm install`?", package_json_path.display() ))); } @@ -290,14 +328,38 @@ impl Resolver for CliGraphResolver { to_node_mode(mode), &PermissionsContainer::allow_all(), ); - if let Ok(Some(resolution)) = node_result { - if let Some(cjs_resolutions) = &self.cjs_resolutions { - if let NodeResolution::CommonJs(specifier) = &resolution { - // remember that this was a common js resolution - cjs_resolutions.insert(specifier.clone()); + match node_result { + Ok(Some(resolution)) => { + if let Some(cjs_resolutions) = &self.cjs_resolutions { + if let NodeResolution::CommonJs(specifier) = &resolution { + // remember that this was a common js resolution + cjs_resolutions.insert(specifier.clone()); + } } + return Ok(resolution.into_url()); + } + Ok(None) => { + self + .check_surface_byonm_node_error( + specifier, + referrer, + to_node_mode(mode), + anyhow!("Cannot find \"{}\"", specifier), + resolver, + ) + .map_err(ResolveError::Other)?; + } + Err(err) => { + self + .check_surface_byonm_node_error( + specifier, + referrer, + to_node_mode(mode), + err, + resolver, + ) + .map_err(ResolveError::Other)?; } - return Ok(resolution.into_url()); } } } diff --git a/cli/tests/integration/npm_tests.rs b/cli/tests/integration/npm_tests.rs index c3fe2949c2..5104036e3f 100644 --- a/cli/tests/integration/npm_tests.rs +++ b/cli/tests/integration/npm_tests.rs @@ -2313,6 +2313,55 @@ console.log(getKind()); output.assert_exit_code(1); } +#[test] +pub fn byonm_package_specifier_not_installed_and_invalid_subpath() { + let test_context = TestContextBuilder::for_npm() + .env("DENO_UNSTABLE_BYONM", "1") + .use_temp_cwd() + .build(); + let dir = test_context.temp_dir(); + dir.path().join("package.json").write_json(&json!({ + "dependencies": { + "chalk": "4", + "@denotest/conditional-exports-strict": "1" + } + })); + dir.write( + "main.ts", + "import chalk from 'chalk'; console.log(chalk.green('hi'));", + ); + + // no npm install has been run, so this should give an informative error + let output = test_context.new_command().args("run main.ts").run(); + output.assert_matches_text( + r#"error: Could not resolve "chalk", but found it in a package.json. Deno expects the node_modules/ directory to be up to date. Did you forget to run `npm install`? + at file:///[WILDCARD]/main.ts:1:19 +"#, + ); + output.assert_exit_code(1); + + // now test for an invalid sub path after doing an npm install + dir.write( + "main.ts", + "import '@denotest/conditional-exports-strict/test';", + ); + + test_context + .new_command() + .name("npm") + .args("install") + .run() + .skip_output_check(); + + let output = test_context.new_command().args("run main.ts").run(); + output.assert_matches_text( + r#"error: [ERR_PACKAGE_PATH_NOT_EXPORTED] Package subpath './test' is not defined by "exports" in '[WILDCARD]' imported from '[WILDCARD]main.ts' + at file:///[WILDCARD]/main.ts:1:8 +"#, + ); + output.assert_exit_code(1); +} + #[test] pub fn byonm_package_npm_specifier_not_installed_and_invalid_subpath() { let test_context = TestContextBuilder::for_npm() @@ -2334,7 +2383,7 @@ pub fn byonm_package_npm_specifier_not_installed_and_invalid_subpath() { // no npm install has been run, so this should give an informative error let output = test_context.new_command().args("run main.ts").run(); output.assert_matches_text( - r#"error: Could not find '[WILDCARD]package.json'. Maybe run `npm install`? + r#"error: Could not find '[WILDCARD]package.json'. Deno expects the node_modules/ directory to be up to date. Did you forget to run `npm install`? at file:///[WILDCARD]/main.ts:1:19 "#, ); diff --git a/ext/node/lib.rs b/ext/node/lib.rs index a54d5a0102..6d7e85ec46 100644 --- a/ext/node/lib.rs +++ b/ext/node/lib.rs @@ -32,6 +32,7 @@ pub use path::PathClean; pub use polyfill::is_builtin_node_module; pub use polyfill::SUPPORTED_BUILTIN_NODE_MODULES; pub use polyfill::SUPPORTED_BUILTIN_NODE_MODULES_WITH_PREFIX; +pub use resolution::parse_npm_pkg_name; pub use resolution::NodeModuleKind; pub use resolution::NodeResolution; pub use resolution::NodeResolutionMode; diff --git a/ext/node/resolution.rs b/ext/node/resolution.rs index cbe9f643f9..cb4fef4052 100644 --- a/ext/node/resolution.rs +++ b/ext/node/resolution.rs @@ -448,7 +448,7 @@ impl NodeResolver { } /// Checks if the resolved file has a corresponding declaration file. - pub(super) fn path_to_declaration_path( + fn path_to_declaration_path( &self, path: PathBuf, referrer_kind: NodeModuleKind, @@ -975,7 +975,7 @@ impl NodeResolver { permissions: &dyn NodePermissions, ) -> Result, AnyError> { let (package_name, package_subpath, _is_scoped) = - parse_package_name(specifier, referrer)?; + parse_npm_pkg_name(specifier, referrer)?; // ResolveSelf let Some(package_config) = @@ -1485,7 +1485,7 @@ fn throw_exports_not_found( ) } -fn parse_package_name( +pub fn parse_npm_pkg_name( specifier: &str, referrer: &ModuleSpecifier, ) -> Result<(String, String, bool), AnyError> { @@ -1727,15 +1727,15 @@ mod tests { let dummy_referrer = Url::parse("http://example.com").unwrap(); assert_eq!( - parse_package_name("fetch-blob", &dummy_referrer).unwrap(), + parse_npm_pkg_name("fetch-blob", &dummy_referrer).unwrap(), ("fetch-blob".to_string(), ".".to_string(), false) ); assert_eq!( - parse_package_name("@vue/plugin-vue", &dummy_referrer).unwrap(), + parse_npm_pkg_name("@vue/plugin-vue", &dummy_referrer).unwrap(), ("@vue/plugin-vue".to_string(), ".".to_string(), true) ); assert_eq!( - parse_package_name("@astrojs/prism/dist/highlighter", &dummy_referrer) + parse_npm_pkg_name("@astrojs/prism/dist/highlighter", &dummy_referrer) .unwrap(), ( "@astrojs/prism".to_string(),