From cc406c8360b4ba559d7f13e14d2a32e1ab761b0d Mon Sep 17 00:00:00 2001 From: David Sherret Date: Fri, 19 May 2023 18:39:27 -0400 Subject: [PATCH] feat(vendor): support for npm specifiers (#19186) We never properly added support for this. This fixes vendoring when it has npm or node specifiers. Vendoring occurs by adding a `"nodeModulesDir": true` property to deno.json then it uses a local node_modules directory. This can be opted out by setting `"nodeModulesDir": false` or running with `--node-modules-dir=false`. Closes #18090 Closes #17210 Closes #17619 Closes #16778 --- cli/args/flags.rs | 14 +- cli/args/mod.rs | 9 + cli/factory.rs | 18 + cli/npm/mod.rs | 1 + cli/npm/resolvers/mod.rs | 3 +- cli/tests/integration/vendor_tests.rs | 184 ++++++++-- .../testdata/vendor/npm_and_node_specifier.ts | 2 + cli/tools/vendor/import_map.rs | 2 +- cli/tools/vendor/mod.rs | 334 ++++++++++++++---- cli/tools/vendor/specifiers.rs | 2 +- test_util/src/temp_dir.rs | 4 + 11 files changed, 463 insertions(+), 110 deletions(-) create mode 100644 cli/tests/testdata/vendor/npm_and_node_specifier.ts diff --git a/cli/args/flags.rs b/cli/args/flags.rs index 63ec2b9471..c4d8a3f87e 100644 --- a/cli/args/flags.rs +++ b/cli/args/flags.rs @@ -1343,7 +1343,7 @@ TypeScript compiler cache: Subdirectory containing TS compiler output.", .arg(lock_arg()) .arg(config_arg()) .arg(import_map_arg()) - .arg(local_npm_arg()) + .arg(node_modules_dir_arg()) .arg( Arg::new("json") .long("json") @@ -1862,6 +1862,7 @@ Remote modules and multiple modules may also be specified: .arg(config_arg()) .arg(import_map_arg()) .arg(lock_arg()) + .arg(node_modules_dir_arg()) .arg(reload_arg()) .arg(ca_file_arg()) } @@ -1875,7 +1876,7 @@ fn compile_args_without_check_args(app: Command) -> Command { .arg(import_map_arg()) .arg(no_remote_arg()) .arg(no_npm_arg()) - .arg(local_npm_arg()) + .arg(node_modules_dir_arg()) .arg(config_arg()) .arg(no_config_arg()) .arg(reload_arg()) @@ -2424,7 +2425,7 @@ fn no_npm_arg() -> Arg { .help("Do not resolve npm modules") } -fn local_npm_arg() -> Arg { +fn node_modules_dir_arg() -> Arg { Arg::new("node-modules-dir") .long("node-modules-dir") .num_args(0..=1) @@ -2719,7 +2720,7 @@ fn info_parse(flags: &mut Flags, matches: &mut ArgMatches) { import_map_arg_parse(flags, matches); location_arg_parse(flags, matches); ca_file_arg_parse(flags, matches); - local_npm_args_parse(flags, matches); + node_modules_dir_arg_parse(flags, matches); lock_arg_parse(flags, matches); no_lock_arg_parse(flags, matches); no_remote_arg_parse(flags, matches); @@ -2975,6 +2976,7 @@ fn vendor_parse(flags: &mut Flags, matches: &mut ArgMatches) { config_args_parse(flags, matches); import_map_arg_parse(flags, matches); lock_arg_parse(flags, matches); + node_modules_dir_arg_parse(flags, matches); reload_arg_parse(flags, matches); flags.subcommand = DenoSubcommand::Vendor(VendorFlags { @@ -3000,7 +3002,7 @@ fn compile_args_without_check_parse( import_map_arg_parse(flags, matches); no_remote_arg_parse(flags, matches); no_npm_arg_parse(flags, matches); - local_npm_args_parse(flags, matches); + node_modules_dir_arg_parse(flags, matches); config_args_parse(flags, matches); reload_arg_parse(flags, matches); lock_args_parse(flags, matches); @@ -3254,7 +3256,7 @@ fn no_npm_arg_parse(flags: &mut Flags, matches: &mut ArgMatches) { } } -fn local_npm_args_parse(flags: &mut Flags, matches: &mut ArgMatches) { +fn node_modules_dir_arg_parse(flags: &mut Flags, matches: &mut ArgMatches) { flags.node_modules_dir = matches.remove_one::("node-modules-dir"); } diff --git a/cli/args/mod.rs b/cli/args/mod.rs index fef27f464d..fdef509839 100644 --- a/cli/args/mod.rs +++ b/cli/args/mod.rs @@ -881,6 +881,15 @@ impl CliOptions { self.maybe_node_modules_folder.clone() } + pub fn node_modules_dir_enablement(&self) -> Option { + self.flags.node_modules_dir.or_else(|| { + self + .maybe_config_file + .as_ref() + .and_then(|c| c.node_modules_dir()) + }) + } + pub fn node_modules_dir_specifier(&self) -> Option { self .maybe_node_modules_folder diff --git a/cli/factory.rs b/cli/factory.rs index 0c85536c4c..c64268ce37 100644 --- a/cli/factory.rs +++ b/cli/factory.rs @@ -29,6 +29,7 @@ use crate::npm::create_npm_fs_resolver; use crate::npm::CliNpmRegistryApi; use crate::npm::CliNpmResolver; use crate::npm::NpmCache; +use crate::npm::NpmPackageFsResolver; use crate::npm::NpmResolution; use crate::npm::PackageJsonDepsInstaller; use crate::resolver::CliGraphResolver; @@ -325,6 +326,23 @@ impl CliFactory { .await } + pub async fn create_node_modules_npm_fs_resolver( + &self, + node_modules_dir_path: PathBuf, + ) -> Result, AnyError> { + Ok(create_npm_fs_resolver( + self.fs().clone(), + self.npm_cache()?.clone(), + self.text_only_progress_bar(), + CliNpmRegistryApi::default_url().to_owned(), + self.npm_resolution().await?.clone(), + // when an explicit path is provided here, it will create the + // local node_modules variant of an npm fs resolver + Some(node_modules_dir_path), + self.options.npm_system_info(), + )) + } + pub fn package_json_deps_provider(&self) -> &Arc { self.services.package_json_deps_provider.get_or_init(|| { Arc::new(PackageJsonDepsProvider::new( diff --git a/cli/npm/mod.rs b/cli/npm/mod.rs index 488f8eae6a..5f875c7439 100644 --- a/cli/npm/mod.rs +++ b/cli/npm/mod.rs @@ -14,4 +14,5 @@ pub use registry::CliNpmRegistryApi; pub use resolution::NpmResolution; pub use resolvers::create_npm_fs_resolver; pub use resolvers::CliNpmResolver; +pub use resolvers::NpmPackageFsResolver; pub use resolvers::NpmProcessState; diff --git a/cli/npm/resolvers/mod.rs b/cli/npm/resolvers/mod.rs index a41727ddac..0f123c3829 100644 --- a/cli/npm/resolvers/mod.rs +++ b/cli/npm/resolvers/mod.rs @@ -36,11 +36,12 @@ use crate::args::Lockfile; use crate::util::fs::canonicalize_path_maybe_not_exists_with_fs; use crate::util::progress_bar::ProgressBar; -use self::common::NpmPackageFsResolver; use self::local::LocalNpmPackageResolver; use super::resolution::NpmResolution; use super::NpmCache; +pub use self::common::NpmPackageFsResolver; + /// State provided to the process via an environment variable. #[derive(Clone, Debug, Serialize, Deserialize)] pub struct NpmProcessState { diff --git a/cli/tests/integration/vendor_tests.rs b/cli/tests/integration/vendor_tests.rs index 1f159fe802..94ab2ad5ae 100644 --- a/cli/tests/integration/vendor_tests.rs +++ b/cli/tests/integration/vendor_tests.rs @@ -10,6 +10,7 @@ use test_util as util; use test_util::TempDir; use util::http_server; use util::new_deno_dir; +use util::TestContextBuilder; #[test] fn output_dir_exists() { @@ -186,15 +187,13 @@ fn import_map_output_dir() { String::from_utf8_lossy(&output.stderr).trim(), format!( concat!( - "Ignoring import map. Specifying an import map file ({}) in the deno ", - "vendor output directory is not supported. If you wish to use an ", - "import map while vendoring, please specify one located outside this ", - "directory.\n", + "{}\n", "Download http://localhost:4545/vendor/logger.ts\n", - "{}", + "{}\n\n{}", ), - PathBuf::from("vendor").join("import_map.json").display(), - success_text_updated_deno_json("1 module", "vendor/"), + ignoring_import_map_text(), + vendored_text("1 module", "vendor/"), + success_text_updated_deno_json("vendor/"), ) ); assert!(output.status.success()); @@ -511,8 +510,9 @@ fn update_existing_config_test() { assert_eq!( String::from_utf8_lossy(&output.stderr).trim(), format!( - "Download http://localhost:4545/vendor/logger.ts\n{}", - success_text_updated_deno_json("1 module", "vendor2",) + "Download http://localhost:4545/vendor/logger.ts\n{}\n\n{}", + vendored_text("1 module", "vendor2"), + success_text_updated_deno_json("vendor2",) ) ); assert_eq!(String::from_utf8_lossy(&output.stdout).trim(), ""); @@ -537,38 +537,158 @@ fn update_existing_config_test() { assert!(output.status.success()); } +#[test] +fn vendor_npm_node_specifiers() { + let context = TestContextBuilder::for_npm().use_temp_cwd().build(); + let temp_dir = context.temp_dir(); + temp_dir.write( + "my_app.ts", + concat!( + "import { path, getValue, setValue } from 'http://localhost:4545/vendor/npm_and_node_specifier.ts';\n", + "setValue(5);\n", + "console.log(path.isAbsolute(Deno.cwd()), getValue());", + ), + ); + temp_dir.write("deno.json", "{}"); + + let output = context.new_command().args("vendor my_app.ts").run(); + output.assert_matches_text( + format!( + concat!( + "Download http://localhost:4545/vendor/npm_and_node_specifier.ts\n", + "Download http://localhost:4545/npm/registry/@denotest/esm-basic\n", + "Download http://localhost:4545/npm/registry/@denotest/esm-basic/1.0.0.tgz\n", + "{}\n", + "Initialize @denotest/esm-basic@1.0.0\n", + "{}\n\n", + "{}\n", + ), + vendored_text("1 module", "vendor/"), + vendored_npm_package_text("1 npm package"), + success_text_updated_deno_json("vendor/") + ) + ); + let output = context.new_command().args("run -A my_app.ts").run(); + output.assert_matches_text("true 5\n"); + assert!(temp_dir.path().join("node_modules").exists()); + assert!(temp_dir.path().join("deno.lock").exists()); + + // now try re-vendoring with a lockfile + let output = context.new_command().args("vendor --force my_app.ts").run(); + output.assert_matches_text(format!( + "{}\n{}\n\n{}\n", + ignoring_import_map_text(), + vendored_text("1 module", "vendor/"), + success_text_updated_deno_json("vendor/"), + )); + + // delete the node_modules folder + temp_dir.remove_dir_all("node_modules"); + + // vendor with --node-modules-dir=false + let output = context + .new_command() + .args("vendor --node-modules-dir=false --force my_app.ts") + .run(); + output.assert_matches_text(format!( + "{}\n{}\n\n{}\n", + ignoring_import_map_text(), + vendored_text("1 module", "vendor/"), + success_text_updated_deno_json("vendor/") + )); + assert!(!temp_dir.path().join("node_modules").exists()); + + // delete the deno.json + temp_dir.remove_file("deno.json"); + + // vendor with --node-modules-dir + let output = context + .new_command() + .args("vendor --node-modules-dir --force my_app.ts") + .run(); + output.assert_matches_text(format!( + "Initialize @denotest/esm-basic@1.0.0\n{}\n\n{}\n", + vendored_text("1 module", "vendor/"), + use_import_map_text("vendor/") + )); +} + +#[test] +fn vendor_only_npm_specifiers() { + let context = TestContextBuilder::for_npm().use_temp_cwd().build(); + let temp_dir = context.temp_dir(); + temp_dir.write( + "my_app.ts", + concat!( + "import { getValue, setValue } from 'npm:@denotest/esm-basic';\n", + "setValue(5);\n", + "console.log(path.isAbsolute(Deno.cwd()), getValue());", + ), + ); + temp_dir.write("deno.json", "{}"); + + let output = context.new_command().args("vendor my_app.ts").run(); + output.assert_matches_text( + format!( + concat!( + "Download http://localhost:4545/npm/registry/@denotest/esm-basic\n", + "Download http://localhost:4545/npm/registry/@denotest/esm-basic/1.0.0.tgz\n", + "{}\n", + "Initialize @denotest/esm-basic@1.0.0\n", + "{}\n", + ), + vendored_text("0 modules", "vendor/"), + vendored_npm_package_text("1 npm package"), + ) + ); +} + fn success_text(module_count: &str, dir: &str, has_import_map: bool) -> String { let mut text = format!("Vendored {module_count} into {dir} directory."); if has_import_map { - let f = format!( - concat!( - "\n\nTo use vendored modules, specify the `--import-map {}import_map.json` flag when ", - r#"invoking Deno subcommands or add an `"importMap": ""` "#, - "entry to a deno.json file.", - ), - if dir != "vendor/" { - format!("{}{}", dir.trim_end_matches('/'), if cfg!(windows) { '\\' } else {'/'}) - } else { - dir.to_string() - } - ); - write!(text, "{f}").unwrap(); + write!(text, "\n\n{}", use_import_map_text(dir)).unwrap(); } text } -fn success_text_updated_deno_json(module_count: &str, dir: &str) -> String { +fn use_import_map_text(dir: &str) -> String { + format!( + concat!( + "To use vendored modules, specify the `--import-map {}import_map.json` flag when ", + r#"invoking Deno subcommands or add an `"importMap": ""` "#, + "entry to a deno.json file.", + ), + if dir != "vendor/" { + format!("{}{}", dir.trim_end_matches('/'), if cfg!(windows) { '\\' } else {'/'}) + } else { + dir.to_string() + } + ) +} + +fn vendored_text(module_count: &str, dir: &str) -> String { + format!("Vendored {} into {} directory.", module_count, dir) +} + +fn vendored_npm_package_text(package_count: &str) -> String { + format!( + concat!( + "Vendored {} into node_modules directory. Set `nodeModulesDir: false` ", + "in the Deno configuration file to disable vendoring npm packages in the future.", + ), + package_count + ) +} + +fn success_text_updated_deno_json(dir: &str) -> String { format!( concat!( - "Vendored {} into {} directory.\n\n", "Updated your local Deno configuration file with a reference to the ", "new vendored import map at {}import_map.json. Invoking Deno subcommands will ", "now automatically resolve using the vendored modules. You may override ", "this by providing the `--import-map ` flag or by ", "manually editing your Deno configuration file.", ), - module_count, - dir, if dir != "vendor/" { format!( "{}{}", @@ -580,3 +700,15 @@ fn success_text_updated_deno_json(module_count: &str, dir: &str) -> String { } ) } + +fn ignoring_import_map_text() -> String { + format!( + concat!( + "Ignoring import map. Specifying an import map file ({}) in the deno ", + "vendor output directory is not supported. If you wish to use an ", + "import map while vendoring, please specify one located outside this ", + "directory.", + ), + PathBuf::from("vendor").join("import_map.json").display(), + ) +} diff --git a/cli/tests/testdata/vendor/npm_and_node_specifier.ts b/cli/tests/testdata/vendor/npm_and_node_specifier.ts new file mode 100644 index 0000000000..61962e836b --- /dev/null +++ b/cli/tests/testdata/vendor/npm_and_node_specifier.ts @@ -0,0 +1,2 @@ +export { default as path } from "node:path"; +export { getValue, setValue } from "npm:@denotest/esm-basic"; diff --git a/cli/tools/vendor/import_map.rs b/cli/tools/vendor/import_map.rs index 562ae0216d..dbda81a3a0 100644 --- a/cli/tools/vendor/import_map.rs +++ b/cli/tools/vendor/import_map.rs @@ -304,7 +304,7 @@ fn handle_dep_specifier( referrer, mappings, ) - } else { + } else if specifier.scheme() == "file" { handle_local_dep_specifier( text, unresolved_specifier, diff --git a/cli/tools/vendor/mod.rs b/cli/tools/vendor/mod.rs index d478c2b57f..5690f5b227 100644 --- a/cli/tools/vendor/mod.rs +++ b/cli/tools/vendor/mod.rs @@ -5,6 +5,7 @@ use std::path::PathBuf; use std::sync::Arc; use deno_ast::ModuleSpecifier; +use deno_ast::TextChange; use deno_core::anyhow::bail; use deno_core::anyhow::Context; use deno_core::error::AnyError; @@ -12,6 +13,7 @@ use deno_core::resolve_url_or_path; use log::warn; use crate::args::CliOptions; +use crate::args::ConfigFile; use crate::args::Flags; use crate::args::FmtOptionsConfig; use crate::args::VendorFlags; @@ -51,6 +53,9 @@ pub async fn vendor( cli_options.initial_cwd(), ) .await?; + let npm_package_count = graph.npm_packages.len(); + let try_add_node_modules_dir = npm_package_count > 0 + && cli_options.node_modules_dir_enablement().unwrap_or(true); let vendored_count = build::build( graph, factory.parsed_source_cache()?, @@ -70,9 +75,48 @@ pub async fn vendor( }, raw_output_dir.display(), ); + + let try_add_import_map = vendored_count > 0; + let modified_result = maybe_update_config_file( + &output_dir, + cli_options, + try_add_import_map, + try_add_node_modules_dir, + ); + + // cache the node_modules folder when it's been added to the config file + if modified_result.added_node_modules_dir { + let node_modules_path = cli_options.node_modules_dir_path().or_else(|| { + cli_options + .maybe_config_file_specifier() + .filter(|c| c.scheme() == "file") + .and_then(|c| c.to_file_path().ok()) + .map(|config_path| config_path.parent().unwrap().join("node_modules")) + }); + if let Some(node_modules_path) = node_modules_path { + factory + .create_node_modules_npm_fs_resolver(node_modules_path) + .await? + .cache_packages() + .await?; + } + log::info!( + concat!( + "Vendored {} npm {} into node_modules directory. Set `nodeModulesDir: false` ", + "in the Deno configuration file to disable vendoring npm packages in the future.", + ), + npm_package_count, + if npm_package_count == 1 { + "package" + } else { + "packages" + }, + ); + } + if vendored_count > 0 { let import_map_path = raw_output_dir.join("import_map.json"); - if maybe_update_config_file(&output_dir, cli_options) { + if modified_result.updated_import_map { log::info!( concat!( "\nUpdated your local Deno configuration file with a reference to the ", @@ -154,107 +198,156 @@ fn validate_options( Ok(()) } -fn maybe_update_config_file(output_dir: &Path, options: &CliOptions) -> bool { +fn maybe_update_config_file( + output_dir: &Path, + options: &CliOptions, + try_add_import_map: bool, + try_add_node_modules_dir: bool, +) -> ModifiedResult { assert!(output_dir.is_absolute()); - let config_file_specifier = match options.maybe_config_file_specifier() { - Some(f) => f, - None => return false, + let config_file = match options.maybe_config_file() { + Some(config_file) => config_file, + None => return ModifiedResult::default(), }; + if config_file.specifier.scheme() != "file" { + return ModifiedResult::default(); + } - let fmt_config = options - .maybe_config_file() - .as_ref() - .and_then(|config| config.to_fmt_config().ok()) + let fmt_config = config_file + .to_fmt_config() + .ok() .unwrap_or_default() .unwrap_or_default(); let result = update_config_file( - &config_file_specifier, - &ModuleSpecifier::from_file_path(output_dir.join("import_map.json")) - .unwrap(), + config_file, &fmt_config.options, + if try_add_import_map { + Some( + ModuleSpecifier::from_file_path(output_dir.join("import_map.json")) + .unwrap(), + ) + } else { + None + }, + try_add_node_modules_dir, ); match result { - Ok(()) => true, + Ok(modified_result) => modified_result, Err(err) => { warn!("Error updating config file. {:#}", err); - false + ModifiedResult::default() } } } fn update_config_file( - config_specifier: &ModuleSpecifier, - import_map_specifier: &ModuleSpecifier, + config_file: &ConfigFile, fmt_options: &FmtOptionsConfig, -) -> Result<(), AnyError> { - if config_specifier.scheme() != "file" { - return Ok(()); - } - - let config_path = specifier_to_file_path(config_specifier)?; + import_map_specifier: Option, + try_add_node_modules_dir: bool, +) -> Result { + let config_path = specifier_to_file_path(&config_file.specifier)?; let config_text = std::fs::read_to_string(&config_path)?; - let relative_text = - match relative_specifier(config_specifier, import_map_specifier) { - Some(text) => text, - None => return Ok(()), // ignore - }; - if let Some(new_text) = - update_config_text(&config_text, &relative_text, fmt_options) - { + let import_map_specifier = + import_map_specifier.and_then(|import_map_specifier| { + relative_specifier(&config_file.specifier, &import_map_specifier) + }); + let modified_result = update_config_text( + &config_text, + fmt_options, + import_map_specifier.as_deref(), + try_add_node_modules_dir, + )?; + if let Some(new_text) = &modified_result.new_text { std::fs::write(config_path, new_text)?; } + Ok(modified_result) +} - Ok(()) +#[derive(Default)] +struct ModifiedResult { + updated_import_map: bool, + added_node_modules_dir: bool, + new_text: Option, } fn update_config_text( text: &str, - import_map_specifier: &str, fmt_options: &FmtOptionsConfig, -) -> Option { + import_map_specifier: Option<&str>, + try_add_node_modules_dir: bool, +) -> Result { use jsonc_parser::ast::ObjectProp; use jsonc_parser::ast::Value; let ast = - jsonc_parser::parse_to_ast(text, &Default::default(), &Default::default()) - .ok()?; + jsonc_parser::parse_to_ast(text, &Default::default(), &Default::default())?; let obj = match ast.value { Some(Value::Object(obj)) => obj, - _ => return None, // shouldn't happen, so ignore + _ => bail!("Failed updating config file due to no object."), }; - let import_map_specifier = import_map_specifier.replace('\"', "\\\""); + let mut modified_result = ModifiedResult::default(); + let mut text_changes = Vec::new(); + let mut should_format = false; - match obj.get("importMap") { - Some(ObjectProp { - value: Value::StringLit(lit), - .. - }) => Some(format!( - "{}{}{}", - &text[..lit.range.start + 1], - import_map_specifier, - &text[lit.range.end - 1..], - )), - None => { - // insert it crudely at a position that won't cause any issues - // with comments and format after to make it look nice + if try_add_node_modules_dir { + // Only modify the nodeModulesDir property if it's not set + // as this allows people to opt-out of this when vendoring + // by specifying `nodeModulesDir: false` + if obj.get("nodeModulesDir").is_none() { let insert_position = obj.range.end - 1; - let insert_text = format!( - r#"{}"importMap": "{}""#, - if obj.properties.is_empty() { "" } else { "," }, - import_map_specifier - ); - let new_text = format!( - "{}{}{}", - &text[..insert_position], - insert_text, - &text[insert_position..], - ); - format_json(&new_text, fmt_options) - .ok() - .map(|formatted_text| formatted_text.unwrap_or(new_text)) + text_changes.push(TextChange { + range: insert_position..insert_position, + new_text: r#""nodeModulesDir": true"#.to_string(), + }); + should_format = true; + modified_result.added_node_modules_dir = true; } - // shouldn't happen, so ignore - Some(_) => None, } + + if let Some(import_map_specifier) = import_map_specifier { + let import_map_specifier = import_map_specifier.replace('\"', "\\\""); + match obj.get("importMap") { + Some(ObjectProp { + value: Value::StringLit(lit), + .. + }) => { + text_changes.push(TextChange { + range: lit.range.start..lit.range.end, + new_text: format!("\"{}\"", import_map_specifier), + }); + modified_result.updated_import_map = true; + } + None => { + // insert it crudely at a position that won't cause any issues + // with comments and format after to make it look nice + let insert_position = obj.range.end - 1; + text_changes.push(TextChange { + range: insert_position..insert_position, + new_text: format!(r#""importMap": "{}""#, import_map_specifier), + }); + should_format = true; + modified_result.updated_import_map = true; + } + // shouldn't happen + Some(_) => { + bail!("Failed updating importMap in config file due to invalid type.") + } + } + } + + if text_changes.is_empty() { + return Ok(modified_result); + } + + let new_text = deno_ast::apply_text_changes(text, text_changes); + modified_result.new_text = if should_format { + format_json(&new_text, fmt_options) + .ok() + .map(|formatted_text| formatted_text.unwrap_or(new_text)) + } else { + Some(new_text) + }; + Ok(modified_result) } fn is_dir_empty(dir_path: &Path) -> Result { @@ -288,36 +381,94 @@ mod internal_test { #[test] fn update_config_text_no_existing_props_add_prop() { - let text = update_config_text( + let result = update_config_text( "{\n}", - "./vendor/import_map.json", &Default::default(), + Some("./vendor/import_map.json"), + false, ) .unwrap(); + assert!(result.updated_import_map); + assert!(!result.added_node_modules_dir); assert_eq!( - text, + result.new_text.unwrap(), r#"{ "importMap": "./vendor/import_map.json" } +"# + ); + + let result = update_config_text( + "{\n}", + &Default::default(), + Some("./vendor/import_map.json"), + true, + ) + .unwrap(); + assert!(result.updated_import_map); + assert!(result.added_node_modules_dir); + assert_eq!( + result.new_text.unwrap(), + r#"{ + "nodeModulesDir": true, + "importMap": "./vendor/import_map.json" +} +"# + ); + + let result = + update_config_text("{\n}", &Default::default(), None, true).unwrap(); + assert!(!result.updated_import_map); + assert!(result.added_node_modules_dir); + assert_eq!( + result.new_text.unwrap(), + r#"{ + "nodeModulesDir": true +} "# ); } #[test] fn update_config_text_existing_props_add_prop() { - let text = update_config_text( + let result = update_config_text( r#"{ "tasks": { "task1": "other" } } "#, - "./vendor/import_map.json", &Default::default(), + Some("./vendor/import_map.json"), + false, ) .unwrap(); assert_eq!( - text, + result.new_text.unwrap(), + r#"{ + "tasks": { + "task1": "other" + }, + "importMap": "./vendor/import_map.json" +} +"# + ); + + // trailing comma + let result = update_config_text( + r#"{ + "tasks": { + "task1": "other" + }, +} +"#, + &Default::default(), + Some("./vendor/import_map.json"), + false, + ) + .unwrap(); + assert_eq!( + result.new_text.unwrap(), r#"{ "tasks": { "task1": "other" @@ -330,21 +481,54 @@ mod internal_test { #[test] fn update_config_text_update_prop() { - let text = update_config_text( + let result = update_config_text( r#"{ "importMap": "./local.json" } "#, - "./vendor/import_map.json", &Default::default(), + Some("./vendor/import_map.json"), + false, ) .unwrap(); assert_eq!( - text, + result.new_text.unwrap(), r#"{ "importMap": "./vendor/import_map.json" } "# ); } + + #[test] + fn no_update_node_modules_dir() { + // will not update if this is already set (even if it's false) + let result = update_config_text( + r#"{ + "nodeModulesDir": false +} +"#, + &Default::default(), + None, + true, + ) + .unwrap(); + assert!(!result.added_node_modules_dir); + assert!(!result.updated_import_map); + assert_eq!(result.new_text, None); + + let result = update_config_text( + r#"{ + "nodeModulesDir": true +} +"#, + &Default::default(), + None, + true, + ) + .unwrap(); + assert!(!result.added_node_modules_dir); + assert!(!result.updated_import_map); + assert_eq!(result.new_text, None); + } } diff --git a/cli/tools/vendor/specifiers.rs b/cli/tools/vendor/specifiers.rs index 7418bcb8b5..bb7e0317a8 100644 --- a/cli/tools/vendor/specifiers.rs +++ b/cli/tools/vendor/specifiers.rs @@ -65,7 +65,7 @@ pub fn make_url_relative( } pub fn is_remote_specifier(specifier: &ModuleSpecifier) -> bool { - specifier.scheme().to_lowercase().starts_with("http") + matches!(specifier.scheme().to_lowercase().as_str(), "http" | "https") } pub fn is_remote_specifier_text(text: &str) -> bool { diff --git a/test_util/src/temp_dir.rs b/test_util/src/temp_dir.rs index dc638c7eaf..f0f866d886 100644 --- a/test_util/src/temp_dir.rs +++ b/test_util/src/temp_dir.rs @@ -58,6 +58,10 @@ impl TempDir { fs::create_dir_all(self.path().join(path)).unwrap(); } + pub fn remove_file(&self, path: impl AsRef) { + fs::remove_file(self.path().join(path)).unwrap(); + } + pub fn remove_dir_all(&self, path: impl AsRef) { fs::remove_dir_all(self.path().join(path)).unwrap(); }