fix(install): surface package.json dependency errors (#26023)

cli/args/flags.rs

@@ -2910,6 +2910,7 @@ List all available tasks:
           .help("Specify the directory to run the task in")
           .value_hint(ValueHint::DirPath),
       )
+      .arg(node_modules_dir_arg())
   })
 }
 
@@ -4974,6 +4975,7 @@ fn task_parse(flags: &mut Flags, matches: &mut ArgMatches) {
     .unwrap_or(ConfigFlag::Discover);
 
   unstable_args_parse(flags, matches, UnstableArgsConfig::ResolutionAndRuntime);
+  node_modules_arg_parse(flags, matches);
 
   let mut task_flags = TaskFlags {
     cwd: matches.remove_one::<String>("cwd"),

cli/args/package_json.rs

@@ -6,6 +6,7 @@ use std::sync::Arc;
 use deno_config::workspace::Workspace;
 use deno_core::serde_json;
 use deno_package_json::PackageJsonDepValue;
+use deno_package_json::PackageJsonDepValueParseError;
 use deno_semver::npm::NpmPackageReqReference;
 use deno_semver::package::PackageReq;
 
@@ -26,6 +27,7 @@ pub struct InstallNpmWorkspacePkg {
 pub struct NpmInstallDepsProvider {
   remote_pkgs: Vec<InstallNpmRemotePkg>,
   workspace_pkgs: Vec<InstallNpmWorkspacePkg>,
+  pkg_json_dep_errors: Vec<PackageJsonDepValueParseError>,
 }
 
 impl NpmInstallDepsProvider {
@@ -37,6 +39,7 @@ impl NpmInstallDepsProvider {
     // todo(dsherret): estimate capacity?
     let mut workspace_pkgs = Vec::new();
     let mut remote_pkgs = Vec::new();
+    let mut pkg_json_dep_errors = Vec::new();
     let workspace_npm_pkgs = workspace.npm_packages();
 
     for (_, folder) in workspace.config_folders() {
@@ -83,8 +86,12 @@ impl NpmInstallDepsProvider {
         let deps = pkg_json.resolve_local_package_json_deps();
         let mut pkg_pkgs = Vec::with_capacity(deps.len());
         for (alias, dep) in deps {
-          let Ok(dep) = dep else {
+          let dep = match dep {
-            continue;
+            Ok(dep) => dep,
+            Err(err) => {
+              pkg_json_dep_errors.push(err);
+              continue;
+            }
           };
           match dep {
             PackageJsonDepValue::Req(pkg_req) => {
@@ -131,14 +138,19 @@ impl NpmInstallDepsProvider {
     Self {
       remote_pkgs,
       workspace_pkgs,
+      pkg_json_dep_errors,
     }
   }
 
-  pub fn remote_pkgs(&self) -> &Vec<InstallNpmRemotePkg> {
+  pub fn remote_pkgs(&self) -> &[InstallNpmRemotePkg] {
     &self.remote_pkgs
   }
 
-  pub fn workspace_pkgs(&self) -> &Vec<InstallNpmWorkspacePkg> {
+  pub fn workspace_pkgs(&self) -> &[InstallNpmWorkspacePkg] {
     &self.workspace_pkgs
   }
+
+  pub fn pkg_json_dep_errors(&self) -> &[PackageJsonDepValueParseError] {
+    &self.pkg_json_dep_errors
+  }
 }

cli/npm/managed/mod.rs

@@ -20,6 +20,7 @@ use deno_npm::resolution::ValidSerializedNpmResolutionSnapshot;
 use deno_npm::NpmPackageId;
 use deno_npm::NpmResolutionPackage;
 use deno_npm::NpmSystemInfo;
+use deno_runtime::colors;
 use deno_runtime::deno_fs::FileSystem;
 use deno_runtime::deno_node::NodePermissions;
 use deno_runtime::deno_node::NodeRequireResolver;
@@ -478,6 +479,25 @@ impl ManagedCliNpmResolver {
     self.resolution.resolve_pkg_id_from_pkg_req(req)
   }
 
+  pub fn ensure_no_pkg_json_dep_errors(&self) -> Result<(), AnyError> {
+    for err in self.npm_install_deps_provider.pkg_json_dep_errors() {
+      match err {
+        deno_package_json::PackageJsonDepValueParseError::VersionReq(_) => {
+          return Err(
+            AnyError::from(err.clone())
+              .context("Failed to install from package.json"),
+          );
+        }
+        deno_package_json::PackageJsonDepValueParseError::Unsupported {
+          ..
+        } => {
+          log::warn!("{} {} in package.json", colors::yellow("Warning"), err)
+        }
+      }
+    }
+    Ok(())
+  }
+
   /// Ensures that the top level `package.json` dependencies are installed.
   /// This may set up the `node_modules` directory.
   ///
@@ -489,6 +509,7 @@ impl ManagedCliNpmResolver {
     if !self.top_level_install_flag.raise() {
       return Ok(false); // already did this
     }
+
     let pkg_json_remote_pkgs = self.npm_install_deps_provider.remote_pkgs();
     if pkg_json_remote_pkgs.is_empty() {
       return Ok(false);

cli/tools/installer.rs

@@ -298,6 +298,10 @@ async fn install_local(
     }
     InstallFlagsLocal::TopLevel => {
       let factory = CliFactory::from_flags(flags);
+      // surface any errors in the package.json
+      if let Some(npm_resolver) = factory.npm_resolver().await?.as_managed() {
+        npm_resolver.ensure_no_pkg_json_dep_errors()?;
+      }
       crate::tools::registry::cache_top_level_deps(&factory, None).await?;
 
       if let Some(lockfile) = factory.cli_options()?.maybe_lockfile() {

cli/tools/registry/pm.rs

@@ -558,12 +558,7 @@ pub async fn add(
     result.context("Failed to update configuration file")?;
   }
 
-  // clear the previously cached package.json from memory before reloading it
+  npm_install_after_modification(flags, Some(jsr_resolver)).await?;
-  node_resolver::PackageJsonThreadLocalCache::clear();
-  // make a new CliFactory to pick up the updated config file
-  let cli_factory = CliFactory::from_flags(flags);
-  // cache deps
-  cache_deps::cache_top_level_deps(&cli_factory, Some(jsr_resolver)).await?;
 
   Ok(())
 }
@@ -786,15 +781,33 @@ pub async fn remove(
       config.commit().await?;
     }
 
-    // Update deno.lock
+    npm_install_after_modification(flags, None).await?;
-    node_resolver::PackageJsonThreadLocalCache::clear();
-    let cli_factory = CliFactory::from_flags(flags);
-    cache_deps::cache_top_level_deps(&cli_factory, None).await?;
   }
 
   Ok(())
 }
 
+async fn npm_install_after_modification(
+  flags: Arc<Flags>,
+  // explicitly provided to prevent redownloading
+  jsr_resolver: Option<Arc<crate::jsr::JsrFetchResolver>>,
+) -> Result<(), AnyError> {
+  // clear the previously cached package.json from memory before reloading it
+  node_resolver::PackageJsonThreadLocalCache::clear();
+
+  // make a new CliFactory to pick up the updated config file
+  let cli_factory = CliFactory::from_flags(flags);
+  // surface any errors in the package.json
+  let npm_resolver = cli_factory.npm_resolver().await?;
+  if let Some(npm_resolver) = npm_resolver.as_managed() {
+    npm_resolver.ensure_no_pkg_json_dep_errors()?;
+  }
+  // npm install
+  cache_deps::cache_top_level_deps(&cli_factory, jsr_resolver).await?;
+
+  Ok(())
+}
+
 fn update_config_file_content<
   I: IntoIterator<Item = (&'static str, Option<String>)>,
 >(

cli/tools/registry/pm/cache_deps.rs

@@ -11,6 +11,7 @@ use deno_core::futures::StreamExt;
 use deno_semver::package::PackageReq;
 
 pub async fn cache_top_level_deps(
+  // todo(dsherret): don't pass the factory into this function. Instead use ctor deps
   factory: &CliFactory,
   jsr_resolver: Option<Arc<crate::jsr::JsrFetchResolver>>,
 ) -> Result<(), AnyError> {

tests/specs/run/package_json/invalid_value/__test__.jsonc

@@ -6,49 +6,25 @@
       "args": "run --quiet --node-modules-dir=auto ok.ts",
       "output": "ok.ts.out"
     },
-    "run_ok_byonm": {
-      "steps": [
-        {
-          "args": "install",
-          "output": "install.out"
-        },
-        {
-          "args": "run ok.ts",
-          "output": "ok.ts.out"
-        }
-      ]
-    },
     // should fail when referencing a failing dep entry
     "run_error_auto": {
       "args": "run --node-modules-dir=auto error.ts",
       "exitCode": 1,
       "output": "error_auto.out"
     },
-    "run_error_byonm": {
+    "install_error_byonm": {
-      "steps": [
+      "args": "install",
-        {
+      "output": "install.out",
-          "args": "install",
+      "exitCode": 1
-          "output": "install.out"
+    },
-        },
+    "add_error_byonm": {
-        {
+      "args": "add npm:cowsay",
-          "args": "run error.ts",
+      "output": "add.out",
-          "exitCode": 1,
+      "exitCode": 1
-          "output": "error.out"
-        }
-      ]
     },
-    // should output a warning about the failing dep entry
     "task_test": {
-      "steps": [
+      "args": "task --node-modules-dir=auto test",
-        {
+      "output": "task.out"
-          "args": "install",
-          "output": "install.out"
-        },
-        {
-          "args": "task test",
-          "output": "task.out"
-        }
-      ]
     }
   }
 }

tests/specs/run/package_json/invalid_value/add.out

@@ -0,0 +1,8 @@
+Add npm:cowsay@1.5.0
+error: Failed to install from package.json
+
+Caused by:
+    0: Invalid version requirement
+    1: Unexpected character.
+         invalid stuff that won't parse
+         ~

tests/specs/run/package_json/invalid_value/install.out

@@ -1,3 +1,7 @@
-Download http://localhost:4260/@denotest/esm-basic
+error: Failed to install from package.json
-Download http://localhost:4260/@denotest/esm-basic/1.0.0.tgz
+
-Initialize @denotest/esm-basic@1.0.0
+Caused by:
+    0: Invalid version requirement
+    1: Unexpected character.
+         invalid stuff that won't parse
+         ~

tests/specs/run/package_json/invalid_value/task.out

@@ -1,2 +1,5 @@
+Download http://localhost:4260/@denotest/esm-basic
+Download http://localhost:4260/@denotest/esm-basic/1.0.0.tgz
+Initialize @denotest/esm-basic@1.0.0
 Task test echo 1
 1