From a1470a7e1f41c08ca059c51d8ac4eac6b564fb54 Mon Sep 17 00:00:00 2001 From: Nathan Whitaker Date: Wed, 4 Sep 2024 17:02:20 -0700 Subject: [PATCH 1/9] First go --- cli/tools/registry/pm.rs | 432 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 431 insertions(+), 1 deletion(-) diff --git a/cli/tools/registry/pm.rs b/cli/tools/registry/pm.rs index 4a36b459a807c5..c461af34e24a8c 100644 --- a/cli/tools/registry/pm.rs +++ b/cli/tools/registry/pm.rs @@ -5,8 +5,10 @@ mod cache_deps; pub use cache_deps::cache_top_level_deps; use std::borrow::Cow; +use std::cell::RefCell; use std::path::Path; use std::path::PathBuf; +use std::rc::Rc; use std::sync::Arc; use deno_ast::TextChange; @@ -27,6 +29,7 @@ use jsonc_parser::ast::Value; use crate::args::AddFlags; use crate::args::CacheSetting; +use crate::args::CliOptions; use crate::args::Flags; use crate::args::RemoveFlags; use crate::factory::CliFactory; @@ -54,11 +57,247 @@ impl DenoConfigFormat { } } +trait DepManifest { + fn specifier(&self) -> Cow; + fn existing_imports(&self) -> Result, AnyError>; + fn fmt_options(&self) -> FmtOptionsConfig; + fn imports_key(&self) -> &'static str; + fn file_name(&self) -> &'static str; + fn import_entry(&self, selected: SelectedPackage) -> (String, String); +} + +struct DenoConfig { + config: Arc, + format: DenoConfigFormat, +} + +impl DenoConfig { + fn from_options(options: &CliOptions) -> Result, AnyError> { + let start_dir = &options.start_dir; + if let Some(deno) = start_dir.maybe_deno_json() { + Ok(Some(Self { + config: deno.clone(), + format: DenoConfigFormat::from_specifier(&deno.specifier)?, + })) + } else { + Ok(None) + } + } +} + +impl NpmConfig { + fn from_options(options: &CliOptions) -> Result, AnyError> { + let start_dir = &options.start_dir; + if let Some(pkg_json) = start_dir.maybe_pkg_json() { + Ok(Some(Self { + config: pkg_json.clone(), + fmt_options: None, + })) + } else { + Ok(None) + } + } +} + +struct NpmConfig { + config: Arc, + fmt_options: Option, +} + enum DenoOrPackageJson { Deno(Arc, DenoConfigFormat), Npm(Arc, Option), } +impl DepManifest for Box { + fn specifier(&self) -> Cow { + (**self).specifier() + } + + fn existing_imports(&self) -> Result, AnyError> { + (**self).existing_imports() + } + + fn fmt_options(&self) -> FmtOptionsConfig { + (**self).fmt_options() + } + + fn imports_key(&self) -> &'static str { + (**self).imports_key() + } + + fn file_name(&self) -> &'static str { + (**self).file_name() + } + + fn import_entry(&self, selected: SelectedPackage) -> (String, String) { + (**self).import_entry(selected) + } +} + +impl DepManifest for DenoConfig { + fn specifier(&self) -> Cow { + Cow::Borrowed(&self.config.specifier) + } + + fn existing_imports(&self) -> Result, AnyError> { + if let Some(imports) = self.config.json.imports.clone() { + match serde_json::from_value(imports) { + Ok(map) => Ok(map), + Err(err) => { + bail!("Malformed \"imports\" configuration: {err}") + } + } + } else { + Ok(Default::default()) + } + } + + fn fmt_options(&self) -> FmtOptionsConfig { + self + .config + .to_fmt_config() + .ok() + .map(|f| f.options) + .unwrap_or_default() + } + + fn imports_key(&self) -> &'static str { + "imports" + } + + fn file_name(&self) -> &'static str { + match self.format { + DenoConfigFormat::Json => "deno.json", + DenoConfigFormat::Jsonc => "deno.jsonc", + } + } + + fn import_entry(&self, selected: SelectedPackage) -> (String, String) { + ( + selected.import_name, + format!("{}@{}", selected.package_name, selected.version_req), + ) + } +} + +impl DepManifest for NpmConfig { + fn specifier(&self) -> Cow { + Cow::Owned(self.config.specifier()) + } + + fn existing_imports(&self) -> Result, AnyError> { + Ok(self.config.dependencies.clone().unwrap_or_default()) + } + + fn fmt_options(&self) -> FmtOptionsConfig { + self.fmt_options.clone().unwrap_or_default() + } + + fn imports_key(&self) -> &'static str { + "dependencies" + } + + fn file_name(&self) -> &'static str { + "package.json" + } + + fn import_entry(&self, selected: SelectedPackage) -> (String, String) { + package_json_dependency_entry(selected) + } +} + +struct MutableManifest { + imports: IndexMap, + config: C, +} + +enum CommitError { + HasImportMap, + ParseError(jsonc_parser::errors::ParseError), + ConfigNotObject, + RemoteConfig, + WriteFileError(std::io::Error), +} + +impl MutableManifest { + fn into_dyn(self) -> MutableManifest> { + MutableManifest { + imports: self.imports, + config: Box::new(self.config), + } + } +} +impl MutableManifest { + fn new(config: C) -> Result { + let specifier = config.specifier(); + if specifier.scheme() != "file" { + bail!("Can't add dependencies to a remote configuration file"); + } + let imports = config.existing_imports()?; + Ok(Self { imports, config }) + } + + fn add(&mut self, selected: SelectedPackage) { + let (name, version) = self.config.import_entry(selected); + self.imports.insert(name, version); + } + + async fn commit(self) -> Result<(), CommitError> { + let config_specifier = self.config.specifier(); + if config_specifier.scheme() != "file" { + return Err(CommitError::RemoteConfig); + } + let config_file_path = config_specifier.to_file_path().unwrap(); + 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(), + ) + .map_err(CommitError::ParseError)?; + + let obj = match ast.value { + Some(Value::Object(obj)) => obj, + _ => return Err(CommitError::ConfigNotObject), + }; + + if obj.get_string("importMap").is_some() { + return Err(CommitError::HasImportMap); + } + + let mut import_list: Vec<(String, String)> = + self.imports.into_iter().collect(); + + import_list.sort_by(|(k1, _), (k2, _)| k1.cmp(k2)); + let generated_imports = generate_imports(import_list); + + let fmt_config_options = self.config.fmt_options(); + + let new_text = update_config_file_content( + obj, + &config_file_contents, + generated_imports, + fmt_config_options, + self.config.imports_key(), + self.config.file_name(), + ); + + tokio::fs::write(&config_file_path, new_text) + .await + .map_err(CommitError::WriteFileError)?; + Ok(()) + } +} + impl DenoOrPackageJson { fn specifier(&self) -> Cow { match self { @@ -165,6 +404,17 @@ impl DenoOrPackageJson { } } +fn create_deno_json( + flags: &Arc, + options: &CliOptions, +) -> Result { + std::fs::write(options.initial_cwd().join("deno.json"), "{}\n") + .context("Failed to create deno.json file")?; + log::info!("Created deno.json configuration file."); + let factory = CliFactory::from_flags(flags.clone()); + Ok(factory) +} + fn package_json_dependency_entry( selected: SelectedPackage, ) -> (String, String) { @@ -197,7 +447,7 @@ impl std::fmt::Display for AddCommandName { } } -pub async fn add( +pub async fn add_( flags: Arc, add_flags: AddFlags, cmd_name: AddCommandName, @@ -362,6 +612,186 @@ pub async fn add( Ok(()) } +pub async fn add( + flags: Arc, + add_flags: AddFlags, + cmd_name: AddCommandName, +) -> Result<(), 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()); + + let npm_config = npm_config + .map(|config| MutableManifest::new(config)) + .transpose()?; + let deno_config = deno_config + .map(|config| MutableManifest::new(config)) + .transpose()?; + + let http_client = cli_factory.http_client_provider(); + + let mut selected_packages = Vec::with_capacity(add_flags.packages.len()); + let mut package_reqs = Vec::with_capacity(add_flags.packages.len()); + + for entry_text in add_flags.packages.iter() { + let req = AddPackageReq::parse(entry_text).with_context(|| { + format!("Failed to parse package required: {}", entry_text) + })?; + + package_reqs.push(req); + } + + let deps_http_cache = cli_factory.global_http_cache()?; + let mut deps_file_fetcher = FileFetcher::new( + deps_http_cache.clone(), + CacheSetting::ReloadAll, + true, + http_client.clone(), + Default::default(), + None, + ); + deps_file_fetcher.set_download_log_level(log::Level::Trace); + let deps_file_fetcher = Arc::new(deps_file_fetcher); + let jsr_resolver = Arc::new(JsrFetchResolver::new(deps_file_fetcher.clone())); + let npm_resolver = Arc::new(NpmFetchResolver::new(deps_file_fetcher)); + + let package_futures = package_reqs + .into_iter() + .map({ + let jsr_resolver = jsr_resolver.clone(); + move |package_req| { + find_package_and_select_version_for_req( + jsr_resolver.clone(), + npm_resolver.clone(), + package_req, + ) + .boxed_local() + } + }) + .collect::>(); + + let stream_of_futures = deno_core::futures::stream::iter(package_futures); + let mut buffered = stream_of_futures.buffered(10); + + while let Some(package_and_version_result) = buffered.next().await { + let package_and_version = package_and_version_result?; + + match package_and_version { + PackageAndVersion::NotFound { + package: package_name, + found_npm_package, + package_req, + } => { + if found_npm_package { + log::error!("{} was not found, but a matching npm package exists. Did you mean `{}`?", crate::colors::red(package_name), crate::colors::yellow(format!("deno {cmd_name} npm:{package_req}"))); + } else { + log::error!("{} was not found.", crate::colors::red(package_name)); + } + } + PackageAndVersion::Selected(selected) => { + selected_packages.push(selected); + } + } + } + + let (config_for_npm_deps, config_for_other_deps) = + match (npm_config, deno_config) { + (Some(npm), Some(deno)) => ( + Rc::new(RefCell::new(Some(npm.into_dyn()))), + Rc::new(RefCell::new(Some(deno.into_dyn()))), + ), + (Some(npm), None) => { + let shared = Rc::new(RefCell::new(Some(npm.into_dyn()))); + (shared.clone(), shared) + } + (None, Some(deno)) => { + let shared = Rc::new(RefCell::new(Some(deno.into_dyn()))); + (shared.clone(), shared) + } + (None, None) => unreachable!(), + }; + + for selected_package in selected_packages { + log::info!( + "Add {}{}{}", + crate::colors::green(&selected_package.package_name), + crate::colors::gray("@"), + selected_package.selected_version + ); + + if selected_package.package_name.starts_with("npm:") { + config_for_npm_deps + .borrow_mut() + .as_mut() + .unwrap() + .add(selected_package); + } else { + config_for_other_deps + .borrow_mut() + .as_mut() + .unwrap() + .add(selected_package); + } + } + + let mut commit_futures = vec![]; + if let Some(npm) = config_for_npm_deps.take() { + commit_futures.push(npm.commit()); + } + if let Some(deno) = config_for_other_deps.take() { + commit_futures.push(deno.commit()); + } + let commit_futures = + deno_core::futures::future::join_all(commit_futures).await; + + for result in commit_futures { + match result { + Ok(()) => {} + Err(CommitError::HasImportMap) => { + bail!( + "`deno add` is not supported when configuration file contains an \"importMap\" field. Inline the import map into the Deno configuration file." + ); + } + Err(CommitError::ParseError(err)) => { + bail!("Failed updating config file due to parse error: {err}"); + } + Err(CommitError::ConfigNotObject) => { + bail!("Failed updating config file because it isn't an object."); + } + Err(CommitError::RemoteConfig) => { + bail!("Can't add dependencies to a remote configuration file"); + } + Err(CommitError::WriteFileError(err)) => { + bail!("Failed to update configuration file: {err}"); + } + } + } + + // 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); + // cache deps + cache_deps::cache_top_level_deps(&cli_factory, Some(jsr_resolver)).await?; + + Ok(()) +} + struct SelectedPackage { import_name: String, package_name: String, From 45bf8d81150832679b7dd54c26654e65a992a9aa Mon Sep 17 00:00:00 2001 From: Nathan Whitaker Date: Thu, 5 Sep 2024 10:20:50 -0700 Subject: [PATCH 2/9] Refactor --- Cargo.lock | 1 + Cargo.toml | 1 + cli/Cargo.toml | 1 + cli/tools/registry/pm.rs | 500 ++++++++++----------------------------- ext/node/Cargo.toml | 2 +- 5 files changed, 126 insertions(+), 379 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 195f55069c0168..60ce75fab46eb0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1254,6 +1254,7 @@ dependencies = [ "which 4.4.2", "winapi", "winres", + "yoke", "zeromq", "zip", "zstd", diff --git a/Cargo.toml b/Cargo.toml index 70d74eb9a95aaf..19a9d48f9b0ebf 100644 --- a/Cargo.toml +++ b/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" diff --git a/cli/Cargo.toml b/cli/Cargo.toml index a1d8f31dd0e551..de5d5424bcafed 100644 --- a/cli/Cargo.toml +++ b/cli/Cargo.toml @@ -162,6 +162,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 diff --git a/cli/tools/registry/pm.rs b/cli/tools/registry/pm.rs index c461af34e24a8c..02babcea263f44 100644 --- a/cli/tools/registry/pm.rs +++ b/cli/tools/registry/pm.rs @@ -26,6 +26,7 @@ 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; @@ -57,15 +58,6 @@ impl DenoConfigFormat { } } -trait DepManifest { - fn specifier(&self) -> Cow; - fn existing_imports(&self) -> Result, AnyError>; - fn fmt_options(&self) -> FmtOptionsConfig; - fn imports_key(&self) -> &'static str; - fn file_name(&self) -> &'static str; - fn import_entry(&self, selected: SelectedPackage) -> (String, String); -} - struct DenoConfig { config: Arc, format: DenoConfigFormat, @@ -83,6 +75,15 @@ impl DenoConfig { Ok(None) } } + + fn new( + config: Arc, + ) -> Result { + Ok(Self { + format: DenoConfigFormat::from_specifier(&config.specifier)?, + config, + }) + } } impl NpmConfig { @@ -97,6 +98,16 @@ impl NpmConfig { Ok(None) } } + + fn new( + config: Arc, + fmt_options: Option, + ) -> Self { + Self { + config, + fmt_options, + } + } } struct NpmConfig { @@ -105,150 +116,39 @@ struct NpmConfig { } enum DenoOrPackageJson { - Deno(Arc, DenoConfigFormat), - Npm(Arc, Option), + Deno(DenoConfig), + Npm(NpmConfig), } -impl DepManifest for Box { - fn specifier(&self) -> Cow { - (**self).specifier() - } - - fn existing_imports(&self) -> Result, AnyError> { - (**self).existing_imports() - } - - fn fmt_options(&self) -> FmtOptionsConfig { - (**self).fmt_options() - } - - fn imports_key(&self) -> &'static str { - (**self).imports_key() - } - - fn file_name(&self) -> &'static str { - (**self).file_name() - } - - fn import_entry(&self, selected: SelectedPackage) -> (String, String) { - (**self).import_entry(selected) +impl From for DenoOrPackageJson { + fn from(config: DenoConfig) -> Self { + Self::Deno(config) } } -impl DepManifest for DenoConfig { - fn specifier(&self) -> Cow { - Cow::Borrowed(&self.config.specifier) - } - - fn existing_imports(&self) -> Result, AnyError> { - if let Some(imports) = self.config.json.imports.clone() { - match serde_json::from_value(imports) { - Ok(map) => Ok(map), - Err(err) => { - bail!("Malformed \"imports\" configuration: {err}") - } - } - } else { - Ok(Default::default()) - } - } - - fn fmt_options(&self) -> FmtOptionsConfig { - self - .config - .to_fmt_config() - .ok() - .map(|f| f.options) - .unwrap_or_default() - } - - fn imports_key(&self) -> &'static str { - "imports" - } - - fn file_name(&self) -> &'static str { - match self.format { - DenoConfigFormat::Json => "deno.json", - DenoConfigFormat::Jsonc => "deno.jsonc", - } - } - - fn import_entry(&self, selected: SelectedPackage) -> (String, String) { - ( - selected.import_name, - format!("{}@{}", selected.package_name, selected.version_req), - ) +impl From for DenoOrPackageJson { + fn from(config: NpmConfig) -> Self { + Self::Npm(config) } } -impl DepManifest for NpmConfig { - fn specifier(&self) -> Cow { - Cow::Owned(self.config.specifier()) - } - - fn existing_imports(&self) -> Result, AnyError> { - Ok(self.config.dependencies.clone().unwrap_or_default()) - } - - fn fmt_options(&self) -> FmtOptionsConfig { - self.fmt_options.clone().unwrap_or_default() - } - - fn imports_key(&self) -> &'static str { - "dependencies" - } +#[derive(yoke::Yokeable)] +struct JsoncObjectView<'a>(jsonc_parser::ast::Object<'a>); - fn file_name(&self) -> &'static str { - "package.json" - } - - fn import_entry(&self, selected: SelectedPackage) -> (String, String) { - package_json_dependency_entry(selected) - } -} - -struct MutableManifest { +struct MutConfig { imports: IndexMap, - config: C, + config: DenoOrPackageJson, + ast: Yoke, String>, + path: PathBuf, } -enum CommitError { - HasImportMap, - ParseError(jsonc_parser::errors::ParseError), - ConfigNotObject, - RemoteConfig, - WriteFileError(std::io::Error), -} - -impl MutableManifest { - fn into_dyn(self) -> MutableManifest> { - MutableManifest { - imports: self.imports, - config: Box::new(self.config), - } - } -} -impl MutableManifest { - fn new(config: C) -> Result { +impl MutConfig { + async fn new(config: DenoOrPackageJson) -> Result { let specifier = config.specifier(); if specifier.scheme() != "file" { bail!("Can't add dependencies to a remote configuration file"); } - let imports = config.existing_imports()?; - Ok(Self { imports, config }) - } - - fn add(&mut self, selected: SelectedPackage) { - let (name, version) = self.config.import_entry(selected); - self.imports.insert(name, version); - } - - async fn commit(self) -> Result<(), CommitError> { - let config_specifier = self.config.specifier(); - if config_specifier.scheme() != "file" { - return Err(CommitError::RemoteConfig); - } - let config_file_path = config_specifier.to_file_path().unwrap(); + let config_file_path = specifier.to_file_path().unwrap(); let config_file_contents = { let contents = tokio::fs::read_to_string(&config_file_path).await.unwrap(); @@ -258,22 +158,48 @@ impl MutableManifest { contents } }; - let ast = jsonc_parser::parse_to_ast( - &config_file_contents, - &Default::default(), - &Default::default(), - ) - .map_err(CommitError::ParseError)?; - - let obj = match ast.value { - Some(Value::Object(obj)) => obj, - _ => return Err(CommitError::ConfigNotObject), - }; + 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 updating config file because it isn't an object."), + }; + Ok::<_, AnyError>(JsoncObjectView(obj)) + })?; - if obj.get_string("importMap").is_some() { - return Err(CommitError::HasImportMap); + let obj: &JsoncObjectView = ast.get(); + if obj.0.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 {}", + ), + specifier + ); } + let imports = config.existing_imports()?; + Ok(Self { + imports, + config, + ast, + path: config_file_path, + }) + } + fn add(&mut self, selected: SelectedPackage) { + let (name, version) = self.config.import_entry(selected); + self.imports.insert(name, version); + } + + async fn commit(self) -> Result<(), AnyError> { let mut import_list: Vec<(String, String)> = self.imports.into_iter().collect(); @@ -283,17 +209,15 @@ impl MutableManifest { let fmt_config_options = self.config.fmt_options(); let new_text = update_config_file_content( - obj, - &config_file_contents, + &self.ast.get().0, + self.ast.backing_cart(), generated_imports, fmt_config_options, self.config.imports_key(), self.config.file_name(), ); - tokio::fs::write(&config_file_path, new_text) - .await - .map_err(CommitError::WriteFileError)?; + tokio::fs::write(&self.path, new_text).await?; Ok(()) } } @@ -301,8 +225,8 @@ impl MutableManifest { impl DenoOrPackageJson { fn specifier(&self) -> Cow { match self { - Self::Deno(d, ..) => Cow::Borrowed(&d.specifier), - Self::Npm(n, ..) => Cow::Owned(n.specifier()), + Self::Deno(d, ..) => Cow::Borrowed(&d.config.specifier), + Self::Npm(n, ..) => Cow::Owned(n.config.specifier()), } } @@ -310,7 +234,7 @@ impl DenoOrPackageJson { fn existing_imports(&self) -> Result, AnyError> { match self { DenoOrPackageJson::Deno(deno, ..) => { - if let Some(imports) = deno.json.imports.clone() { + if let Some(imports) = deno.config.json.imports.clone() { match serde_json::from_value(imports) { Ok(map) => Ok(map), Err(err) => { @@ -322,7 +246,7 @@ impl DenoOrPackageJson { } } DenoOrPackageJson::Npm(npm, ..) => { - Ok(npm.dependencies.clone().unwrap_or_default()) + Ok(npm.config.dependencies.clone().unwrap_or_default()) } } } @@ -330,11 +254,14 @@ impl DenoOrPackageJson { 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() + } } } @@ -347,7 +274,7 @@ impl DenoOrPackageJson { 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", }, @@ -355,8 +282,14 @@ impl DenoOrPackageJson { } } - fn is_npm(&self) -> bool { - matches!(self, Self::Npm(..)) + fn import_entry(&self, selected: SelectedPackage) -> (String, String) { + match self { + DenoOrPackageJson::Deno(_) => ( + selected.import_name, + format!("{}@{}", selected.package_name, selected.version_req), + ), + DenoOrPackageJson::Npm(_) => package_json_dependency_entry(selected), + } } /// Get the preferred config file to operate on @@ -373,29 +306,20 @@ impl DenoOrPackageJson { // when both are present, for now, // default to deno.json (Some(deno), Some(_) | None) => Ok(( - DenoOrPackageJson::Deno( - deno.clone(), - DenoConfigFormat::from_specifier(&deno.specifier)?, - ), + DenoOrPackageJson::Deno(DenoConfig::new(deno.clone())?), + factory, + )), + (None, Some(package_json)) => Ok(( + DenoOrPackageJson::Npm(NpmConfig::new(package_json.clone(), None)), factory, )), - (None, Some(package_json)) => { - Ok((DenoOrPackageJson::Npm(package_json.clone(), None), factory)) - } (None, None) => { - 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 factory = create_deno_json(&flags, options)?; 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, + DenoConfig::from_options(&options)? + .expect("Just created deno.json"), ), factory, )) @@ -447,171 +371,6 @@ impl std::fmt::Display for AddCommandName { } } -pub async fn add_( - flags: Arc, - add_flags: AddFlags, - cmd_name: AddCommandName, -) -> Result<(), AnyError> { - let (config_file, cli_factory) = - DenoOrPackageJson::from_flags(flags.clone())?; - - let config_specifier = config_file.specifier(); - if config_specifier.scheme() != "file" { - bail!("Can't add dependencies to a remote configuration file"); - } - let config_file_path = config_specifier.to_file_path().unwrap(); - - let http_client = cli_factory.http_client_provider(); - - let mut selected_packages = Vec::with_capacity(add_flags.packages.len()); - let mut package_reqs = Vec::with_capacity(add_flags.packages.len()); - - for entry_text in add_flags.packages.iter() { - let req = AddPackageReq::parse(entry_text).with_context(|| { - format!("Failed to parse package required: {}", entry_text) - })?; - - package_reqs.push(req); - } - - let deps_http_cache = cli_factory.global_http_cache()?; - let mut deps_file_fetcher = FileFetcher::new( - deps_http_cache.clone(), - CacheSetting::ReloadAll, - true, - http_client.clone(), - Default::default(), - None, - ); - deps_file_fetcher.set_download_log_level(log::Level::Trace); - let deps_file_fetcher = Arc::new(deps_file_fetcher); - let jsr_resolver = Arc::new(JsrFetchResolver::new(deps_file_fetcher.clone())); - let npm_resolver = Arc::new(NpmFetchResolver::new(deps_file_fetcher)); - - let package_futures = package_reqs - .into_iter() - .map({ - let jsr_resolver = jsr_resolver.clone(); - move |package_req| { - find_package_and_select_version_for_req( - jsr_resolver.clone(), - npm_resolver.clone(), - package_req, - ) - .boxed_local() - } - }) - .collect::>(); - - let stream_of_futures = deno_core::futures::stream::iter(package_futures); - let mut buffered = stream_of_futures.buffered(10); - - while let Some(package_and_version_result) = buffered.next().await { - let package_and_version = package_and_version_result?; - - match package_and_version { - PackageAndVersion::NotFound { - package: package_name, - found_npm_package, - package_req, - } => { - if found_npm_package { - bail!("{} was not found, but a matching npm package exists. Did you mean `{}`?", crate::colors::red(package_name), crate::colors::yellow(format!("deno {cmd_name} npm:{package_req}"))); - } else { - bail!("{} was not found.", crate::colors::red(package_name)); - } - } - PackageAndVersion::Selected(selected) => { - selected_packages.push(selected); - } - } - } - - 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 {}{}{}", - crate::colors::green(&selected_package.package_name), - crate::colors::gray("@"), - selected_package.selected_version - ); - - if is_npm { - let (name, version) = package_json_dependency_entry(selected_package); - existing_imports.insert(name, version) - } else { - existing_imports.insert( - selected_package.import_name, - format!( - "{}@{}", - selected_package.package_name, selected_package.version_req - ), - ) - }; - } - 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 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")?; - - // 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); - // cache deps - cache_deps::cache_top_level_deps(&cli_factory, Some(jsr_resolver)).await?; - - Ok(()) -} - pub async fn add( flags: Arc, add_flags: AddFlags, @@ -636,12 +395,16 @@ pub async fn add( }; assert!(deno_config.is_some() || npm_config.is_some()); - let npm_config = npm_config - .map(|config| MutableManifest::new(config)) - .transpose()?; - let deno_config = deno_config - .map(|config| MutableManifest::new(config)) - .transpose()?; + let npm_config = if let Some(config) = npm_config { + Some(MutConfig::new(config.into()).await?) + } else { + None + }; + let deno_config = if let Some(config) = deno_config { + Some(MutConfig::new(config.into()).await?) + } else { + None + }; let http_client = cli_factory.http_client_provider(); @@ -698,9 +461,9 @@ pub async fn add( package_req, } => { if found_npm_package { - log::error!("{} was not found, but a matching npm package exists. Did you mean `{}`?", crate::colors::red(package_name), crate::colors::yellow(format!("deno {cmd_name} npm:{package_req}"))); + bail!("{} was not found, but a matching npm package exists. Did you mean `{}`?", crate::colors::red(package_name), crate::colors::yellow(format!("deno {cmd_name} npm:{package_req}"))); } else { - log::error!("{} was not found.", crate::colors::red(package_name)); + bail!("{} was not found.", crate::colors::red(package_name)); } } PackageAndVersion::Selected(selected) => { @@ -712,15 +475,15 @@ pub async fn add( let (config_for_npm_deps, config_for_other_deps) = match (npm_config, deno_config) { (Some(npm), Some(deno)) => ( - Rc::new(RefCell::new(Some(npm.into_dyn()))), - Rc::new(RefCell::new(Some(deno.into_dyn()))), + Rc::new(RefCell::new(Some(npm))), + Rc::new(RefCell::new(Some(deno))), ), (Some(npm), None) => { - let shared = Rc::new(RefCell::new(Some(npm.into_dyn()))); + let shared = Rc::new(RefCell::new(Some(npm))); (shared.clone(), shared) } (None, Some(deno)) => { - let shared = Rc::new(RefCell::new(Some(deno.into_dyn()))); + let shared = Rc::new(RefCell::new(Some(deno))); (shared.clone(), shared) } (None, None) => unreachable!(), @@ -760,26 +523,7 @@ pub async fn add( deno_core::futures::future::join_all(commit_futures).await; for result in commit_futures { - match result { - Ok(()) => {} - Err(CommitError::HasImportMap) => { - bail!( - "`deno add` is not supported when configuration file contains an \"importMap\" field. Inline the import map into the Deno configuration file." - ); - } - Err(CommitError::ParseError(err)) => { - bail!("Failed updating config file due to parse error: {err}"); - } - Err(CommitError::ConfigNotObject) => { - bail!("Failed updating config file because it isn't an object."); - } - Err(CommitError::RemoteConfig) => { - bail!("Can't add dependencies to a remote configuration file"); - } - Err(CommitError::WriteFileError(err)) => { - bail!("Failed to update configuration file: {err}"); - } - } + result.context("Failed to update configuration file")?; } // clear the previously cached package.json from memory before reloading it @@ -1041,7 +785,7 @@ pub async fn remove( } fn update_config_file_content( - obj: jsonc_parser::ast::Object, + obj: &jsonc_parser::ast::Object, config_file_contents: &str, generated_imports: String, fmt_options: FmtOptionsConfig, diff --git a/ext/node/Cargo.toml b/ext/node/Cargo.toml index 783ad45c4b0b49..a6ee09064f4ed9 100644 --- a/ext/node/Cargo.toml +++ b/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 From 3bcb5fa0a1ad43c006eb875b442a1df0936e113c Mon Sep 17 00:00:00 2001 From: Nathan Whitaker Date: Thu, 5 Sep 2024 13:53:57 -0700 Subject: [PATCH 3/9] Refactor --- cli/tools/registry/pm.rs | 450 +++++++++++++++++++-------------------- 1 file changed, 222 insertions(+), 228 deletions(-) diff --git a/cli/tools/registry/pm.rs b/cli/tools/registry/pm.rs index 02babcea263f44..edab72f27de29a 100644 --- a/cli/tools/registry/pm.rs +++ b/cli/tools/registry/pm.rs @@ -6,9 +6,7 @@ pub use cache_deps::cache_top_level_deps; use std::borrow::Cow; use std::cell::RefCell; -use std::path::Path; use std::path::PathBuf; -use std::rc::Rc; use std::sync::Arc; use deno_ast::TextChange; @@ -61,28 +59,54 @@ impl DenoConfigFormat { struct DenoConfig { config: Arc, format: DenoConfigFormat, + imports: IndexMap, } +fn deno_json_imports( + config: &deno_config::deno_json::ConfigFile, +) -> Result, 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, AnyError> { let start_dir = &options.start_dir; - if let Some(deno) = start_dir.maybe_deno_json() { + if let Some(config) = start_dir.maybe_deno_json() { Ok(Some(Self { - config: deno.clone(), - format: DenoConfigFormat::from_specifier(&deno.specifier)?, + imports: deno_json_imports(config)?, + config: config.clone(), + format: DenoConfigFormat::from_specifier(&config.specifier)?, })) } else { Ok(None) } } - fn new( - config: Arc, - ) -> Result { - Ok(Self { - format: DenoConfigFormat::from_specifier(&config.specifier)?, - config, - }) + 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)> { + vec![("imports", std::mem::take(&mut self.imports))] } } @@ -91,6 +115,8 @@ impl NpmConfig { 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, })) @@ -99,20 +125,39 @@ impl NpmConfig { } } - fn new( - config: Arc, - fmt_options: Option, - ) -> Self { - Self { - config, - fmt_options, + 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)> { + vec![ + ("dependencies", std::mem::take(&mut self.dependencies)), + ( + "devDependencies", + std::mem::take(&mut self.dev_dependencies), + ), + ] + } } struct NpmConfig { config: Arc, fmt_options: Option, + dependencies: IndexMap, + dev_dependencies: IndexMap, } enum DenoOrPackageJson { @@ -136,13 +181,27 @@ impl From for DenoOrPackageJson { struct JsoncObjectView<'a>(jsonc_parser::ast::Object<'a>); struct MutConfig { - imports: IndexMap, config: DenoOrPackageJson, + // the `Yoke` is so we can carry the parsed object (which borrows from + // the source) along with the source itself ast: Yoke, String>, path: PathBuf, + modified: bool, } impl MutConfig { + fn ast(&self) -> &jsonc_parser::ast::Object<'_> { + &self.ast.get().0 + } + async fn maybe_new( + config: Option>, + ) -> Result, AnyError> { + if let Some(config) = config { + Ok(Some(Self::new(config.into()).await?)) + } else { + Ok(None) + } + } async fn new(config: DenoOrPackageJson) -> Result { let specifier = config.specifier(); if specifier.scheme() != "file" { @@ -173,47 +232,56 @@ impl MutConfig { }; Ok::<_, AnyError>(JsoncObjectView(obj)) })?; - - let obj: &JsoncObjectView = ast.get(); - if obj.0.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 {}", - ), - specifier - ); - } - let imports = config.existing_imports()?; Ok(Self { - imports, config, ast, path: config_file_path, + modified: false, }) } - fn add(&mut self, selected: SelectedPackage) { - let (name, version) = self.config.import_entry(selected); - self.imports.insert(name, version); + 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; } - async fn commit(self) -> Result<(), AnyError> { - let mut import_list: Vec<(String, String)> = - self.imports.into_iter().collect(); + 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 + } - import_list.sort_by(|(k1, _), (k2, _)| k1.cmp(k2)); - let generated_imports = generate_imports(import_list); + 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.ast.get().0, self.ast.backing_cart(), - generated_imports, fmt_config_options, - self.config.imports_key(), + 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(), ); @@ -230,27 +298,6 @@ impl DenoOrPackageJson { } } - /// Returns the existing imports/dependencies from the config. - fn existing_imports(&self) -> Result, AnyError> { - match self { - DenoOrPackageJson::Deno(deno, ..) => { - if let Some(imports) = deno.config.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.config.dependencies.clone().unwrap_or_default()) - } - } - } - fn fmt_options(&self) -> FmtOptionsConfig { match self { DenoOrPackageJson::Deno(deno, ..) => deno @@ -265,10 +312,12 @@ impl DenoOrPackageJson { } } - fn imports_key(&self) -> &'static str { + fn take_import_fields( + &mut self, + ) -> Vec<(&'static str, IndexMap)> { match self { - DenoOrPackageJson::Deno(..) => "imports", - DenoOrPackageJson::Npm(..) => "dependencies", + Self::Deno(d) => d.take_import_fields(), + Self::Npm(n) => n.take_import_fields(), } } @@ -281,51 +330,6 @@ impl DenoOrPackageJson { DenoOrPackageJson::Npm(..) => "package.json", } } - - fn import_entry(&self, selected: SelectedPackage) -> (String, String) { - match self { - DenoOrPackageJson::Deno(_) => ( - selected.import_name, - format!("{}@{}", selected.package_name, selected.version_req), - ), - DenoOrPackageJson::Npm(_) => package_json_dependency_entry(selected), - } - } - - /// 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) -> 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(DenoConfig::new(deno.clone())?), - factory, - )), - (None, Some(package_json)) => Ok(( - DenoOrPackageJson::Npm(NpmConfig::new(package_json.clone(), None)), - factory, - )), - (None, None) => { - let factory = create_deno_json(&flags, options)?; - let options = factory.cli_options()?.clone(); - Ok(( - DenoOrPackageJson::Deno( - DenoConfig::from_options(&options)? - .expect("Just created deno.json"), - ), - factory, - )) - } - } - } } fn create_deno_json( @@ -371,11 +375,9 @@ impl std::fmt::Display for AddCommandName { } } -pub async fn add( - flags: Arc, - add_flags: AddFlags, - cmd_name: AddCommandName, -) -> Result<(), AnyError> { +fn load_configs( + flags: &Arc, +) -> Result<(CliFactory, Option, Option), AnyError> { let cli_factory = CliFactory::from_flags(flags.clone()); let options = cli_factory.cli_options()?; let npm_config = NpmConfig::from_options(options)?; @@ -383,7 +385,7 @@ pub async fn add( Some(config) => (cli_factory, Some(config)), None if npm_config.is_some() => (cli_factory, None), None => { - let factory = create_deno_json(&flags, &options)?; + let factory = create_deno_json(flags, &options)?; let options = factory.cli_options()?.clone(); ( factory, @@ -394,14 +396,35 @@ pub async fn add( } }; assert!(deno_config.is_some() || npm_config.is_some()); + Ok((cli_factory, npm_config, deno_config)) +} + +pub async fn add( + flags: Arc, + add_flags: AddFlags, + cmd_name: AddCommandName, +) -> Result<(), AnyError> { + let (cli_factory, npm_config, deno_config) = load_configs(&flags)?; let npm_config = if let Some(config) = npm_config { - Some(MutConfig::new(config.into()).await?) + Some(RefCell::new(MutConfig::new(config.into()).await?)) } else { None }; - let deno_config = if let Some(config) = deno_config { - Some(MutConfig::new(config.into()).await?) + let deno_config = if let Some(deno) = deno_config { + let specifier = deno.config.specifier.to_string(); + let config = MutConfig::new(deno.into()).await?; + if config.ast().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 {}", + ), + specifier + ); + } + Some(RefCell::new(config)) } else { None }; @@ -473,19 +496,9 @@ pub async fn add( } let (config_for_npm_deps, config_for_other_deps) = - match (npm_config, deno_config) { - (Some(npm), Some(deno)) => ( - Rc::new(RefCell::new(Some(npm))), - Rc::new(RefCell::new(Some(deno))), - ), - (Some(npm), None) => { - let shared = Rc::new(RefCell::new(Some(npm))); - (shared.clone(), shared) - } - (None, Some(deno)) => { - let shared = Rc::new(RefCell::new(Some(deno))); - (shared.clone(), shared) - } + match (npm_config.as_ref(), deno_config.as_ref()) { + (Some(npm), Some(deno)) => (npm, deno), + (Some(config), None) | (None, Some(config)) => (config, config), (None, None) => unreachable!(), }; @@ -500,24 +513,20 @@ pub async fn add( if selected_package.package_name.starts_with("npm:") { config_for_npm_deps .borrow_mut() - .as_mut() - .unwrap() - .add(selected_package); + .add(selected_package, false); } else { config_for_other_deps .borrow_mut() - .as_mut() - .unwrap() - .add(selected_package); + .add(selected_package, false); } } let mut commit_futures = vec![]; - if let Some(npm) = config_for_npm_deps.take() { - commit_futures.push(npm.commit()); + if let Some(npm) = npm_config { + commit_futures.push(npm.into_inner().commit()); } - if let Some(deno) = config_for_other_deps.take() { - commit_futures.push(deno.commit()); + if let Some(deno) = deno_config { + commit_futures.push(deno.into_inner().commit()); } let commit_futures = deno_core::futures::future::join_all(commit_futures).await; @@ -692,7 +701,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() { @@ -705,68 +715,29 @@ 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, - 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, 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 = [ + MutConfig::maybe_new(npm_config).await?, + MutConfig::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![]; - 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, - )?; + for package in &remove_flags.packages { + let mut removed = false; + for config in &mut configs { + if let Some(config) = config { + removed |= config.remove(package); + } + } + if removed { + removed_packages.push(package.clone()); + } } if removed_packages.is_empty() { @@ -775,6 +746,12 @@ pub async fn remove( for package in &removed_packages { log::info!("Removed {}", crate::colors::green(package)); } + for config in configs { + if let Some(config) = config { + config.commit().await?; + } + } + // Update deno.lock node_resolver::PackageJsonThreadLocalCache::clear(); let cli_factory = CliFactory::from_flags(flags); @@ -784,41 +761,58 @@ pub async fn remove( Ok(()) } -fn update_config_file_content( +fn update_config_file_content< + I: IntoIterator)>, +>( 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 { - value: Value::Object(lit), - .. - }) => text_changes.push(TextChange { - range: (lit.range.start + 1)..(lit.range.end - 1), - new_text: generated_imports, - }), - None => { - let insert_position = obj.range.end - 1; - text_changes.push(TextChange { - range: insert_position..insert_position, - // NOTE(bartlomieju): adding `\n` here to force the formatter to always - // produce a config file that is multiline, like so: - // ``` - // { - // "imports": { - // "": ":@" - // } - // } - new_text: format!("\"{imports_key}\": {{\n {generated_imports} }}"), - }) + for (key, value) in entries { + match obj.get(key) { + Some(ObjectProp { + value: Value::Object(lit), + range, + .. + }) => { + if let Some(value) = value { + text_changes.push(TextChange { + range: (lit.range.start + 1)..(lit.range.end - 1), + new_text: value, + }) + } else { + text_changes.push(TextChange { + range: range.start..range.end, + 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, + // NOTE(bartlomieju): adding `\n` here to force the formatter to always + // produce a config file that is multiline, like so: + // ``` + // { + // "imports": { + // "": ":@" + // } + // } + new_text: format!("\"{key}\": {{\n {value} }}"), + }) + } + } + // we verified the shape of `imports`/`dependencies` above + Some(_) => unreachable!(), } - // we verified the shape of `imports`/`dependencies` above - Some(_) => unreachable!(), } let new_text = From 89dcdedee5283809d216ba71a45079fac8ae402b Mon Sep 17 00:00:00 2001 From: Nathan Whitaker Date: Thu, 5 Sep 2024 14:57:25 -0700 Subject: [PATCH 4/9] Cleanup --- cli/tools/registry/pm.rs | 76 +++++++++++++++++++--------------------- 1 file changed, 37 insertions(+), 39 deletions(-) diff --git a/cli/tools/registry/pm.rs b/cli/tools/registry/pm.rs index edab72f27de29a..dfec9122d275a0 100644 --- a/cli/tools/registry/pm.rs +++ b/cli/tools/registry/pm.rs @@ -177,10 +177,11 @@ impl From for DenoOrPackageJson { } } +/// 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 MutConfig { +struct ModifiableConfig { config: DenoOrPackageJson, // the `Yoke` is so we can carry the parsed object (which borrows from // the source) along with the source itself @@ -189,10 +190,13 @@ struct MutConfig { modified: bool, } -impl MutConfig { - fn ast(&self) -> &jsonc_parser::ast::Object<'_> { +impl ModifiableConfig { + 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>, ) -> Result, AnyError> { @@ -205,7 +209,7 @@ impl MutConfig { async fn new(config: DenoOrPackageJson) -> Result { let specifier = config.specifier(); if specifier.scheme() != "file" { - bail!("Can't add dependencies to a remote configuration file"); + bail!("Can't update a remote configuration file"); } let config_file_path = specifier.to_file_path().unwrap(); let config_file_contents = { @@ -228,9 +232,12 @@ impl MutConfig { })?; let obj = match ast.value { Some(Value::Object(obj)) => obj, - _ => bail!("Failed updating config file because it isn't an object."), + _ => bail!( + "Failed to update config file at {}, expected an object", + specifier + ), }; - Ok::<_, AnyError>(JsoncObjectView(obj)) + Ok(JsoncObjectView(obj)) })?; Ok(Self { config, @@ -269,8 +276,8 @@ impl MutConfig { let fmt_config_options = self.config.fmt_options(); let new_text = update_config_file_content( - &self.ast.get().0, - self.ast.backing_cart(), + self.obj(), + self.contents(), fmt_config_options, import_fields.into_iter().map(|(k, v)| { ( @@ -405,29 +412,23 @@ pub async fn add( cmd_name: AddCommandName, ) -> Result<(), AnyError> { let (cli_factory, npm_config, deno_config) = load_configs(&flags)?; + let mut npm_config = ModifiableConfig::maybe_new(npm_config).await?; + let mut deno_config = ModifiableConfig::maybe_new(deno_config).await?; - let npm_config = if let Some(config) = npm_config { - Some(RefCell::new(MutConfig::new(config.into()).await?)) - } else { - None - }; - let deno_config = if let Some(deno) = deno_config { - let specifier = deno.config.specifier.to_string(); - let config = MutConfig::new(deno.into()).await?; - if config.ast().get_string("importMap").is_some() { + if let Some(deno) = &deno_config { + let specifier = deno.config.specifier(); + if deno.obj().get_string("importMap").is_some() { bail!( concat!( - "`deno add` is not supported when configuration file contains an \"importMap\" field. ", + "`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 ); } - Some(RefCell::new(config)) - } else { - None - }; + } let http_client = cli_factory.http_client_provider(); @@ -495,13 +496,6 @@ pub async fn add( } } - let (config_for_npm_deps, config_for_other_deps) = - match (npm_config.as_ref(), deno_config.as_ref()) { - (Some(npm), Some(deno)) => (npm, deno), - (Some(config), None) | (None, Some(config)) => (config, config), - (None, None) => unreachable!(), - }; - for selected_package in selected_packages { log::info!( "Add {}{}{}", @@ -511,22 +505,26 @@ pub async fn add( ); if selected_package.package_name.starts_with("npm:") { - config_for_npm_deps - .borrow_mut() - .add(selected_package, false); + if let Some(npm) = &mut npm_config { + npm.add(selected_package, false); + } else { + deno_config.as_mut().unwrap().add(selected_package, false); + } } else { - config_for_other_deps - .borrow_mut() - .add(selected_package, false); + if let Some(deno) = &mut deno_config { + deno.add(selected_package, false); + } else { + npm_config.as_mut().unwrap().add(selected_package, false); + } } } let mut commit_futures = vec![]; if let Some(npm) = npm_config { - commit_futures.push(npm.into_inner().commit()); + commit_futures.push(npm.commit()); } if let Some(deno) = deno_config { - commit_futures.push(deno.into_inner().commit()); + commit_futures.push(deno.commit()); } let commit_futures = deno_core::futures::future::join_all(commit_futures).await; @@ -722,8 +720,8 @@ pub async fn remove( let (_, npm_config, deno_config) = load_configs(&flags)?; let mut configs = [ - MutConfig::maybe_new(npm_config).await?, - MutConfig::maybe_new(deno_config).await?, + ModifiableConfig::maybe_new(npm_config).await?, + ModifiableConfig::maybe_new(deno_config).await?, ]; let mut removed_packages = vec![]; From 6767e9ce07a70554ba4cea2272cef48d087c4449 Mon Sep 17 00:00:00 2001 From: Nathan Whitaker Date: Thu, 5 Sep 2024 15:48:21 -0700 Subject: [PATCH 5/9] Fix + tests --- cli/tools/registry/pm.rs | 31 +++++++++---- .../package_json_and_deno_json/__test__.jsonc | 46 +++++++++++++++++++ .../add/package_json_and_deno_json/add.out | 12 +++++ .../add_esm_basic.out | 4 ++ .../add/package_json_and_deno_json/deno.json | 1 + .../npm_prefer_deno.json.out | 5 ++ .../npm_prefer_package.json.out | 6 +++ .../package_json_and_deno_json/package.json | 1 + tests/specs/remove/basic/__test__.jsonc | 3 ++ .../specs/remove/package_json/__test__.jsonc | 27 +++++++++++ tests/specs/remove/package_json/package.json | 4 ++ tests/specs/remove/package_json/rm_add.out | 4 ++ .../package_json/rm_add_package.json.out | 3 ++ .../remove/package_json/rm_esm_basic.out | 1 + .../rm_esm_basic_package.json.out | 2 + 15 files changed, 141 insertions(+), 9 deletions(-) create mode 100644 tests/specs/add/package_json_and_deno_json/__test__.jsonc create mode 100644 tests/specs/add/package_json_and_deno_json/add.out create mode 100644 tests/specs/add/package_json_and_deno_json/add_esm_basic.out create mode 100644 tests/specs/add/package_json_and_deno_json/deno.json create mode 100644 tests/specs/add/package_json_and_deno_json/npm_prefer_deno.json.out create mode 100644 tests/specs/add/package_json_and_deno_json/npm_prefer_package.json.out create mode 100644 tests/specs/add/package_json_and_deno_json/package.json create mode 100644 tests/specs/remove/package_json/__test__.jsonc create mode 100644 tests/specs/remove/package_json/package.json create mode 100644 tests/specs/remove/package_json/rm_add.out create mode 100644 tests/specs/remove/package_json/rm_add_package.json.out create mode 100644 tests/specs/remove/package_json/rm_esm_basic.out create mode 100644 tests/specs/remove/package_json/rm_esm_basic_package.json.out diff --git a/cli/tools/registry/pm.rs b/cli/tools/registry/pm.rs index dfec9122d275a0..8a87ed2e57a0c2 100644 --- a/cli/tools/registry/pm.rs +++ b/cli/tools/registry/pm.rs @@ -5,7 +5,6 @@ mod cache_deps; pub use cache_deps::cache_top_level_deps; use std::borrow::Cow; -use std::cell::RefCell; use std::path::PathBuf; use std::sync::Arc; @@ -769,14 +768,22 @@ fn update_config_file_content< file_name: &str, ) -> String { let mut text_changes = vec![]; - for (key, value) in entries { - match obj.get(key) { - Some(ObjectProp { - value: Value::Object(lit), - range, - .. - }) => { + 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, + .. + }, + )) => { if let Some(value) = value { text_changes.push(TextChange { range: (lit.range.start + 1)..(lit.range.end - 1), @@ -784,7 +791,13 @@ fn update_config_file_content< }) } else { text_changes.push(TextChange { - range: range.start..range.end, + // 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(), }) } diff --git a/tests/specs/add/package_json_and_deno_json/__test__.jsonc b/tests/specs/add/package_json_and_deno_json/__test__.jsonc new file mode 100644 index 00000000000000..4d886d98b37dfd --- /dev/null +++ b/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" + } + ] + } + } +} diff --git a/tests/specs/add/package_json_and_deno_json/add.out b/tests/specs/add/package_json_and_deno_json/add.out new file mode 100644 index 00000000000000..5577a55acc7b3b --- /dev/null +++ b/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] diff --git a/tests/specs/add/package_json_and_deno_json/add_esm_basic.out b/tests/specs/add/package_json_and_deno_json/add_esm_basic.out new file mode 100644 index 00000000000000..42161f3ae1d01c --- /dev/null +++ b/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 diff --git a/tests/specs/add/package_json_and_deno_json/deno.json b/tests/specs/add/package_json_and_deno_json/deno.json new file mode 100644 index 00000000000000..9e26dfeeb6e641 --- /dev/null +++ b/tests/specs/add/package_json_and_deno_json/deno.json @@ -0,0 +1 @@ +{} \ No newline at end of file diff --git a/tests/specs/add/package_json_and_deno_json/npm_prefer_deno.json.out b/tests/specs/add/package_json_and_deno_json/npm_prefer_deno.json.out new file mode 100644 index 00000000000000..38ca2d4b856529 --- /dev/null +++ b/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" + } +} diff --git a/tests/specs/add/package_json_and_deno_json/npm_prefer_package.json.out b/tests/specs/add/package_json_and_deno_json/npm_prefer_package.json.out new file mode 100644 index 00000000000000..b7b19afb35c2da --- /dev/null +++ b/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" + } +} diff --git a/tests/specs/add/package_json_and_deno_json/package.json b/tests/specs/add/package_json_and_deno_json/package.json new file mode 100644 index 00000000000000..0967ef424bce67 --- /dev/null +++ b/tests/specs/add/package_json_and_deno_json/package.json @@ -0,0 +1 @@ +{} diff --git a/tests/specs/remove/basic/__test__.jsonc b/tests/specs/remove/basic/__test__.jsonc index 2f4d82c88f3d8e..495496b5cdde68 100644 --- a/tests/specs/remove/basic/__test__.jsonc +++ b/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" }] } diff --git a/tests/specs/remove/package_json/__test__.jsonc b/tests/specs/remove/package_json/__test__.jsonc new file mode 100644 index 00000000000000..c51b00d0fc9aa3 --- /dev/null +++ b/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" + } + ] +} diff --git a/tests/specs/remove/package_json/package.json b/tests/specs/remove/package_json/package.json new file mode 100644 index 00000000000000..6de96bc2014a94 --- /dev/null +++ b/tests/specs/remove/package_json/package.json @@ -0,0 +1,4 @@ +{ + "dependencies": { "@denotest/add": "^1.0.0" }, + "devDependencies": { "@denotest/esm-basic": "^1.0.0" } +} diff --git a/tests/specs/remove/package_json/rm_add.out b/tests/specs/remove/package_json/rm_add.out new file mode 100644 index 00000000000000..b98c27bae627ae --- /dev/null +++ b/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 diff --git a/tests/specs/remove/package_json/rm_add_package.json.out b/tests/specs/remove/package_json/rm_add_package.json.out new file mode 100644 index 00000000000000..d5ca56e00478be --- /dev/null +++ b/tests/specs/remove/package_json/rm_add_package.json.out @@ -0,0 +1,3 @@ +{ + "devDependencies": { "@denotest/esm-basic": "^1.0.0" } +} diff --git a/tests/specs/remove/package_json/rm_esm_basic.out b/tests/specs/remove/package_json/rm_esm_basic.out new file mode 100644 index 00000000000000..86ad9e28da8c2c --- /dev/null +++ b/tests/specs/remove/package_json/rm_esm_basic.out @@ -0,0 +1 @@ +Removed @denotest/esm-basic diff --git a/tests/specs/remove/package_json/rm_esm_basic_package.json.out b/tests/specs/remove/package_json/rm_esm_basic_package.json.out new file mode 100644 index 00000000000000..2c63c0851048d8 --- /dev/null +++ b/tests/specs/remove/package_json/rm_esm_basic_package.json.out @@ -0,0 +1,2 @@ +{ +} From 775a26a9e2adbd1fcde125ebea8b00dc7d5c27d0 Mon Sep 17 00:00:00 2001 From: Nathan Whitaker Date: Thu, 5 Sep 2024 15:49:40 -0700 Subject: [PATCH 6/9] Rename --- cli/tools/registry/pm.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/cli/tools/registry/pm.rs b/cli/tools/registry/pm.rs index 8a87ed2e57a0c2..1819362351f73f 100644 --- a/cli/tools/registry/pm.rs +++ b/cli/tools/registry/pm.rs @@ -180,7 +180,7 @@ impl From for DenoOrPackageJson { #[derive(yoke::Yokeable)] struct JsoncObjectView<'a>(jsonc_parser::ast::Object<'a>); -struct ModifiableConfig { +struct ConfigUpdater { config: DenoOrPackageJson, // the `Yoke` is so we can carry the parsed object (which borrows from // the source) along with the source itself @@ -189,7 +189,7 @@ struct ModifiableConfig { modified: bool, } -impl ModifiableConfig { +impl ConfigUpdater { fn obj(&self) -> &jsonc_parser::ast::Object<'_> { &self.ast.get().0 } @@ -411,8 +411,8 @@ pub async fn add( cmd_name: AddCommandName, ) -> Result<(), AnyError> { let (cli_factory, npm_config, deno_config) = load_configs(&flags)?; - let mut npm_config = ModifiableConfig::maybe_new(npm_config).await?; - let mut deno_config = ModifiableConfig::maybe_new(deno_config).await?; + let mut npm_config = ConfigUpdater::maybe_new(npm_config).await?; + let mut deno_config = ConfigUpdater::maybe_new(deno_config).await?; if let Some(deno) = &deno_config { let specifier = deno.config.specifier(); @@ -719,8 +719,8 @@ pub async fn remove( let (_, npm_config, deno_config) = load_configs(&flags)?; let mut configs = [ - ModifiableConfig::maybe_new(npm_config).await?, - ModifiableConfig::maybe_new(deno_config).await?, + ConfigUpdater::maybe_new(npm_config).await?, + ConfigUpdater::maybe_new(deno_config).await?, ]; let mut removed_packages = vec![]; From 1f758737fa50aac4deea19bf5278d48572374307 Mon Sep 17 00:00:00 2001 From: Nathan Whitaker Date: Thu, 5 Sep 2024 16:13:56 -0700 Subject: [PATCH 7/9] Fmt --- tests/specs/add/package_json_and_deno_json/deno.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/specs/add/package_json_and_deno_json/deno.json b/tests/specs/add/package_json_and_deno_json/deno.json index 9e26dfeeb6e641..0967ef424bce67 100644 --- a/tests/specs/add/package_json_and_deno_json/deno.json +++ b/tests/specs/add/package_json_and_deno_json/deno.json @@ -1 +1 @@ -{} \ No newline at end of file +{} From 9812180dcedaab57fc3fc26715741f7256c86499 Mon Sep 17 00:00:00 2001 From: Nathan Whitaker Date: Thu, 5 Sep 2024 16:31:11 -0700 Subject: [PATCH 8/9] Appease clippy --- cli/tools/registry/pm.rs | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/cli/tools/registry/pm.rs b/cli/tools/registry/pm.rs index 1819362351f73f..46611afb9cebc6 100644 --- a/cli/tools/registry/pm.rs +++ b/cli/tools/registry/pm.rs @@ -222,7 +222,7 @@ impl ConfigUpdater { }; let ast = Yoke::try_attach_to_cart(config_file_contents, |contents| { let ast = jsonc_parser::parse_to_ast( - &contents, + contents, &Default::default(), &Default::default(), ) @@ -391,7 +391,7 @@ fn load_configs( Some(config) => (cli_factory, Some(config)), None if npm_config.is_some() => (cli_factory, None), None => { - let factory = create_deno_json(flags, &options)?; + let factory = create_deno_json(flags, options)?; let options = factory.cli_options()?.clone(); ( factory, @@ -509,12 +509,10 @@ pub async fn add( } else { deno_config.as_mut().unwrap().add(selected_package, false); } + } else if let Some(deno) = &mut deno_config { + deno.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); - } + npm_config.as_mut().unwrap().add(selected_package, false); } } @@ -727,10 +725,8 @@ pub async fn remove( for package in &remove_flags.packages { let mut removed = false; - for config in &mut configs { - if let Some(config) = config { - removed |= config.remove(package); - } + for config in configs.iter_mut().flatten() { + removed |= config.remove(package); } if removed { removed_packages.push(package.clone()); @@ -743,10 +739,8 @@ pub async fn remove( for package in &removed_packages { log::info!("Removed {}", crate::colors::green(package)); } - for config in configs { - if let Some(config) = config { - config.commit().await?; - } + for config in configs.into_iter().flatten() { + config.commit().await?; } // Update deno.lock From f1ff9ce23875844c06d8c6cd271270b7bdd1c09b Mon Sep 17 00:00:00 2001 From: Nathan Whitaker Date: Fri, 6 Sep 2024 09:42:27 -0700 Subject: [PATCH 9/9] Don't unwrap --- cli/tools/registry/pm.rs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/cli/tools/registry/pm.rs b/cli/tools/registry/pm.rs index 46611afb9cebc6..98986a5a38733f 100644 --- a/cli/tools/registry/pm.rs +++ b/cli/tools/registry/pm.rs @@ -210,10 +210,15 @@ impl ConfigUpdater { if specifier.scheme() != "file" { bail!("Can't update a remote configuration file"); } - let config_file_path = specifier.to_file_path().unwrap(); + 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.unwrap(); + 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 {