fix(unstable/byonm): improve error messages (#20987)

This improves the error messages when a specifier can't be resolved from
a deno module into an npm package.

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<PackageJson> {
+    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<PathBuf, AnyError> {
+      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;

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),

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,7 +328,8 @@ impl Resolver for CliGraphResolver {
                 to_node_mode(mode),
                 &PermissionsContainer::allow_all(),
               );
-              if let Ok(Some(resolution)) = node_result {
+              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
@@ -299,6 +338,29 @@ impl Resolver for CliGraphResolver {
                   }
                   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)?;
+                }
+              }
             }
           }
         }

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
 "#,
   );

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;

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<Option<PathBuf>, 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(),