From c5449d71da2d623e866d733b6db180a6f94ff7c6 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Mon, 14 Oct 2024 15:35:52 -0400 Subject: fix(install): support installing npm package with alias (#26246) Just tried this out today and it wasn't properly implemented in https://github.com/denoland/deno/pull/24156 --- cli/tools/registry/pm.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) (limited to 'cli/tools/registry/pm.rs') diff --git a/cli/tools/registry/pm.rs b/cli/tools/registry/pm.rs index 5dc042620..3276acfbf 100644 --- a/cli/tools/registry/pm.rs +++ b/cli/tools/registry/pm.rs @@ -363,7 +363,14 @@ fn package_json_dependency_entry( selected: SelectedPackage, ) -> (String, String) { if let Some(npm_package) = selected.package_name.strip_prefix("npm:") { - (npm_package.into(), selected.version_req) + if selected.import_name == npm_package { + (npm_package.into(), selected.version_req) + } else { + ( + selected.import_name, + format!("npm:{}@{}", npm_package, selected.version_req), + ) + } } else if let Some(jsr_package) = selected.package_name.strip_prefix("jsr:") { let jsr_package = jsr_package.strip_prefix('@').unwrap_or(jsr_package); let scope_replaced = jsr_package.replace('/', "__"); @@ -741,6 +748,9 @@ fn generate_imports(mut packages_to_version: Vec<(String, String)>) -> String { let mut contents = vec![]; let len = packages_to_version.len(); for (index, (package, version)) in packages_to_version.iter().enumerate() { + if index == 0 { + contents.push(String::new()); // force a newline at the start + } // TODO(bartlomieju): fix it, once we start support specifying version on the cli contents.push(format!("\"{}\": \"{}\"", package, version)); if index != len - 1 { -- cgit v1.2.3 From 797405fc61b2d155941506fb53d498076e121017 Mon Sep 17 00:00:00 2001 From: Nathan Whitaker <17734409+nathanwhit@users.noreply.github.com> Date: Tue, 15 Oct 2024 09:38:42 -0700 Subject: fix(add): create deno.json when running `deno add jsr:` (#26275) Fixes https://github.com/denoland/deno/issues/26119. Originally I wanted to put them in package.json if there's no deno.json, but on second thought it makes more sense to just create a deno.json --- cli/tools/registry/pm.rs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) (limited to 'cli/tools/registry/pm.rs') diff --git a/cli/tools/registry/pm.rs b/cli/tools/registry/pm.rs index 3276acfbf..ff900d113 100644 --- a/cli/tools/registry/pm.rs +++ b/cli/tools/registry/pm.rs @@ -400,14 +400,17 @@ impl std::fmt::Display for AddCommandName { fn load_configs( flags: &Arc, + has_jsr_specifiers: impl FnOnce() -> bool, ) -> 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)?; 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 => { + None if npm_config.is_some() && !has_jsr_specifiers() => { + (cli_factory, None) + } + _ => { let factory = create_deno_json(flags, options)?; let options = factory.cli_options()?.clone(); ( @@ -427,7 +430,9 @@ pub async fn add( add_flags: AddFlags, cmd_name: AddCommandName, ) -> Result<(), AnyError> { - let (cli_factory, npm_config, deno_config) = load_configs(&flags)?; + let (cli_factory, npm_config, deno_config) = load_configs(&flags, || { + add_flags.packages.iter().any(|s| s.starts_with("jsr:")) + })?; let mut npm_config = ConfigUpdater::maybe_new(npm_config).await?; let mut deno_config = ConfigUpdater::maybe_new(deno_config).await?; @@ -764,7 +769,7 @@ pub async fn remove( flags: Arc, remove_flags: RemoveFlags, ) -> Result<(), AnyError> { - let (_, npm_config, deno_config) = load_configs(&flags)?; + let (_, npm_config, deno_config) = load_configs(&flags, || false)?; let mut configs = [ ConfigUpdater::maybe_new(npm_config).await?, -- cgit v1.2.3 From 2929d583d2f690b5a3d600296363a8ecd90440eb Mon Sep 17 00:00:00 2001 From: Satya Rohith Date: Wed, 16 Oct 2024 14:50:41 +0530 Subject: fix(cli): consolidate pkg parser for install & remove (#26298) Closes https://github.com/denoland/deno/issues/26283 --- cli/tools/registry/pm.rs | 84 ++++++++++++++++++++++++++++++------------------ 1 file changed, 52 insertions(+), 32 deletions(-) (limited to 'cli/tools/registry/pm.rs') diff --git a/cli/tools/registry/pm.rs b/cli/tools/registry/pm.rs index ff900d113..f716dd2ca 100644 --- a/cli/tools/registry/pm.rs +++ b/cli/tools/registry/pm.rs @@ -470,7 +470,7 @@ pub async fn add( 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(|| { + let req = AddRmPackageReq::parse(entry_text).with_context(|| { format!("Failed to parse package required: {}", entry_text) })?; @@ -596,10 +596,10 @@ enum PackageAndVersion { async fn find_package_and_select_version_for_req( jsr_resolver: Arc, npm_resolver: Arc, - add_package_req: AddPackageReq, + add_package_req: AddRmPackageReq, ) -> Result { match add_package_req.value { - AddPackageReqValue::Jsr(req) => { + AddRmPackageReqValue::Jsr(req) => { let jsr_prefixed_name = format!("jsr:{}", &req.name); let Some(nv) = jsr_resolver.req_to_nv(&req).await else { if npm_resolver.req_to_nv(&req).await.is_some() { @@ -628,7 +628,7 @@ async fn find_package_and_select_version_for_req( selected_version: nv.version.to_string(), })) } - AddPackageReqValue::Npm(req) => { + AddRmPackageReqValue::Npm(req) => { let npm_prefixed_name = format!("npm:{}", &req.name); let Some(nv) = npm_resolver.req_to_nv(&req).await else { return Ok(PackageAndVersion::NotFound { @@ -653,18 +653,18 @@ async fn find_package_and_select_version_for_req( } #[derive(Debug, PartialEq, Eq)] -enum AddPackageReqValue { +enum AddRmPackageReqValue { Jsr(PackageReq), Npm(PackageReq), } #[derive(Debug, PartialEq, Eq)] -struct AddPackageReq { +struct AddRmPackageReq { alias: String, - value: AddPackageReqValue, + value: AddRmPackageReqValue, } -impl AddPackageReq { +impl AddRmPackageReq { pub fn parse(entry_text: &str) -> Result, AnyError> { enum Prefix { Jsr, @@ -719,9 +719,9 @@ impl AddPackageReq { let req_ref = JsrPackageReqReference::from_str(&format!("jsr:{}", entry_text))?; let package_req = req_ref.into_inner().req; - Ok(Ok(AddPackageReq { + Ok(Ok(AddRmPackageReq { alias: maybe_alias.unwrap_or_else(|| package_req.name.to_string()), - value: AddPackageReqValue::Jsr(package_req), + value: AddRmPackageReqValue::Jsr(package_req), })) } Prefix::Npm => { @@ -739,9 +739,9 @@ impl AddPackageReq { deno_semver::RangeSetOrTag::Tag("latest".into()), ); } - Ok(Ok(AddPackageReq { + Ok(Ok(AddRmPackageReq { alias: maybe_alias.unwrap_or_else(|| package_req.name.to_string()), - value: AddPackageReqValue::Npm(package_req), + value: AddRmPackageReqValue::Npm(package_req), })) } } @@ -779,12 +779,28 @@ pub async fn remove( let mut removed_packages = vec![]; for package in &remove_flags.packages { - let mut removed = false; + let req = AddRmPackageReq::parse(package).with_context(|| { + format!("Failed to parse package required: {}", package) + })?; + let mut parsed_pkg_name = None; for config in configs.iter_mut().flatten() { - removed |= config.remove(package); + match &req { + Ok(rm_pkg) => { + if config.remove(&rm_pkg.alias) && parsed_pkg_name.is_none() { + parsed_pkg_name = Some(rm_pkg.alias.clone()); + } + } + Err(pkg) => { + // An alias or a package name without registry/version + // constraints. Try to remove the package anyway. + if config.remove(&pkg.name) && parsed_pkg_name.is_none() { + parsed_pkg_name = Some(pkg.name.clone()); + } + } + } } - if removed { - removed_packages.push(package.clone()); + if let Some(pkg) = parsed_pkg_name { + removed_packages.push(pkg); } } @@ -913,48 +929,52 @@ mod test { #[test] fn test_parse_add_package_req() { assert_eq!( - AddPackageReq::parse("jsr:foo").unwrap().unwrap(), - AddPackageReq { + AddRmPackageReq::parse("jsr:foo").unwrap().unwrap(), + AddRmPackageReq { alias: "foo".to_string(), - value: AddPackageReqValue::Jsr(PackageReq::from_str("foo").unwrap()) + value: AddRmPackageReqValue::Jsr(PackageReq::from_str("foo").unwrap()) } ); assert_eq!( - AddPackageReq::parse("alias@jsr:foo").unwrap().unwrap(), - AddPackageReq { + AddRmPackageReq::parse("alias@jsr:foo").unwrap().unwrap(), + AddRmPackageReq { alias: "alias".to_string(), - value: AddPackageReqValue::Jsr(PackageReq::from_str("foo").unwrap()) + value: AddRmPackageReqValue::Jsr(PackageReq::from_str("foo").unwrap()) } ); assert_eq!( - AddPackageReq::parse("@alias/pkg@npm:foo").unwrap().unwrap(), - AddPackageReq { + AddRmPackageReq::parse("@alias/pkg@npm:foo") + .unwrap() + .unwrap(), + AddRmPackageReq { alias: "@alias/pkg".to_string(), - value: AddPackageReqValue::Npm( + value: AddRmPackageReqValue::Npm( PackageReq::from_str("foo@latest").unwrap() ) } ); assert_eq!( - AddPackageReq::parse("@alias/pkg@jsr:foo").unwrap().unwrap(), - AddPackageReq { + AddRmPackageReq::parse("@alias/pkg@jsr:foo") + .unwrap() + .unwrap(), + AddRmPackageReq { alias: "@alias/pkg".to_string(), - value: AddPackageReqValue::Jsr(PackageReq::from_str("foo").unwrap()) + value: AddRmPackageReqValue::Jsr(PackageReq::from_str("foo").unwrap()) } ); assert_eq!( - AddPackageReq::parse("alias@jsr:foo@^1.5.0") + AddRmPackageReq::parse("alias@jsr:foo@^1.5.0") .unwrap() .unwrap(), - AddPackageReq { + AddRmPackageReq { alias: "alias".to_string(), - value: AddPackageReqValue::Jsr( + value: AddRmPackageReqValue::Jsr( PackageReq::from_str("foo@^1.5.0").unwrap() ) } ); assert_eq!( - AddPackageReq::parse("@scope/pkg@tag") + AddRmPackageReq::parse("@scope/pkg@tag") .unwrap() .unwrap_err() .to_string(), -- cgit v1.2.3 From e515f3dd0ea61bb3001e98ad7733ccb67c341f1e Mon Sep 17 00:00:00 2001 From: Marvin Hagemeister Date: Wed, 16 Oct 2024 18:34:33 +0200 Subject: fix(add): exact version should not have range `^` specifier (#26302) Fixes https://github.com/denoland/deno/issues/26299 --- cli/tools/registry/pm.rs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) (limited to 'cli/tools/registry/pm.rs') diff --git a/cli/tools/registry/pm.rs b/cli/tools/registry/pm.rs index f716dd2ca..02731d303 100644 --- a/cli/tools/registry/pm.rs +++ b/cli/tools/registry/pm.rs @@ -617,9 +617,11 @@ async fn find_package_and_select_version_for_req( }); }; let range_symbol = if req.version_req.version_text().starts_with('~') { - '~' + "~" + } else if req.version_req.version_text() == nv.version.to_string() { + "" } else { - '^' + "^" }; Ok(PackageAndVersion::Selected(SelectedPackage { import_name: add_package_req.alias, @@ -637,11 +639,15 @@ async fn find_package_and_select_version_for_req( package_req: req, }); }; + let range_symbol = if req.version_req.version_text().starts_with('~') { - '~' + "~" + } else if req.version_req.version_text() == nv.version.to_string() { + "" } else { - '^' + "^" }; + Ok(PackageAndVersion::Selected(SelectedPackage { import_name: add_package_req.alias, package_name: npm_prefixed_name, -- cgit v1.2.3 From 39fb55096ec12c8c18d15dff63a750a22169e3e6 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Mon, 21 Oct 2024 14:17:08 -0400 Subject: fix(install): better json editing (#26450) 1. Respects the formatting of the file (ex. keeps four space indents or tabs). 2. Handles editing of comments. 3. Handles trailing commas. 4. Code is easier to maintain. --- cli/tools/registry/pm.rs | 618 ++++++++++++++++------------------------------- 1 file changed, 208 insertions(+), 410 deletions(-) (limited to 'cli/tools/registry/pm.rs') diff --git a/cli/tools/registry/pm.rs b/cli/tools/registry/pm.rs index 02731d303..bd5290998 100644 --- a/cli/tools/registry/pm.rs +++ b/cli/tools/registry/pm.rs @@ -1,32 +1,22 @@ // Copyright 2018-2024 the Deno authors. All rights reserved. MIT license. -mod cache_deps; - -pub use cache_deps::cache_top_level_deps; -use deno_semver::jsr::JsrPackageReqReference; -use deno_semver::npm::NpmPackageReqReference; -use deno_semver::VersionReq; - -use std::borrow::Cow; use std::path::PathBuf; use std::sync::Arc; -use deno_ast::TextChange; -use deno_config::deno_json::FmtOptionsConfig; -use deno_core::anyhow::anyhow; use deno_core::anyhow::bail; use deno_core::anyhow::Context; use deno_core::error::AnyError; use deno_core::futures::FutureExt; use deno_core::futures::StreamExt; -use deno_core::serde_json; -use deno_core::ModuleSpecifier; -use deno_runtime::deno_node; +use deno_path_util::url_to_file_path; +use deno_semver::jsr::JsrPackageReqReference; +use deno_semver::npm::NpmPackageReqReference; use deno_semver::package::PackageReq; -use indexmap::IndexMap; -use jsonc_parser::ast::ObjectProp; -use jsonc_parser::ast::Value; -use yoke::Yoke; +use deno_semver::VersionReq; +use jsonc_parser::cst::CstObject; +use jsonc_parser::cst::CstObjectProp; +use jsonc_parser::cst::CstRootNode; +use jsonc_parser::json; use crate::args::AddFlags; use crate::args::CacheSetting; @@ -38,236 +28,181 @@ use crate::file_fetcher::FileFetcher; use crate::jsr::JsrFetchResolver; use crate::npm::NpmFetchResolver; -enum DenoConfigFormat { - Json, - Jsonc, -} +mod cache_deps; -impl DenoConfigFormat { - fn from_specifier(spec: &ModuleSpecifier) -> Result { - let file_name = spec - .path_segments() - .ok_or_else(|| anyhow!("Empty path in deno config specifier: {spec}"))? - .last() - .unwrap(); - match file_name { - "deno.json" => Ok(Self::Json), - "deno.jsonc" => Ok(Self::Jsonc), - _ => bail!("Unsupported deno config file: {file_name}"), - } - } -} +pub use cache_deps::cache_top_level_deps; -struct DenoConfig { - config: Arc, - format: DenoConfigFormat, - imports: IndexMap, +#[derive(Debug, Copy, Clone)] +enum ConfigKind { + DenoJson, + PackageJson, } -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(), - ) +struct ConfigUpdater { + kind: ConfigKind, + cst: CstRootNode, + root_object: CstObject, + path: PathBuf, + modified: bool, } -impl DenoConfig { - fn from_options(options: &CliOptions) -> Result, 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), - ); +impl ConfigUpdater { + fn new( + kind: ConfigKind, + config_file_path: PathBuf, + ) -> Result { + let config_file_contents = std::fs::read_to_string(&config_file_path) + .with_context(|| { + format!("Reading config file '{}'", config_file_path.display()) + })?; + let cst = CstRootNode::parse(&config_file_contents, &Default::default()) + .with_context(|| { + format!("Parsing config file '{}'", config_file_path.display()) + })?; + let root_object = cst.object_value_or_set(); + Ok(Self { + kind, + cst, + root_object, + path: config_file_path, + modified: false, + }) } - fn remove(&mut self, package: &str) -> bool { - self.imports.shift_remove(package).is_some() + fn display_path(&self) -> String { + deno_path_util::url_from_file_path(&self.path) + .map(|u| u.to_string()) + .unwrap_or_else(|_| self.path.display().to_string()) } - fn take_import_fields( - &mut self, - ) -> Vec<(&'static str, IndexMap)> { - vec![("imports", std::mem::take(&mut self.imports))] + fn obj(&self) -> &CstObject { + &self.root_object } -} -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 { - 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 contents(&self) -> String { + self.cst.to_string() } fn add(&mut self, selected: SelectedPackage, dev: bool) { - let (name, version) = package_json_dependency_entry(selected); - if dev { - self.dependencies.swap_remove(&name); - self.dev_dependencies.insert(name, version); - } else { - self.dev_dependencies.swap_remove(&name); - self.dependencies.insert(name, version); + fn insert_index(object: &CstObject, searching_name: &str) -> usize { + object + .properties() + .into_iter() + .take_while(|prop| { + let prop_name = + prop.name().and_then(|name| name.decoded_value().ok()); + match prop_name { + Some(current_name) => { + searching_name.cmp(¤t_name) == std::cmp::Ordering::Greater + } + None => true, + } + }) + .count() } - } - - 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 { - Deno(DenoConfig), - Npm(NpmConfig), -} - -impl From for DenoOrPackageJson { - fn from(config: DenoConfig) -> Self { - Self::Deno(config) - } -} - -impl From for DenoOrPackageJson { - fn from(config: NpmConfig) -> Self { - Self::Npm(config) - } -} + match self.kind { + ConfigKind::DenoJson => { + let imports = self.root_object.object_value_or_set("imports"); + let value = + format!("{}@{}", selected.package_name, selected.version_req); + if let Some(prop) = imports.get(&selected.import_name) { + prop.set_value(json!(value)); + } else { + let index = insert_index(&imports, &selected.import_name); + imports.insert(index, &selected.import_name, json!(value)); + } + } + ConfigKind::PackageJson => { + let deps_prop = self.root_object.get("dependencies"); + let dev_deps_prop = self.root_object.get("devDependencies"); + + let dependencies = if dev { + self + .root_object + .object_value("devDependencies") + .unwrap_or_else(|| { + let index = deps_prop + .as_ref() + .map(|p| p.property_index() + 1) + .unwrap_or_else(|| self.root_object.properties().len()); + self + .root_object + .insert(index, "devDependencies", json!({})) + .object_value_or_set() + }) + } else { + self + .root_object + .object_value("dependencies") + .unwrap_or_else(|| { + let index = dev_deps_prop + .as_ref() + .map(|p| p.property_index()) + .unwrap_or_else(|| self.root_object.properties().len()); + self + .root_object + .insert(index, "dependencies", json!({})) + .object_value_or_set() + }) + }; + let other_dependencies = if dev { + deps_prop.and_then(|p| p.value().and_then(|v| v.as_object())) + } else { + dev_deps_prop.and_then(|p| p.value().and_then(|v| v.as_object())) + }; -/// Wrapper around `jsonc_parser::ast::Object` that can be stored in a `Yoke` -#[derive(yoke::Yokeable)] -struct JsoncObjectView<'a>(jsonc_parser::ast::Object<'a>); + let (alias, value) = package_json_dependency_entry(selected); -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, String>, - path: PathBuf, - modified: bool, -} + if let Some(other) = other_dependencies { + if let Some(prop) = other.get(&alias) { + remove_prop_and_maybe_parent_prop(prop); + } + } -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>, - ) -> 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" { - 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 + if let Some(prop) = dependencies.get(&alias) { + prop.set_value(json!(value)); + } else { + let index = insert_index(&dependencies, &alias); + dependencies.insert(index, &alias, json!(value)); + } } - }; - 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), + let removed = match self.kind { + ConfigKind::DenoJson => { + if let Some(prop) = self + .root_object + .object_value("imports") + .and_then(|i| i.get(package)) + { + remove_prop_and_maybe_parent_prop(prop); + true + } else { + false + } + } + ConfigKind::PackageJson => { + let deps = [ + self + .root_object + .object_value("dependencies") + .and_then(|deps| deps.get(package)), + self + .root_object + .object_value("devDependencies") + .and_then(|deps| deps.get(package)), + ]; + let removed = deps.iter().any(|d| d.is_some()); + for dep in deps.into_iter().flatten() { + remove_prop_and_maybe_parent_prop(dep); + } + removed + } }; if removed { self.modified = true; @@ -275,76 +210,28 @@ impl ConfigUpdater { removed } - async fn commit(mut self) -> Result<(), AnyError> { + fn commit(&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?; + let new_text = self.contents(); + std::fs::write(&self.path, new_text).with_context(|| { + format!("failed writing to '{}'", self.path.display()) + })?; Ok(()) } } -impl DenoOrPackageJson { - fn specifier(&self) -> Cow { - match self { - 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.fmt_options.clone().unwrap_or_default() - } - } - } - - fn take_import_fields( - &mut self, - ) -> Vec<(&'static str, IndexMap)> { - match self { - Self::Deno(d) => d.take_import_fields(), - Self::Npm(n) => n.take_import_fields(), - } - } - - fn file_name(&self) -> &'static str { - match self { - DenoOrPackageJson::Deno(config) => match config.format { - DenoConfigFormat::Json => "deno.json", - DenoConfigFormat::Jsonc => "deno.jsonc", - }, - DenoOrPackageJson::Npm(..) => "package.json", - } +fn remove_prop_and_maybe_parent_prop(prop: CstObjectProp) { + let parent = prop.parent().unwrap().as_object().unwrap(); + prop.remove(); + if parent.properties().is_empty() { + let parent_property = parent.parent().unwrap(); + let root_object = parent_property.parent().unwrap().as_object().unwrap(); + // remove the property + parent_property.remove(); + root_object.ensure_multiline(); } } @@ -401,11 +288,27 @@ impl std::fmt::Display for AddCommandName { fn load_configs( flags: &Arc, has_jsr_specifiers: impl FnOnce() -> bool, -) -> Result<(CliFactory, Option, Option), AnyError> { +) -> 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)?; - let (cli_factory, deno_config) = match DenoConfig::from_options(options)? { + let start_dir = &options.start_dir; + let npm_config = match start_dir.maybe_pkg_json() { + Some(pkg_json) => Some(ConfigUpdater::new( + ConfigKind::PackageJson, + pkg_json.path.clone(), + )?), + None => None, + }; + let deno_config = match start_dir.maybe_deno_json() { + Some(deno_json) => Some(ConfigUpdater::new( + ConfigKind::DenoJson, + url_to_file_path(&deno_json.specifier)?, + )?), + None => None, + }; + + let (cli_factory, deno_config) = match deno_config { Some(config) => (cli_factory, Some(config)), None if npm_config.is_some() && !has_jsr_specifiers() => { (cli_factory, None) @@ -413,11 +316,16 @@ fn load_configs( _ => { let factory = create_deno_json(flags, options)?; let options = factory.cli_options()?.clone(); + let deno_json = options + .start_dir + .maybe_deno_json() + .expect("Just created deno.json"); ( factory, - Some( - DenoConfig::from_options(&options)?.expect("Just created deno.json"), - ), + Some(ConfigUpdater::new( + ConfigKind::DenoJson, + url_to_file_path(&deno_json.specifier)?, + )?), ) } }; @@ -430,15 +338,13 @@ pub async fn add( add_flags: AddFlags, cmd_name: AddCommandName, ) -> Result<(), AnyError> { - let (cli_factory, npm_config, deno_config) = load_configs(&flags, || { - add_flags.packages.iter().any(|s| s.starts_with("jsr:")) - })?; - let mut npm_config = ConfigUpdater::maybe_new(npm_config).await?; - let mut deno_config = ConfigUpdater::maybe_new(deno_config).await?; + let (cli_factory, mut npm_config, mut deno_config) = + load_configs(&flags, || { + add_flags.packages.iter().any(|s| s.starts_with("jsr:")) + })?; if let Some(deno) = &deno_config { - let specifier = deno.config.specifier(); - if deno.obj().get_string("importMap").is_some() { + if deno.obj().get("importMap").is_some() { bail!( concat!( "`deno {}` is not supported when configuration file contains an \"importMap\" field. ", @@ -446,7 +352,7 @@ pub async fn add( " at {}", ), cmd_name, - specifier + deno.display_path(), ); } } @@ -558,18 +464,11 @@ pub async fn add( } } - let mut commit_futures = vec![]; if let Some(npm) = npm_config { - commit_futures.push(npm.commit()); + 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; - - for result in commit_futures { - result.context("Failed to update configuration file")?; + deno.commit()?; } npm_install_after_modification(flags, Some(jsr_resolver)).await?; @@ -754,33 +653,13 @@ impl AddRmPackageReq { } } -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() { - if index == 0 { - contents.push(String::new()); // force a newline at the start - } - // TODO(bartlomieju): fix it, once we start support specifying version on the cli - contents.push(format!("\"{}\": \"{}\"", package, version)); - if index != len - 1 { - contents.push(",".to_string()); - } - } - contents.join("\n") -} - pub async fn remove( flags: Arc, remove_flags: RemoveFlags, ) -> Result<(), AnyError> { let (_, npm_config, deno_config) = load_configs(&flags, || false)?; - let mut configs = [ - ConfigUpdater::maybe_new(npm_config).await?, - ConfigUpdater::maybe_new(deno_config).await?, - ]; + let mut configs = [npm_config, deno_config]; let mut removed_packages = vec![]; @@ -817,7 +696,7 @@ pub async fn remove( log::info!("Removed {}", crate::colors::green(package)); } for config in configs.into_iter().flatten() { - config.commit().await?; + config.commit()?; } npm_install_after_modification(flags, None).await?; @@ -847,87 +726,6 @@ async fn npm_install_after_modification( Ok(()) } -fn update_config_file_content< - I: IntoIterator)>, ->( - obj: &jsonc_parser::ast::Object, - config_file_contents: &str, - fmt_options: FmtOptionsConfig, - entries: I, - file_name: &str, -) -> String { - let mut text_changes = vec![]; - 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, - .. - }, - )) => { - 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 { - // 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, - // 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!(), - } - } - - let new_text = - deno_ast::apply_text_changes(config_file_contents, text_changes); - - crate::tools::fmt::format_json( - &PathBuf::from(file_name), - &new_text, - &fmt_options, - ) - .ok() - .map(|formatted_text| formatted_text.unwrap_or_else(|| new_text.clone())) - .unwrap_or(new_text) -} - #[cfg(test)] mod test { use super::*; -- cgit v1.2.3 From 67280f8b558902729c805fd3d8f26d4c434fc211 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartek=20Iwa=C5=84czuk?= Date: Tue, 22 Oct 2024 00:08:45 +0100 Subject: fix(install): update lockfile when using package.json (#26458) This commit makes sure that `deno add`, `deno install` and `deno remove` update the lockfile if only `package.json` file is present. Fixes https://github.com/denoland/deno/issues/26270 --- cli/tools/registry/pm.rs | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'cli/tools/registry/pm.rs') diff --git a/cli/tools/registry/pm.rs b/cli/tools/registry/pm.rs index bd5290998..2060b9a13 100644 --- a/cli/tools/registry/pm.rs +++ b/cli/tools/registry/pm.rs @@ -723,6 +723,10 @@ async fn npm_install_after_modification( // npm install cache_deps::cache_top_level_deps(&cli_factory, jsr_resolver).await?; + if let Some(lockfile) = cli_factory.cli_options()?.maybe_lockfile() { + lockfile.write_if_changed()?; + } + Ok(()) } -- cgit v1.2.3 From 5f0bb3c6f4328003012e98ba70ce18e4e2e842de Mon Sep 17 00:00:00 2001 From: Marvin Hagemeister Date: Thu, 24 Oct 2024 20:03:56 +0200 Subject: fix: `.npmrc` settings not being passed to install/add command (#26473) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We weren't passing the resolved npmrc settings to the install commands. This lead us to always fall back to the default registry url instead of using the one from npmrc. Fixes https://github.com/denoland/deno/issues/26139 Fixes https://github.com/denoland/deno/issues/26033 Fixes https://github.com/denoland/deno/issues/25924 Fixes https://github.com/denoland/deno/issues/25822 Fixes https://github.com/denoland/deno/issues/26152 --------- Co-authored-by: Bartek Iwańczuk --- cli/tools/registry/pm.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) (limited to 'cli/tools/registry/pm.rs') diff --git a/cli/tools/registry/pm.rs b/cli/tools/registry/pm.rs index 2060b9a13..d1be901d6 100644 --- a/cli/tools/registry/pm.rs +++ b/cli/tools/registry/pm.rs @@ -367,10 +367,14 @@ pub async fn add( Default::default(), None, ); + + let npmrc = cli_factory.cli_options().unwrap().npmrc(); + 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 npm_resolver = + Arc::new(NpmFetchResolver::new(deps_file_fetcher, npmrc.clone())); let mut selected_packages = Vec::with_capacity(add_flags.packages.len()); let mut package_reqs = Vec::with_capacity(add_flags.packages.len()); -- cgit v1.2.3 From 2c8a0e791732ab00907ca11c3a4918ad26ead03f Mon Sep 17 00:00:00 2001 From: Nathan Whitaker <17734409+nathanwhit@users.noreply.github.com> Date: Fri, 1 Nov 2024 19:10:35 -0700 Subject: fix(add): only add npm deps to package.json if it's at least as close as deno.json (#26683) Fixes https://github.com/denoland/deno/issues/26653 --- cli/tools/registry/pm.rs | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) (limited to 'cli/tools/registry/pm.rs') diff --git a/cli/tools/registry/pm.rs b/cli/tools/registry/pm.rs index d1be901d6..4e0387998 100644 --- a/cli/tools/registry/pm.rs +++ b/cli/tools/registry/pm.rs @@ -1,5 +1,6 @@ // Copyright 2018-2024 the Deno authors. All rights reserved. MIT license. +use std::path::Path; use std::path::PathBuf; use std::sync::Arc; @@ -333,6 +334,14 @@ fn load_configs( Ok((cli_factory, npm_config, deno_config)) } +fn path_distance(a: &Path, b: &Path) -> usize { + let diff = pathdiff::diff_paths(a, b); + let Some(diff) = diff else { + return usize::MAX; + }; + diff.components().count() +} + pub async fn add( flags: Arc, add_flags: AddFlags, @@ -357,6 +366,21 @@ pub async fn add( } } + let start_dir = cli_factory.cli_options()?.start_dir.dir_path(); + + // only prefer to add npm deps to `package.json` if there isn't a closer deno.json. + // example: if deno.json is in the CWD and package.json is in the parent, we should add + // npm deps to deno.json, since it's closer + let prefer_npm_config = match (npm_config.as_ref(), deno_config.as_ref()) { + (Some(npm), Some(deno)) => { + let npm_distance = path_distance(&npm.path, &start_dir); + let deno_distance = path_distance(&deno.path, &start_dir); + npm_distance <= deno_distance + } + (Some(_), None) => true, + (None, _) => false, + }; + let http_client = cli_factory.http_client_provider(); let deps_http_cache = cli_factory.global_http_cache()?; let mut deps_file_fetcher = FileFetcher::new( @@ -455,7 +479,7 @@ pub async fn add( selected_package.selected_version ); - if selected_package.package_name.starts_with("npm:") { + if selected_package.package_name.starts_with("npm:") && prefer_npm_config { if let Some(npm) = &mut npm_config { npm.add(selected_package, dev); } else { -- cgit v1.2.3 From 706b1dfcea8ab6bf7d155893ab795669107516a8 Mon Sep 17 00:00:00 2001 From: Nathan Whitaker <17734409+nathanwhit@users.noreply.github.com> Date: Mon, 4 Nov 2024 18:45:00 -0800 Subject: fix(add): better error message when adding package that only has pre-release versions (#26724) Fixes https://github.com/denoland/deno/issues/26597 A small refactor as well to reduce some code duplication --- cli/tools/registry/pm.rs | 205 +++++++++++++++++++++++++++++++++-------------- 1 file changed, 146 insertions(+), 59 deletions(-) (limited to 'cli/tools/registry/pm.rs') diff --git a/cli/tools/registry/pm.rs b/cli/tools/registry/pm.rs index 4e0387998..68913e259 100644 --- a/cli/tools/registry/pm.rs +++ b/cli/tools/registry/pm.rs @@ -12,7 +12,9 @@ use deno_core::futures::StreamExt; use deno_path_util::url_to_file_path; use deno_semver::jsr::JsrPackageReqReference; use deno_semver::npm::NpmPackageReqReference; +use deno_semver::package::PackageNv; use deno_semver::package::PackageReq; +use deno_semver::Version; use deno_semver::VersionReq; use jsonc_parser::cst::CstObject; use jsonc_parser::cst::CstObjectProp; @@ -455,15 +457,32 @@ pub async fn add( match package_and_version { PackageAndVersion::NotFound { package: package_name, - found_npm_package, + help, 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)); + } => match help { + Some(NotFoundHelp::NpmPackage) => { + 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}")) + ); } - } + Some(NotFoundHelp::JsrPackage) => { + bail!( + "{} was not found, but a matching jsr package exists. Did you mean `{}`?", + crate::colors::red(package_name), + crate::colors::yellow(format!("deno {cmd_name} jsr:{package_req}")) + ) + } + Some(NotFoundHelp::PreReleaseVersion(version)) => { + bail!( + "{} has only pre-release versions available. Try specifying a version: `{}`", + crate::colors::red(&package_name), + crate::colors::yellow(format!("deno {cmd_name} {package_name}@^{version}")) + ) + } + None => bail!("{} was not found.", crate::colors::red(package_name)), + }, PackageAndVersion::Selected(selected) => { selected_packages.push(selected); } @@ -511,76 +530,144 @@ struct SelectedPackage { selected_version: String, } +enum NotFoundHelp { + NpmPackage, + JsrPackage, + PreReleaseVersion(Version), +} + enum PackageAndVersion { NotFound { package: String, - found_npm_package: bool, package_req: PackageReq, + help: Option, }, Selected(SelectedPackage), } +fn best_version<'a>( + versions: impl Iterator, +) -> Option<&'a Version> { + let mut maybe_best_version: Option<&Version> = None; + for version in versions { + let is_best_version = maybe_best_version + .as_ref() + .map(|best_version| (*best_version).cmp(version).is_lt()) + .unwrap_or(true); + if is_best_version { + maybe_best_version = Some(version); + } + } + maybe_best_version +} + +trait PackageInfoProvider { + const SPECIFIER_PREFIX: &str; + /// The help to return if a package is found by this provider + const HELP: NotFoundHelp; + async fn req_to_nv(&self, req: &PackageReq) -> Option; + async fn latest_version<'a>(&self, req: &PackageReq) -> Option; +} + +impl PackageInfoProvider for Arc { + const HELP: NotFoundHelp = NotFoundHelp::JsrPackage; + const SPECIFIER_PREFIX: &str = "jsr"; + async fn req_to_nv(&self, req: &PackageReq) -> Option { + (**self).req_to_nv(req).await + } + + async fn latest_version<'a>(&self, req: &PackageReq) -> Option { + let info = self.package_info(&req.name).await?; + best_version( + info + .versions + .iter() + .filter(|(_, version_info)| !version_info.yanked) + .map(|(version, _)| version), + ) + .cloned() + } +} + +impl PackageInfoProvider for Arc { + const HELP: NotFoundHelp = NotFoundHelp::NpmPackage; + const SPECIFIER_PREFIX: &str = "npm"; + async fn req_to_nv(&self, req: &PackageReq) -> Option { + (**self).req_to_nv(req).await + } + + async fn latest_version<'a>(&self, req: &PackageReq) -> Option { + let info = self.package_info(&req.name).await?; + best_version(info.versions.keys()).cloned() + } +} + async fn find_package_and_select_version_for_req( jsr_resolver: Arc, npm_resolver: Arc, add_package_req: AddRmPackageReq, ) -> Result { - match add_package_req.value { - AddRmPackageReqValue::Jsr(req) => { - let jsr_prefixed_name = format!("jsr:{}", &req.name); - let Some(nv) = jsr_resolver.req_to_nv(&req).await else { - if npm_resolver.req_to_nv(&req).await.is_some() { + async fn select( + main_resolver: T, + fallback_resolver: S, + add_package_req: AddRmPackageReq, + ) -> Result { + let req = match &add_package_req.value { + AddRmPackageReqValue::Jsr(req) => req, + AddRmPackageReqValue::Npm(req) => req, + }; + let prefixed_name = format!("{}:{}", T::SPECIFIER_PREFIX, req.name); + let help_if_found_in_fallback = S::HELP; + let Some(nv) = main_resolver.req_to_nv(req).await else { + if fallback_resolver.req_to_nv(req).await.is_some() { + // it's in the other registry + return Ok(PackageAndVersion::NotFound { + package: prefixed_name, + help: Some(help_if_found_in_fallback), + package_req: req.clone(), + }); + } + if req.version_req.version_text() == "*" { + if let Some(pre_release_version) = + main_resolver.latest_version(req).await + { return Ok(PackageAndVersion::NotFound { - package: jsr_prefixed_name, - found_npm_package: true, - package_req: req, + package: prefixed_name, + package_req: req.clone(), + help: Some(NotFoundHelp::PreReleaseVersion( + pre_release_version.clone(), + )), }); } + } - return Ok(PackageAndVersion::NotFound { - package: jsr_prefixed_name, - found_npm_package: false, - package_req: req, - }); - }; - let range_symbol = if req.version_req.version_text().starts_with('~') { - "~" - } else if req.version_req.version_text() == nv.version.to_string() { - "" - } else { - "^" - }; - Ok(PackageAndVersion::Selected(SelectedPackage { - import_name: add_package_req.alias, - package_name: jsr_prefixed_name, - version_req: format!("{}{}", range_symbol, &nv.version), - selected_version: nv.version.to_string(), - })) - } - AddRmPackageReqValue::Npm(req) => { - let npm_prefixed_name = format!("npm:{}", &req.name); - let Some(nv) = npm_resolver.req_to_nv(&req).await else { - return Ok(PackageAndVersion::NotFound { - package: npm_prefixed_name, - found_npm_package: false, - package_req: req, - }); - }; + return Ok(PackageAndVersion::NotFound { + package: prefixed_name, + help: None, + package_req: req.clone(), + }); + }; + let range_symbol = if req.version_req.version_text().starts_with('~') { + "~" + } else if req.version_req.version_text() == nv.version.to_string() { + "" + } else { + "^" + }; + Ok(PackageAndVersion::Selected(SelectedPackage { + import_name: add_package_req.alias, + package_name: prefixed_name, + version_req: format!("{}{}", range_symbol, &nv.version), + selected_version: nv.version.to_string(), + })) + } - let range_symbol = if req.version_req.version_text().starts_with('~') { - "~" - } else if req.version_req.version_text() == nv.version.to_string() { - "" - } else { - "^" - }; - - Ok(PackageAndVersion::Selected(SelectedPackage { - import_name: add_package_req.alias, - package_name: npm_prefixed_name, - version_req: format!("{}{}", range_symbol, &nv.version), - selected_version: nv.version.to_string(), - })) + match &add_package_req.value { + AddRmPackageReqValue::Jsr(_) => { + select(jsr_resolver, npm_resolver, add_package_req).await + } + AddRmPackageReqValue::Npm(_) => { + select(npm_resolver, jsr_resolver, add_package_req).await } } } -- cgit v1.2.3 From 99d5c6e423fe67f7f062296ffd38b38f92b7ab70 Mon Sep 17 00:00:00 2001 From: Miguel Rodrigues <64497525+mbrdg@users.noreply.github.com> Date: Sat, 16 Nov 2024 13:57:14 +0000 Subject: fix(cli): show prefix hint when installing a package globally (#26629) Closes #26545 Shows a hint when a package is installed globally, otherwise fallbacks to the existing implementation. --- cli/tools/registry/pm.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'cli/tools/registry/pm.rs') diff --git a/cli/tools/registry/pm.rs b/cli/tools/registry/pm.rs index 68913e259..c1ea2c75e 100644 --- a/cli/tools/registry/pm.rs +++ b/cli/tools/registry/pm.rs @@ -679,7 +679,7 @@ enum AddRmPackageReqValue { } #[derive(Debug, PartialEq, Eq)] -struct AddRmPackageReq { +pub struct AddRmPackageReq { alias: String, value: AddRmPackageReqValue, } -- cgit v1.2.3