feat(add): Add npm packages to package.json if present (#25477)

Closes https://github.com/denoland/deno/issues/25321

Ended up being a larger refactoring, since we're now juggling
(potentially) two config files in the same `add`, instead of choosing
one. I don't love the shape of the code, but I think it's good enough

Some smaller side improvements:
- `deno remove` supports `jsonc`
- `deno install --dev` will be a really simple change
- if `deno remove` removes the last import/dependency in the
`imports`/`dependencies`/`devDependencies` field, it removes the field
instead of leaving an empty object

Cargo.lock

@@ -1253,6 +1253,7 @@ dependencies = [
  "which 4.4.2",
  "winapi",
  "winres",
+ "yoke",
  "zeromq",
  "zip",
  "zstd",

Cargo.toml

@@ -193,6 +193,7 @@ url = { version = "< 2.5.0", features = ["serde", "expose_internals"] }
 uuid = { version = "1.3.0", features = ["v4"] }
 webpki-roots = "0.26"
 which = "4.2.5"
+yoke = { version = "0.7.4", features = ["derive"] }
 zeromq = { version = "=0.4.0", default-features = false, features = ["tcp-transport", "tokio-runtime"] }
 zstd = "=0.12.4"
 

cli/Cargo.toml

@@ -161,6 +161,7 @@ typed-arena = "=2.0.2"
 uuid = { workspace = true, features = ["serde"] }
 walkdir = "=2.3.2"
 which.workspace = true
+yoke.workspace = true
 zeromq.workspace = true
 zip = { version = "2.1.6", default-features = false, features = ["deflate-flate2"] }
 zstd.workspace = true

cli/tools/registry/pm.rs

@@ -7,7 +7,6 @@ use deno_semver::jsr::JsrPackageReqReference;
 use deno_semver::npm::NpmPackageReqReference;
 
 use std::borrow::Cow;
-use std::path::Path;
 use std::path::PathBuf;
 use std::sync::Arc;
 
@@ -26,9 +25,11 @@ use deno_semver::package::PackageReq;
 use indexmap::IndexMap;
 use jsonc_parser::ast::ObjectProp;
 use jsonc_parser::ast::Value;
+use yoke::Yoke;
 
 use crate::args::AddFlags;
 use crate::args::CacheSetting;
+use crate::args::CliOptions;
 use crate::args::Flags;
 use crate::args::RemoveFlags;
 use crate::factory::CliFactory;
@@ -56,115 +57,303 @@ impl DenoConfigFormat {
   }
 }
 
+struct DenoConfig {
+  config: Arc<deno_config::deno_json::ConfigFile>,
+  format: DenoConfigFormat,
+  imports: IndexMap<String, String>,
+}
+
+fn deno_json_imports(
+  config: &deno_config::deno_json::ConfigFile,
+) -> Result<IndexMap<String, String>, AnyError> {
+  Ok(
+    config
+      .json
+      .imports
+      .clone()
+      .map(|imports| {
+        serde_json::from_value(imports)
+          .map_err(|err| anyhow!("Malformed \"imports\" configuration: {err}"))
+      })
+      .transpose()?
+      .unwrap_or_default(),
+  )
+}
+impl DenoConfig {
+  fn from_options(options: &CliOptions) -> Result<Option<Self>, AnyError> {
+    let start_dir = &options.start_dir;
+    if let Some(config) = start_dir.maybe_deno_json() {
+      Ok(Some(Self {
+        imports: deno_json_imports(config)?,
+        config: config.clone(),
+        format: DenoConfigFormat::from_specifier(&config.specifier)?,
+      }))
+    } else {
+      Ok(None)
+    }
+  }
+
+  fn add(&mut self, selected: SelectedPackage) {
+    self.imports.insert(
+      selected.import_name,
+      format!("{}@{}", selected.package_name, selected.version_req),
+    );
+  }
+
+  fn remove(&mut self, package: &str) -> bool {
+    self.imports.shift_remove(package).is_some()
+  }
+
+  fn take_import_fields(
+    &mut self,
+  ) -> Vec<(&'static str, IndexMap<String, String>)> {
+    vec![("imports", std::mem::take(&mut self.imports))]
+  }
+}
+
+impl NpmConfig {
+  fn from_options(options: &CliOptions) -> Result<Option<Self>, AnyError> {
+    let start_dir = &options.start_dir;
+    if let Some(pkg_json) = start_dir.maybe_pkg_json() {
+      Ok(Some(Self {
+        dependencies: pkg_json.dependencies.clone().unwrap_or_default(),
+        dev_dependencies: pkg_json.dev_dependencies.clone().unwrap_or_default(),
+        config: pkg_json.clone(),
+        fmt_options: None,
+      }))
+    } else {
+      Ok(None)
+    }
+  }
+
+  fn add(&mut self, selected: SelectedPackage, dev: bool) {
+    let (name, version) = package_json_dependency_entry(selected);
+    if dev {
+      self.dev_dependencies.insert(name, version);
+    } else {
+      self.dependencies.insert(name, version);
+    }
+  }
+
+  fn remove(&mut self, package: &str) -> bool {
+    let in_deps = self.dependencies.shift_remove(package).is_some();
+    let in_dev_deps = self.dev_dependencies.shift_remove(package).is_some();
+    in_deps || in_dev_deps
+  }
+
+  fn take_import_fields(
+    &mut self,
+  ) -> Vec<(&'static str, IndexMap<String, String>)> {
+    vec![
+      ("dependencies", std::mem::take(&mut self.dependencies)),
+      (
+        "devDependencies",
+        std::mem::take(&mut self.dev_dependencies),
+      ),
+    ]
+  }
+}
+
+struct NpmConfig {
+  config: Arc<deno_node::PackageJson>,
+  fmt_options: Option<FmtOptionsConfig>,
+  dependencies: IndexMap<String, String>,
+  dev_dependencies: IndexMap<String, String>,
+}
+
 enum DenoOrPackageJson {
-  Deno(Arc<deno_config::deno_json::ConfigFile>, DenoConfigFormat),
-  Npm(Arc<deno_node::PackageJson>, Option<FmtOptionsConfig>),
+  Deno(DenoConfig),
+  Npm(NpmConfig),
+}
+
+impl From<DenoConfig> for DenoOrPackageJson {
+  fn from(config: DenoConfig) -> Self {
+    Self::Deno(config)
+  }
+}
+
+impl From<NpmConfig> for DenoOrPackageJson {
+  fn from(config: NpmConfig) -> Self {
+    Self::Npm(config)
+  }
+}
+
+/// Wrapper around `jsonc_parser::ast::Object` that can be stored in a `Yoke`
+#[derive(yoke::Yokeable)]
+struct JsoncObjectView<'a>(jsonc_parser::ast::Object<'a>);
+
+struct ConfigUpdater {
+  config: DenoOrPackageJson,
+  // the `Yoke` is so we can carry the parsed object (which borrows from
+  // the source) along with the source itself
+  ast: Yoke<JsoncObjectView<'static>, String>,
+  path: PathBuf,
+  modified: bool,
+}
+
+impl ConfigUpdater {
+  fn obj(&self) -> &jsonc_parser::ast::Object<'_> {
+    &self.ast.get().0
+  }
+  fn contents(&self) -> &str {
+    self.ast.backing_cart()
+  }
+  async fn maybe_new(
+    config: Option<impl Into<DenoOrPackageJson>>,
+  ) -> Result<Option<Self>, AnyError> {
+    if let Some(config) = config {
+      Ok(Some(Self::new(config.into()).await?))
+    } else {
+      Ok(None)
+    }
+  }
+  async fn new(config: DenoOrPackageJson) -> Result<Self, AnyError> {
+    let specifier = config.specifier();
+    if specifier.scheme() != "file" {
+      bail!("Can't update a remote configuration file");
+    }
+    let config_file_path = specifier.to_file_path().map_err(|_| {
+      anyhow!("Specifier {specifier:?} is an invalid file path")
+    })?;
+    let config_file_contents = {
+      let contents = tokio::fs::read_to_string(&config_file_path)
+        .await
+        .with_context(|| {
+          format!("Reading config file at: {}", config_file_path.display())
+        })?;
+      if contents.trim().is_empty() {
+        "{}\n".into()
+      } else {
+        contents
+      }
+    };
+    let ast = Yoke::try_attach_to_cart(config_file_contents, |contents| {
+      let ast = jsonc_parser::parse_to_ast(
+        contents,
+        &Default::default(),
+        &Default::default(),
+      )
+      .with_context(|| {
+        format!("Failed to parse config file at {}", specifier)
+      })?;
+      let obj = match ast.value {
+        Some(Value::Object(obj)) => obj,
+        _ => bail!(
+          "Failed to update config file at {}, expected an object",
+          specifier
+        ),
+      };
+      Ok(JsoncObjectView(obj))
+    })?;
+    Ok(Self {
+      config,
+      ast,
+      path: config_file_path,
+      modified: false,
+    })
+  }
+
+  fn add(&mut self, selected: SelectedPackage, dev: bool) {
+    match &mut self.config {
+      DenoOrPackageJson::Deno(deno) => deno.add(selected),
+      DenoOrPackageJson::Npm(npm) => npm.add(selected, dev),
+    }
+    self.modified = true;
+  }
+
+  fn remove(&mut self, package: &str) -> bool {
+    let removed = match &mut self.config {
+      DenoOrPackageJson::Deno(deno) => deno.remove(package),
+      DenoOrPackageJson::Npm(npm) => npm.remove(package),
+    };
+    if removed {
+      self.modified = true;
+    }
+    removed
+  }
+
+  async fn commit(mut self) -> Result<(), AnyError> {
+    if !self.modified {
+      return Ok(());
+    }
+
+    let import_fields = self.config.take_import_fields();
+
+    let fmt_config_options = self.config.fmt_options();
+
+    let new_text = update_config_file_content(
+      self.obj(),
+      self.contents(),
+      fmt_config_options,
+      import_fields.into_iter().map(|(k, v)| {
+        (
+          k,
+          if v.is_empty() {
+            None
+          } else {
+            Some(generate_imports(v.into_iter().collect()))
+          },
+        )
+      }),
+      self.config.file_name(),
+    );
+
+    tokio::fs::write(&self.path, new_text).await?;
+    Ok(())
+  }
 }
 
 impl DenoOrPackageJson {
   fn specifier(&self) -> Cow<ModuleSpecifier> {
     match self {
-      Self::Deno(d, ..) => Cow::Borrowed(&d.specifier),
-      Self::Npm(n, ..) => Cow::Owned(n.specifier()),
-    }
-  }
-
-  /// Returns the existing imports/dependencies from the config.
-  fn existing_imports(&self) -> Result<IndexMap<String, String>, AnyError> {
-    match self {
-      DenoOrPackageJson::Deno(deno, ..) => {
-        if let Some(imports) = deno.json.imports.clone() {
-          match serde_json::from_value(imports) {
-            Ok(map) => Ok(map),
-            Err(err) => {
-              bail!("Malformed \"imports\" configuration: {err}")
-            }
-          }
-        } else {
-          Ok(Default::default())
-        }
-      }
-      DenoOrPackageJson::Npm(npm, ..) => {
-        Ok(npm.dependencies.clone().unwrap_or_default())
-      }
+      Self::Deno(d, ..) => Cow::Borrowed(&d.config.specifier),
+      Self::Npm(n, ..) => Cow::Owned(n.config.specifier()),
     }
   }
 
   fn fmt_options(&self) -> FmtOptionsConfig {
     match self {
       DenoOrPackageJson::Deno(deno, ..) => deno
+        .config
         .to_fmt_config()
         .ok()
         .map(|f| f.options)
         .unwrap_or_default(),
-      DenoOrPackageJson::Npm(_, config) => config.clone().unwrap_or_default(),
+      DenoOrPackageJson::Npm(config) => {
+        config.fmt_options.clone().unwrap_or_default()
+      }
     }
   }
 
-  fn imports_key(&self) -> &'static str {
+  fn take_import_fields(
+    &mut self,
+  ) -> Vec<(&'static str, IndexMap<String, String>)> {
     match self {
-      DenoOrPackageJson::Deno(..) => "imports",
-      DenoOrPackageJson::Npm(..) => "dependencies",
+      Self::Deno(d) => d.take_import_fields(),
+      Self::Npm(n) => n.take_import_fields(),
     }
   }
 
   fn file_name(&self) -> &'static str {
     match self {
-      DenoOrPackageJson::Deno(_, format) => match format {
+      DenoOrPackageJson::Deno(config) => match config.format {
         DenoConfigFormat::Json => "deno.json",
         DenoConfigFormat::Jsonc => "deno.jsonc",
       },
       DenoOrPackageJson::Npm(..) => "package.json",
     }
   }
-
-  fn is_npm(&self) -> bool {
-    matches!(self, Self::Npm(..))
 }
 
-  /// Get the preferred config file to operate on
-  /// given the flags. If no config file is present,
-  /// creates a `deno.json` file - in this case
-  /// we also return a new `CliFactory` that knows about
-  /// the new config
-  fn from_flags(flags: Arc<Flags>) -> Result<(Self, CliFactory), AnyError> {
-    let factory = CliFactory::from_flags(flags.clone());
-    let options = factory.cli_options()?;
-    let start_dir = &options.start_dir;
-
-    match (start_dir.maybe_deno_json(), start_dir.maybe_pkg_json()) {
-      // when both are present, for now,
-      // default to deno.json
-      (Some(deno), Some(_) | None) => Ok((
-        DenoOrPackageJson::Deno(
-          deno.clone(),
-          DenoConfigFormat::from_specifier(&deno.specifier)?,
-        ),
-        factory,
-      )),
-      (None, Some(package_json)) => {
-        Ok((DenoOrPackageJson::Npm(package_json.clone(), None), factory))
-      }
-      (None, None) => {
+fn create_deno_json(
+  flags: &Arc<Flags>,
+  options: &CliOptions,
+) -> Result<CliFactory, AnyError> {
   std::fs::write(options.initial_cwd().join("deno.json"), "{}\n")
     .context("Failed to create deno.json file")?;
-        drop(factory); // drop to prevent use
   log::info!("Created deno.json configuration file.");
   let factory = CliFactory::from_flags(flags.clone());
-        let options = factory.cli_options()?.clone();
-        let start_dir = &options.start_dir;
-        Ok((
-          DenoOrPackageJson::Deno(
-            start_dir.maybe_deno_json().cloned().ok_or_else(|| {
-              anyhow!("config not found, but it was just created")
-            })?,
-            DenoConfigFormat::Json,
-          ),
-          factory,
-        ))
-      }
-    }
-  }
+  Ok(factory)
 }
 
 fn package_json_dependency_entry(
@@ -199,19 +388,53 @@ impl std::fmt::Display for AddCommandName {
   }
 }
 
+fn load_configs(
+  flags: &Arc<Flags>,
+) -> Result<(CliFactory, Option<NpmConfig>, Option<DenoConfig>), AnyError> {
+  let cli_factory = CliFactory::from_flags(flags.clone());
+  let options = cli_factory.cli_options()?;
+  let npm_config = NpmConfig::from_options(options)?;
+  let (cli_factory, deno_config) = match DenoConfig::from_options(options)? {
+    Some(config) => (cli_factory, Some(config)),
+    None if npm_config.is_some() => (cli_factory, None),
+    None => {
+      let factory = create_deno_json(flags, options)?;
+      let options = factory.cli_options()?.clone();
+      (
+        factory,
+        Some(
+          DenoConfig::from_options(&options)?.expect("Just created deno.json"),
+        ),
+      )
+    }
+  };
+  assert!(deno_config.is_some() || npm_config.is_some());
+  Ok((cli_factory, npm_config, deno_config))
+}
+
 pub async fn add(
   flags: Arc<Flags>,
   add_flags: AddFlags,
   cmd_name: AddCommandName,
 ) -> Result<(), AnyError> {
-  let (config_file, cli_factory) =
-    DenoOrPackageJson::from_flags(flags.clone())?;
+  let (cli_factory, npm_config, deno_config) = load_configs(&flags)?;
+  let mut npm_config = ConfigUpdater::maybe_new(npm_config).await?;
+  let mut deno_config = ConfigUpdater::maybe_new(deno_config).await?;
 
-  let config_specifier = config_file.specifier();
-  if config_specifier.scheme() != "file" {
-    bail!("Can't add dependencies to a remote configuration file");
+  if let Some(deno) = &deno_config {
+    let specifier = deno.config.specifier();
+    if deno.obj().get_string("importMap").is_some() {
+      bail!(
+        concat!(
+          "`deno {}` is not supported when configuration file contains an \"importMap\" field. ",
+          "Inline the import map into the Deno configuration file.\n",
+          "    at {}",
+        ),
+        cmd_name,
+        specifier
+      );
+    }
   }
-  let config_file_path = config_specifier.to_file_path().unwrap();
 
   let http_client = cli_factory.http_client_provider();
 
@@ -279,39 +502,6 @@ pub async fn add(
     }
   }
 
-  let config_file_contents = {
-    let contents = tokio::fs::read_to_string(&config_file_path).await.unwrap();
-    if contents.trim().is_empty() {
-      "{}\n".into()
-    } else {
-      contents
-    }
-  };
-  let ast = jsonc_parser::parse_to_ast(
-    &config_file_contents,
-    &Default::default(),
-    &Default::default(),
-  )?;
-
-  let obj = match ast.value {
-    Some(Value::Object(obj)) => obj,
-    _ => bail!("Failed updating config file due to no object."),
-  };
-
-  if obj.get_string("importMap").is_some() {
-    bail!(
-      concat!(
-        "`deno add` is not supported when configuration file contains an \"importMap\" field. ",
-        "Inline the import map into the Deno configuration file.\n",
-        "    at {}",
-      ),
-      config_specifier
-    );
-  }
-
-  let mut existing_imports = config_file.existing_imports()?;
-
-  let is_npm = config_file.is_npm();
   for selected_package in selected_packages {
     log::info!(
       "Add {}{}{}",
@@ -320,39 +510,32 @@ pub async fn add(
       selected_package.selected_version
     );
 
-    if is_npm {
-      let (name, version) = package_json_dependency_entry(selected_package);
-      existing_imports.insert(name, version)
+    if selected_package.package_name.starts_with("npm:") {
+      if let Some(npm) = &mut npm_config {
+        npm.add(selected_package, false);
       } else {
-      existing_imports.insert(
-        selected_package.import_name,
-        format!(
-          "{}@{}",
-          selected_package.package_name, selected_package.version_req
-        ),
-      )
-    };
+        deno_config.as_mut().unwrap().add(selected_package, false);
+      }
+    } else if let Some(deno) = &mut deno_config {
+      deno.add(selected_package, false);
+    } else {
+      npm_config.as_mut().unwrap().add(selected_package, false);
+    }
   }
-  let mut import_list: Vec<(String, String)> =
-    existing_imports.into_iter().collect();
 
-  import_list.sort_by(|(k1, _), (k2, _)| k1.cmp(k2));
-  let generated_imports = generate_imports(import_list);
+  let mut commit_futures = vec![];
+  if let Some(npm) = npm_config {
+    commit_futures.push(npm.commit());
+  }
+  if let Some(deno) = deno_config {
+    commit_futures.push(deno.commit());
+  }
+  let commit_futures =
+    deno_core::futures::future::join_all(commit_futures).await;
 
-  let fmt_config_options = config_file.fmt_options();
-
-  let new_text = update_config_file_content(
-    obj,
-    &config_file_contents,
-    generated_imports,
-    fmt_config_options,
-    config_file.imports_key(),
-    config_file.file_name(),
-  );
-
-  tokio::fs::write(&config_file_path, new_text)
-    .await
-    .context("Failed to update configuration file")?;
+  for result in commit_futures {
+    result.context("Failed to update configuration file")?;
+  }
 
   // clear the previously cached package.json from memory before reloading it
   node_resolver::PackageJsonThreadLocalCache::clear();
@@ -524,7 +707,8 @@ impl AddPackageReq {
   }
 }
 
-fn generate_imports(packages_to_version: Vec<(String, String)>) -> String {
+fn generate_imports(mut packages_to_version: Vec<(String, String)>) -> String {
+  packages_to_version.sort_by(|(k1, _), (k2, _)| k1.cmp(k2));
   let mut contents = vec![];
   let len = packages_to_version.len();
   for (index, (package, version)) in packages_to_version.iter().enumerate() {
@@ -537,68 +721,27 @@ fn generate_imports(packages_to_version: Vec<(String, String)>) -> String {
   contents.join("\n")
 }
 
-fn remove_from_config(
-  config_path: &Path,
-  keys: &[&'static str],
-  packages_to_remove: &[String],
-  removed_packages: &mut Vec<String>,
-  fmt_options: &FmtOptionsConfig,
-) -> Result<(), AnyError> {
-  let mut json: serde_json::Value =
-    serde_json::from_slice(&std::fs::read(config_path)?)?;
-  for key in keys {
-    let Some(obj) = json.get_mut(*key).and_then(|v| v.as_object_mut()) else {
-      continue;
-    };
-    for package in packages_to_remove {
-      if obj.shift_remove(package).is_some() {
-        removed_packages.push(package.clone());
-      }
-    }
-  }
-
-  let config = serde_json::to_string_pretty(&json)?;
-  let config =
-    crate::tools::fmt::format_json(config_path, &config, fmt_options)
-      .ok()
-      .flatten()
-      .unwrap_or(config);
-
-  std::fs::write(config_path, config)
-    .context("Failed to update configuration file")?;
-
-  Ok(())
-}
-
 pub async fn remove(
   flags: Arc<Flags>,
   remove_flags: RemoveFlags,
 ) -> Result<(), AnyError> {
-  let (config_file, factory) = DenoOrPackageJson::from_flags(flags.clone())?;
-  let options = factory.cli_options()?;
-  let start_dir = &options.start_dir;
-  let fmt_config_options = config_file.fmt_options();
+  let (_, npm_config, deno_config) = load_configs(&flags)?;
 
-  let mut removed_packages = Vec::new();
+  let mut configs = [
+    ConfigUpdater::maybe_new(npm_config).await?,
+    ConfigUpdater::maybe_new(deno_config).await?,
+  ];
 
-  if let Some(deno_json) = start_dir.maybe_deno_json() {
-    remove_from_config(
-      &deno_json.specifier.to_file_path().unwrap(),
-      &["imports"],
-      &remove_flags.packages,
-      &mut removed_packages,
-      &fmt_config_options,
-    )?;
+  let mut removed_packages = vec![];
+
+  for package in &remove_flags.packages {
+    let mut removed = false;
+    for config in configs.iter_mut().flatten() {
+      removed |= config.remove(package);
+    }
+    if removed {
+      removed_packages.push(package.clone());
     }
-
-  if let Some(pkg_json) = start_dir.maybe_pkg_json() {
-    remove_from_config(
-      &pkg_json.path,
-      &["dependencies", "devDependencies"],
-      &remove_flags.packages,
-      &mut removed_packages,
-      &fmt_config_options,
-    )?;
   }
 
   if removed_packages.is_empty() {
@@ -607,6 +750,10 @@ pub async fn remove(
     for package in &removed_packages {
       log::info!("Removed {}", crate::colors::green(package));
     }
+    for config in configs.into_iter().flatten() {
+      config.commit().await?;
+    }
+
     // Update deno.lock
     node_resolver::PackageJsonThreadLocalCache::clear();
     let cli_factory = CliFactory::from_flags(flags);
@@ -616,25 +763,54 @@ pub async fn remove(
   Ok(())
 }
 
-fn update_config_file_content(
-  obj: jsonc_parser::ast::Object,
+fn update_config_file_content<
+  I: IntoIterator<Item = (&'static str, Option<String>)>,
+>(
+  obj: &jsonc_parser::ast::Object,
   config_file_contents: &str,
-  generated_imports: String,
   fmt_options: FmtOptionsConfig,
-  imports_key: &str,
+  entries: I,
   file_name: &str,
 ) -> String {
   let mut text_changes = vec![];
-
-  match obj.get(imports_key) {
-    Some(ObjectProp {
+  for (key, value) in entries {
+    match obj.properties.iter().enumerate().find_map(|(idx, k)| {
+      if k.name.as_str() == key {
+        Some((idx, k))
+      } else {
+        None
+      }
+    }) {
+      Some((
+        idx,
+        ObjectProp {
           value: Value::Object(lit),
+          range,
           ..
-    }) => text_changes.push(TextChange {
+        },
+      )) => {
+        if let Some(value) = value {
+          text_changes.push(TextChange {
             range: (lit.range.start + 1)..(lit.range.end - 1),
-      new_text: generated_imports,
+            new_text: value,
+          })
+        } else {
+          text_changes.push(TextChange {
+            // remove field entirely, making sure to
+            // remove the comma if it's not the last field
+            range: range.start..(if idx == obj.properties.len() - 1 {
+              range.end
+            } else {
+              obj.properties[idx + 1].range.start
             }),
+            new_text: "".to_string(),
+          })
+        }
+      }
+
+      // need to add field
       None => {
+        if let Some(value) = value {
           let insert_position = obj.range.end - 1;
           text_changes.push(TextChange {
             range: insert_position..insert_position,
@@ -646,12 +822,14 @@ fn update_config_file_content(
             //     "<package_name>": "<registry>:<package_name>@<semver>"
             //   }
             // }
-        new_text: format!("\"{imports_key}\": {{\n {generated_imports} }}"),
+            new_text: format!("\"{key}\": {{\n {value} }}"),
           })
         }
+      }
       // we verified the shape of `imports`/`dependencies` above
       Some(_) => unreachable!(),
     }
+  }
 
   let new_text =
     deno_ast::apply_text_changes(config_file_contents, text_changes);

ext/node/Cargo.toml

@@ -97,7 +97,7 @@ url.workspace = true
 winapi.workspace = true
 x25519-dalek = { version = "2.0.0", features = ["static_secrets"] }
 x509-parser = "0.15.0"
-yoke = { version = "0.7.4", features = ["derive"] }
+yoke.workspace = true
 
 [target.'cfg(windows)'.dependencies]
 windows-sys.workspace = true

tests/specs/add/package_json_and_deno_json/__test__.jsonc

@@ -0,0 +1,46 @@
+{
+  "tempDir": true,
+  "tests": {
+    "npm_prefers_package_json": {
+      "steps": [
+        {
+          "args": "add npm:@denotest/esm-basic @denotest/add npm:@denotest/say-hello",
+          "output": "add.out"
+        },
+        {
+          "args": [
+            "eval",
+            "console.log(Deno.readTextFileSync('package.json').trim())"
+          ],
+          "output": "npm_prefer_package.json.out"
+        },
+        {
+          "args": [
+            "eval",
+            "console.log(Deno.readTextFileSync('deno.json').trim())"
+          ],
+          "output": "npm_prefer_deno.json.out"
+        }
+      ]
+    },
+    "only_creates_deno_json_if_no_config": {
+      "steps": [
+        {
+          "args": ["eval", "Deno.removeSync('deno.json')"],
+          "output": ""
+        },
+        {
+          "args": "add npm:@denotest/esm-basic",
+          "output": "add_esm_basic.out"
+        },
+        {
+          "args": [
+            "eval",
+            "try { Deno.statSync('deno.json'); console.log('bad'); } catch (e) { if (e instanceof Deno.errors.NotFound) { console.log('good'); } else { console.log('bad error', e); }}"
+          ],
+          "output": "good\n"
+        }
+      ]
+    }
+  }
+}

tests/specs/add/package_json_and_deno_json/add.out

@@ -0,0 +1,12 @@
+[UNORDERED_START]
+Add npm:@denotest/esm-basic@1.0.0
+Add jsr:@denotest/add@1.0.0
+Add npm:@denotest/say-hello@1.0.0
+Download http://localhost:4260/@denotest/esm-basic
+Download http://localhost:4260/@denotest/esm-basic/1.0.0.tgz
+Download http://localhost:4260/@denotest/say-hello
+Download http://localhost:4260/@denotest/say-hello/1.0.0.tgz
+Initialize @denotest/esm-basic@1.0.0
+Initialize @denotest/say-hello@1.0.0
+Download http://127.0.0.1:4250/@denotest/add/1.0.0/mod.ts
+[UNORDERED_END]

tests/specs/add/package_json_and_deno_json/add_esm_basic.out

@@ -0,0 +1,4 @@
+Add npm:@denotest/esm-basic@1.0.0
+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

tests/specs/add/package_json_and_deno_json/deno.json

@@ -0,0 +1 @@
+{}

tests/specs/add/package_json_and_deno_json/npm_prefer_deno.json.out

@@ -0,0 +1,5 @@
+{
+  "imports": {
+    "@denotest/add": "jsr:@denotest/add@^1.0.0"
+  }
+}

tests/specs/add/package_json_and_deno_json/npm_prefer_package.json.out

@@ -0,0 +1,6 @@
+{
+  "dependencies": {
+    "@denotest/esm-basic": "^1.0.0",
+    "@denotest/say-hello": "^1.0.0"
+  }
+}

tests/specs/add/package_json_and_deno_json/package.json

@@ -0,0 +1 @@
+{}

tests/specs/remove/basic/__test__.jsonc

@@ -12,5 +12,8 @@
   }, {
     "args": ["eval", "console.log(Deno.readTextFileSync('deno.lock').trim())"],
     "output": "remove_lock.out"
+  }, {
+    "args": ["eval", "console.log(Deno.readTextFileSync('deno.json').trim())"],
+    "output": "{\n}\n"
   }]
 }

tests/specs/remove/package_json/__test__.jsonc

@@ -0,0 +1,27 @@
+{
+  "tempDir": true,
+  "steps": [
+    {
+      "args": "remove @denotest/add",
+      "output": "rm_add.out"
+    },
+    {
+      "args": [
+        "eval",
+        "console.log(Deno.readTextFileSync('package.json').trim())"
+      ],
+      "output": "rm_add_package.json.out"
+    },
+    {
+      "args": "remove @denotest/esm-basic",
+      "output": "rm_esm_basic.out"
+    },
+    {
+      "args": [
+        "eval",
+        "console.log(Deno.readTextFileSync('package.json').trim())"
+      ],
+      "output": "rm_esm_basic_package.json.out"
+    }
+  ]
+}

tests/specs/remove/package_json/package.json

@@ -0,0 +1,4 @@
+{
+  "dependencies": { "@denotest/add": "^1.0.0" },
+  "devDependencies": { "@denotest/esm-basic": "^1.0.0" }
+}

tests/specs/remove/package_json/rm_add.out

@@ -0,0 +1,4 @@
+Removed @denotest/add
+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

tests/specs/remove/package_json/rm_add_package.json.out

@@ -0,0 +1,3 @@
+{
+  "devDependencies": { "@denotest/esm-basic": "^1.0.0" }
+}

tests/specs/remove/package_json/rm_esm_basic.out

@@ -0,0 +1 @@
+Removed @denotest/esm-basic

tests/specs/remove/package_json/rm_esm_basic_package.json.out

@@ -0,0 +1,2 @@
+{
+}