From e46584a75a5a94cede193945dfd59eed8417aea0 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Wed, 29 Jun 2022 20:41:48 -0400 Subject: [PATCH] fix(vendor): ignore import map in output directory instead of erroring (#14998) --- cli/args/mod.rs | 28 ++++-- cli/main.rs | 5 +- cli/proc_state.rs | 5 +- cli/tests/integration/vendor_tests.rs | 124 ++++++++++++++++---------- cli/tools/vendor/mod.rs | 43 ++++++--- 5 files changed, 135 insertions(+), 70 deletions(-) diff --git a/cli/args/mod.rs b/cli/args/mod.rs index 17bbf06033..5844b0fa7b 100644 --- a/cli/args/mod.rs +++ b/cli/args/mod.rs @@ -44,6 +44,14 @@ use crate::file_fetcher::CacheSetting; use crate::lockfile::Lockfile; use crate::version; +/// Overrides for the options below that when set will +/// use these values over the values derived from the +/// CLI flags or config file. +#[derive(Default)] +struct CliOptionOverrides { + import_map_specifier: Option>, +} + /// Holds the common options used by many sub commands /// and provides some helper function for creating common objects. pub struct CliOptions { @@ -51,6 +59,7 @@ pub struct CliOptions { // application need not concern itself with, so keep these private flags: Flags, maybe_config_file: Option, + overrides: CliOptionOverrides, } impl CliOptions { @@ -72,6 +81,7 @@ impl CliOptions { Ok(Self { maybe_config_file, flags, + overrides: Default::default(), }) } @@ -113,13 +123,21 @@ impl CliOptions { /// Based on an optional command line import map path and an optional /// configuration file, return a resolved module specifier to an import map. - pub fn resolve_import_map_path( + pub fn resolve_import_map_specifier( &self, ) -> Result, AnyError> { - resolve_import_map_specifier( - self.flags.import_map_path.as_deref(), - self.maybe_config_file.as_ref(), - ) + match self.overrides.import_map_specifier.clone() { + Some(path) => Ok(path), + None => resolve_import_map_specifier( + self.flags.import_map_path.as_deref(), + self.maybe_config_file.as_ref(), + ), + } + } + + /// Overrides the import map specifier to use. + pub fn set_import_map_specifier(&mut self, path: Option) { + self.overrides.import_map_specifier = Some(path); } pub fn resolve_root_cert_store(&self) -> Result { diff --git a/cli/main.rs b/cli/main.rs index 4c7fa8f537..4cbb49ecac 100644 --- a/cli/main.rs +++ b/cli/main.rs @@ -769,7 +769,7 @@ async fn bundle_command( if let Ok(Some(import_map_path)) = ps .options - .resolve_import_map_path() + .resolve_import_map_specifier() .map(|ms| ms.and_then(|ref s| s.to_file_path().ok())) { paths_to_watch.push(import_map_path); @@ -1237,8 +1237,7 @@ async fn vendor_command( flags: Flags, vendor_flags: VendorFlags, ) -> Result { - let ps = ProcState::build(flags).await?; - tools::vendor::vendor(ps, vendor_flags).await?; + tools::vendor::vendor(flags, vendor_flags).await?; Ok(0) } diff --git a/cli/proc_state.rs b/cli/proc_state.rs index 47b4ac7527..b6b09c948d 100644 --- a/cli/proc_state.rs +++ b/cli/proc_state.rs @@ -117,7 +117,7 @@ impl ProcState { if let Ok(Some(import_map_path)) = ps .options - .resolve_import_map_path() + .resolve_import_map_specifier() .map(|ms| ms.and_then(|ref s| s.to_file_path().ok())) { files_to_watch_sender.send(vec![import_map_path]).unwrap(); @@ -153,7 +153,8 @@ impl ProcState { let lockfile = cli_options .resolve_lock_file()? .map(|f| Arc::new(Mutex::new(f))); - let maybe_import_map_specifier = cli_options.resolve_import_map_path()?; + let maybe_import_map_specifier = + cli_options.resolve_import_map_specifier()?; let maybe_import_map = if let Some(import_map_specifier) = maybe_import_map_specifier { diff --git a/cli/tests/integration/vendor_tests.rs b/cli/tests/integration/vendor_tests.rs index 7c106c79bd..9b8211cd16 100644 --- a/cli/tests/integration/vendor_tests.rs +++ b/cli/tests/integration/vendor_tests.rs @@ -72,43 +72,6 @@ fn output_dir_exists() { assert!(status.success()); } -#[test] -fn import_map_output_dir() { - let t = TempDir::new(); - t.write("mod.ts", ""); - t.create_dir_all("vendor"); - t.write( - "vendor/import_map.json", - "{ \"imports\": { \"https://localhost/\": \"./localhost/\" }}", - ); - - let deno = util::deno_cmd() - .current_dir(t.path()) - .env("NO_COLOR", "1") - .arg("vendor") - .arg("--force") - .arg("--import-map") - .arg("vendor/import_map.json") - .arg("mod.ts") - .stdout(Stdio::piped()) - .stderr(Stdio::piped()) - .spawn() - .unwrap(); - let output = deno.wait_with_output().unwrap(); - assert_eq!( - String::from_utf8_lossy(&output.stderr).trim(), - format!( - concat!( - "error: Specifying an import map file ({}) in the deno vendor ", - "output directory is not supported. Please specify no import ", - "map or one located outside this directory.", - ), - PathBuf::from("vendor").join("import_map.json").display(), - ), - ); - assert!(!output.status.success()); -} - #[test] fn standard_test() { let _server = http_server(); @@ -185,6 +148,57 @@ fn standard_test() { assert!(output.status.success()); } +#[test] +fn import_map_output_dir() { + let _server = http_server(); + let t = TempDir::new(); + t.write("mod.ts", ""); + t.create_dir_all("vendor"); + t.write( + "vendor/import_map.json", + // will be ignored + "{ \"imports\": { \"https://localhost:4545/\": \"./localhost/\" }}", + ); + t.write( + "deno.json", + "{ \"import_map\": \"./vendor/import_map.json\" }", + ); + t.write( + "my_app.ts", + "import {Logger} from 'http://localhost:4545/vendor/logger.ts'; new Logger().log('outputted');", + ); + + let deno = util::deno_cmd() + .current_dir(t.path()) + .env("NO_COLOR", "1") + .arg("vendor") + .arg("--force") + .arg("--import-map") + .arg("vendor/import_map.json") + .arg("my_app.ts") + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .unwrap(); + let output = deno.wait_with_output().unwrap(); + assert_eq!( + 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", + "Download http://localhost:4545/vendor/logger.ts\n", + "{}", + ), + PathBuf::from("vendor").join("import_map.json").display(), + success_text_updated_deno_json("1 module", "vendor/"), + ) + ); + assert!(output.status.success()); +} + #[test] fn remote_module_test() { let _server = http_server(); @@ -487,16 +501,8 @@ fn update_existing_config_test() { assert_eq!( String::from_utf8_lossy(&output.stderr).trim(), format!( - concat!( - "Download http://localhost:4545/vendor/logger.ts\n", - "Vendored 1 module into vendor2 directory.\n\n", - "Updated your local Deno configuration file with a reference to the ", - "new vendored import map at {}. 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." - ), - PathBuf::from("vendor2").join("import_map.json").display(), + "Download http://localhost:4545/vendor/logger.ts\n{}", + success_text_updated_deno_json("1 module", "vendor2",) ) ); assert_eq!(String::from_utf8_lossy(&output.stdout).trim(), ""); @@ -541,3 +547,27 @@ fn success_text(module_count: &str, dir: &str, has_import_map: bool) -> String { } text } + +fn success_text_updated_deno_json(module_count: &str, 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!( + "{}{}", + dir.trim_end_matches('/'), + if cfg!(windows) { '\\' } else { '/' } + ) + } else { + dir.to_string() + } + ) +} diff --git a/cli/tools/vendor/mod.rs b/cli/tools/vendor/mod.rs index 7c36b5074c..48f6ea7f10 100644 --- a/cli/tools/vendor/mod.rs +++ b/cli/tools/vendor/mod.rs @@ -2,6 +2,7 @@ use std::path::Path; use std::path::PathBuf; +use std::sync::Arc; use deno_ast::ModuleSpecifier; use deno_core::anyhow::bail; @@ -11,6 +12,8 @@ use deno_core::resolve_url_or_path; use deno_runtime::permissions::Permissions; use log::warn; +use crate::args::CliOptions; +use crate::args::Flags; use crate::args::FmtOptionsConfig; use crate::args::VendorFlags; use crate::fs_util; @@ -30,14 +33,20 @@ mod specifiers; #[cfg(test)] mod test; -pub async fn vendor(ps: ProcState, flags: VendorFlags) -> Result<(), AnyError> { - let raw_output_dir = match &flags.output_path { +pub async fn vendor( + flags: Flags, + vendor_flags: VendorFlags, +) -> Result<(), AnyError> { + let mut cli_options = CliOptions::from_flags(flags)?; + let raw_output_dir = match &vendor_flags.output_path { Some(output_path) => output_path.to_owned(), None => PathBuf::from("vendor/"), }; let output_dir = fs_util::resolve_from_cwd(&raw_output_dir)?; - validate_output_dir(&output_dir, &flags, &ps)?; - let graph = create_graph(&ps, &flags).await?; + validate_output_dir(&output_dir, &vendor_flags)?; + validate_options(&mut cli_options, &output_dir)?; + let ps = ProcState::from_options(Arc::new(cli_options)).await?; + let graph = create_graph(&ps, &vendor_flags).await?; let vendored_count = build::build( graph, &output_dir, @@ -86,7 +95,6 @@ pub async fn vendor(ps: ProcState, flags: VendorFlags) -> Result<(), AnyError> { fn validate_output_dir( output_dir: &Path, flags: &VendorFlags, - ps: &ProcState, ) -> Result<(), AnyError> { if !flags.force && !is_dir_empty(output_dir)? { bail!(concat!( @@ -94,12 +102,17 @@ fn validate_output_dir( "--force to ignore this error and potentially overwrite its contents.", )); } + Ok(()) +} +fn validate_options( + options: &mut CliOptions, + output_dir: &Path, +) -> Result<(), AnyError> { // check the import map - if let Some(import_map_path) = ps - .maybe_import_map - .as_ref() - .and_then(|m| specifier_to_file_path(m.base_url()).ok()) + if let Some(import_map_path) = options + .resolve_import_map_specifier()? + .and_then(|p| specifier_to_file_path(&p).ok()) .and_then(|p| fs_util::canonicalize_path(&p).ok()) { // make the output directory in order to canonicalize it for the check below @@ -114,11 +127,12 @@ fn validate_output_dir( let cwd = fs_util::canonicalize_path(&std::env::current_dir()?)?; // We don't allow using the output directory to help generate the // new state because this may lead to cryptic error messages. - bail!( + log::warn!( concat!( - "Specifying an import map file ({}) in the deno vendor output ", - "directory is not supported. Please specify no import map or one ", - "located outside this directory." + "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." ), import_map_path .strip_prefix(&cwd) @@ -126,6 +140,9 @@ fn validate_output_dir( .display() .to_string(), ); + + // don't use an import map in the config + options.set_import_map_specifier(None); } }