From ee27b9dd7e8ef311c6bfaa3264f7b9908a784a24 Mon Sep 17 00:00:00 2001 From: Kitson Kelly Date: Wed, 23 Feb 2022 10:51:14 +1100 Subject: [PATCH] feat: allow specification of import map in config file (#13739) Closes: #12800 --- cli/config_file.rs | 131 +++++++++++++++++++ cli/lsp/language_server.rs | 96 ++++++++++---- cli/main.rs | 22 +++- cli/proc_state.rs | 42 +++--- cli/schemas/config-file.v1.json | 4 + cli/tests/integration/lsp_tests.rs | 93 +++++++++++++ cli/tests/testdata/lsp/deno.import_map.jsonc | 3 + 7 files changed, 335 insertions(+), 56 deletions(-) create mode 100644 cli/tests/testdata/lsp/deno.import_map.jsonc diff --git a/cli/config_file.rs b/cli/config_file.rs index 3ca00b0f4c..53fe2f35e5 100644 --- a/cli/config_file.rs +++ b/cli/config_file.rs @@ -227,6 +227,56 @@ pub fn json_merge(a: &mut Value, b: &Value) { } } +/// 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_specifier( + maybe_import_map_path: Option<&str>, + maybe_config_file: Option<&ConfigFile>, +) -> Result, AnyError> { + if let Some(import_map_path) = maybe_import_map_path { + if let Some(config_file) = &maybe_config_file { + if config_file.to_import_map_path().is_some() { + log::warn!("{} the configuration file \"{}\" contains an entry for \"importMap\" that is being ignored.", crate::colors::yellow("Warning"), config_file.specifier); + } + } + let specifier = deno_core::resolve_url_or_path(import_map_path) + .context(format!("Bad URL (\"{}\") for import map.", import_map_path))?; + return Ok(Some(specifier)); + } else if let Some(config_file) = &maybe_config_file { + // when the import map is specifier in a config file, it needs to be + // resolved relative to the config file, versus the CWD like with the flag + // and with config files, we support both local and remote config files, + // so we have treat them differently. + if let Some(import_map_path) = config_file.to_import_map_path() { + let specifier = + // with local config files, it might be common to specify an import + // map like `"importMap": "import-map.json"`, which is resolvable if + // the file is resolved like a file path, so we will coerce the config + // file into a file path if possible and join the import map path to + // the file path. + if let Ok(config_file_path) = config_file.specifier.to_file_path() { + let import_map_file_path = config_file_path + .parent() + .ok_or_else(|| { + anyhow!("Bad config file specifier: {}", config_file.specifier) + })? + .join(&import_map_path); + ModuleSpecifier::from_file_path(import_map_file_path).unwrap() + // otherwise if the config file is remote, we have no choice but to + // use "import resolution" with the config file as the base. + } else { + deno_core::resolve_import(&import_map_path, config_file.specifier.as_str()) + .context(format!( + "Bad URL (\"{}\") for import map.", + import_map_path + ))? + }; + return Ok(Some(specifier)); + } + } + Ok(None) +} + fn parse_compiler_options( compiler_options: &HashMap, maybe_specifier: Option, @@ -476,6 +526,7 @@ pub struct FmtConfig { #[serde(rename_all = "camelCase")] pub struct ConfigFileJson { pub compiler_options: Option, + pub import_map: Option, pub lint: Option, pub fmt: Option, } @@ -583,6 +634,10 @@ impl ConfigFile { } } + pub fn to_import_map_path(&self) -> Option { + self.json.import_map.clone() + } + pub fn to_lint_config(&self) -> Result, AnyError> { if let Some(config) = self.json.lint.clone() { let lint_config: SerializedLintConfig = serde_json::from_value(config) @@ -914,4 +969,80 @@ mod tests { let err = discover_from(&d, &mut checked).unwrap_err(); assert!(err.to_string().contains("Unable to parse config file")); } + + #[cfg(not(windows))] + #[test] + fn resolve_import_map_config_file() { + let config_text = r#"{ + "importMap": "import_map.json" + }"#; + let config_specifier = + ModuleSpecifier::parse("file:///deno/deno.jsonc").unwrap(); + let config_file = ConfigFile::new(config_text, &config_specifier).unwrap(); + let actual = resolve_import_map_specifier(None, Some(&config_file)); + assert!(actual.is_ok()); + let actual = actual.unwrap(); + assert_eq!( + actual, + Some(ModuleSpecifier::parse("file:///deno/import_map.json").unwrap()) + ); + } + + #[test] + fn resolve_import_map_config_file_remote() { + let config_text = r#"{ + "importMap": "./import_map.json" + }"#; + let config_specifier = + ModuleSpecifier::parse("https://example.com/deno.jsonc").unwrap(); + let config_file = ConfigFile::new(config_text, &config_specifier).unwrap(); + let actual = resolve_import_map_specifier(None, Some(&config_file)); + assert!(actual.is_ok()); + let actual = actual.unwrap(); + assert_eq!( + actual, + Some( + ModuleSpecifier::parse("https://example.com/import_map.json").unwrap() + ) + ); + } + + #[test] + fn resolve_import_map_flags_take_precedence() { + let config_text = r#"{ + "importMap": "import_map.json" + }"#; + let config_specifier = + ModuleSpecifier::parse("file:///deno/deno.jsonc").unwrap(); + let config_file = ConfigFile::new(config_text, &config_specifier).unwrap(); + let actual = + resolve_import_map_specifier(Some("import-map.json"), Some(&config_file)); + let import_map_path = + std::env::current_dir().unwrap().join("import-map.json"); + let expected_specifier = + ModuleSpecifier::from_file_path(&import_map_path).unwrap(); + assert!(actual.is_ok()); + let actual = actual.unwrap(); + assert_eq!(actual, Some(expected_specifier)); + } + + #[test] + fn resolve_import_map_none() { + let config_text = r#"{}"#; + let config_specifier = + ModuleSpecifier::parse("file:///deno/deno.jsonc").unwrap(); + let config_file = ConfigFile::new(config_text, &config_specifier).unwrap(); + let actual = resolve_import_map_specifier(None, Some(&config_file)); + assert!(actual.is_ok()); + let actual = actual.unwrap(); + assert_eq!(actual, None); + } + + #[test] + fn resolve_import_map_no_config() { + let actual = resolve_import_map_specifier(None, None); + assert!(actual.is_ok()); + let actual = actual.unwrap(); + assert_eq!(actual, None); + } } diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index 8e197a6cec..ba275c593a 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -465,29 +465,65 @@ impl Inner { pub async fn update_import_map(&mut self) -> Result<(), AnyError> { let mark = self.performance.mark("update_import_map", None::<()>); self.maybe_cache_server = None; - let maybe_import_map = self.config.get_workspace_settings().import_map; - if let Some(import_map_str) = &maybe_import_map { - lsp_log!("Setting import map from: \"{}\"", import_map_str); - let import_map_url = if let Ok(url) = Url::from_file_path(import_map_str) - { - Ok(url) + let maybe_import_map_url = if let Some(import_map_str) = + self.config.get_workspace_settings().import_map + { + lsp_log!( + "Setting import map from workspace settings: \"{}\"", + import_map_str + ); + if let Some(config_file) = &self.maybe_config_file { + if let Some(import_map_path) = config_file.to_import_map_path() { + lsp_log!("Warning: Import map \"{}\" configured in \"{}\" being ignored due to an import map being explicitly configured in workspace settings.", import_map_path, config_file.specifier); + } + } + if let Ok(url) = Url::from_file_path(&import_map_str) { + Some(url) } else if import_map_str.starts_with("data:") { - Url::parse(import_map_str).map_err(|_| { - anyhow!("Bad data url for import map: {:?}", import_map_str) - }) + Some(Url::parse(&import_map_str).map_err(|_| { + anyhow!("Bad data url for import map: {}", import_map_str) + })?) } else if let Some(root_uri) = &self.root_uri { let root_path = fs_util::specifier_to_file_path(root_uri)?; - let import_map_path = root_path.join(import_map_str); - Url::from_file_path(import_map_path).map_err(|_| { - anyhow!("Bad file path for import map: {:?}", import_map_str) - }) + let import_map_path = root_path.join(&import_map_str); + Some(Url::from_file_path(import_map_path).map_err(|_| { + anyhow!("Bad file path for import map: {}", import_map_str) + })?) } else { - Err(anyhow!( + return Err(anyhow!( "The path to the import map (\"{}\") is not resolvable.", import_map_str - )) - }?; - + )); + } + } else if let Some(config_file) = &self.maybe_config_file { + if let Some(import_map_path) = config_file.to_import_map_path() { + lsp_log!( + "Setting import map from configuration file: \"{}\"", + import_map_path + ); + let specifier = + if let Ok(config_file_path) = config_file.specifier.to_file_path() { + let import_map_file_path = config_file_path + .parent() + .ok_or_else(|| { + anyhow!("Bad config file specifier: {}", config_file.specifier) + })? + .join(&import_map_path); + ModuleSpecifier::from_file_path(import_map_file_path).unwrap() + } else { + deno_core::resolve_import( + &import_map_path, + config_file.specifier.as_str(), + )? + }; + Some(specifier) + } else { + None + } + } else { + None + }; + if let Some(import_map_url) = maybe_import_map_url { let import_map_json = if import_map_url.scheme() == "data" { get_source_from_data_url(&import_map_url)?.0 } else { @@ -508,6 +544,7 @@ impl Inner { self.maybe_import_map_uri = Some(import_map_url); self.maybe_import_map = Some(Arc::new(import_map)); } else { + self.maybe_import_map_uri = None; self.maybe_import_map = None; } self.performance.measure(mark); @@ -854,15 +891,15 @@ impl Inner { if let Err(err) = self.update_cache() { self.client.show_message(MessageType::WARNING, err).await; } - if let Err(err) = self.update_import_map().await { - self.client.show_message(MessageType::WARNING, err).await; - } if let Err(err) = self.update_registries().await { self.client.show_message(MessageType::WARNING, err).await; } if let Err(err) = self.update_config_file() { self.client.show_message(MessageType::WARNING, err).await; } + if let Err(err) = self.update_import_map().await { + self.client.show_message(MessageType::WARNING, err).await; + } if let Err(err) = self.update_tsconfig().await { self.client.show_message(MessageType::WARNING, err).await; } @@ -889,15 +926,6 @@ impl Inner { .map(|f| self.url_map.normalize_url(&f.uri)) .collect(); - // if the current import map has changed, we need to reload it - if let Some(import_map_uri) = &self.maybe_import_map_uri { - if changes.iter().any(|uri| import_map_uri == uri) { - if let Err(err) = self.update_import_map().await { - self.client.show_message(MessageType::WARNING, err).await; - } - touched = true; - } - } // if the current tsconfig has changed, we need to reload it if let Some(config_file) = &self.maybe_config_file { if changes.iter().any(|uri| config_file.specifier == *uri) { @@ -910,6 +938,16 @@ impl Inner { touched = true; } } + // if the current import map, or config file has changed, we need to reload + // reload the import map + if let Some(import_map_uri) = &self.maybe_import_map_uri { + if changes.iter().any(|uri| import_map_uri == uri) || touched { + if let Err(err) = self.update_import_map().await { + self.client.show_message(MessageType::WARNING, err).await; + } + touched = true; + } + } if touched { self.documents.update_config( self.maybe_import_map.clone(), diff --git a/cli/main.rs b/cli/main.rs index f8c5d69df0..091c33b13b 100644 --- a/cli/main.rs +++ b/cli/main.rs @@ -803,9 +803,14 @@ async fn bundle_command( }) .collect(); - if let Some(import_map) = ps.flags.import_map_path.as_ref() { - paths_to_watch - .push(fs_util::resolve_from_cwd(std::path::Path::new(import_map))?); + if let Ok(Some(import_map_path)) = + config_file::resolve_import_map_specifier( + ps.flags.import_map_path.as_deref(), + ps.maybe_config_file.as_ref(), + ) + .map(|ms| ms.map(|ref s| s.to_file_path().ok()).flatten()) + { + paths_to_watch.push(import_map_path); } Ok((paths_to_watch, graph, ps)) @@ -1047,9 +1052,14 @@ async fn run_with_watch(flags: Flags, script: String) -> Result { paths_to_watch.extend(watch_paths); } - if let Some(import_map) = ps.flags.import_map_path.as_ref() { - paths_to_watch - .push(fs_util::resolve_from_cwd(std::path::Path::new(import_map))?); + if let Ok(Some(import_map_path)) = + config_file::resolve_import_map_specifier( + ps.flags.import_map_path.as_deref(), + ps.maybe_config_file.as_ref(), + ) + .map(|ms| ms.map(|ref s| s.to_file_path().ok()).flatten()) + { + paths_to_watch.push(import_map_path); } Ok((paths_to_watch, main_module, ps)) diff --git a/cli/proc_state.rs b/cli/proc_state.rs index 12bdd3149c..90194f397f 100644 --- a/cli/proc_state.rs +++ b/cli/proc_state.rs @@ -151,26 +151,26 @@ impl ProcState { let maybe_config_file = crate::config_file::discover(&flags)?; - let maybe_import_map: Option> = - match flags.import_map_path.as_ref() { - None => None, - Some(import_map_url) => { - let import_map_specifier = - deno_core::resolve_url_or_path(import_map_url).context(format!( - "Bad URL (\"{}\") for import map.", - import_map_url - ))?; - let file = file_fetcher - .fetch(&import_map_specifier, &mut Permissions::allow_all()) - .await - .context(format!( - "Unable to load '{}' import map", - import_map_specifier - ))?; - let import_map = - import_map_from_text(&import_map_specifier, &file.source)?; - Some(Arc::new(import_map)) - } + let maybe_import_map_specifier = + crate::config_file::resolve_import_map_specifier( + flags.import_map_path.as_deref(), + maybe_config_file.as_ref(), + )?; + + let maybe_import_map = + if let Some(import_map_specifier) = maybe_import_map_specifier { + let file = file_fetcher + .fetch(&import_map_specifier, &mut Permissions::allow_all()) + .await + .context(format!( + "Unable to load '{}' import map", + import_map_specifier + ))?; + let import_map = + import_map_from_text(&import_map_specifier, &file.source)?; + Some(Arc::new(import_map)) + } else { + None }; let maybe_inspect_host = flags.inspect.or(flags.inspect_brk); @@ -597,7 +597,7 @@ impl SourceMapGetter for ProcState { if let Ok(specifier) = resolve_url(file_name) { self.file_fetcher.get_source(&specifier).map(|out| { // Do NOT use .lines(): it skips the terminating empty line. - // (due to internally using .split_terminator() instead of .split()) + // (due to internally using_terminator() instead of .split()) let lines: Vec<&str> = out.source.split('\n').collect(); if line_number >= lines.len() { format!( diff --git a/cli/schemas/config-file.v1.json b/cli/schemas/config-file.v1.json index b8c4622a15..2a97ba43ed 100644 --- a/cli/schemas/config-file.v1.json +++ b/cli/schemas/config-file.v1.json @@ -185,6 +185,10 @@ } } }, + "importMap": { + "description": "The location of an import map to be used when resolving modules. If an import map is explicitly specified, it will override this value.", + "type": "string" + }, "lint": { "description": "Configuration for linter", "type": "object", diff --git a/cli/tests/integration/lsp_tests.rs b/cli/tests/integration/lsp_tests.rs index 2a5304dfcf..5547dc2140 100644 --- a/cli/tests/integration/lsp_tests.rs +++ b/cli/tests/integration/lsp_tests.rs @@ -477,6 +477,99 @@ fn lsp_import_map_data_url() { shutdown(&mut client); } +#[test] +fn lsp_import_map_config_file() { + let temp_dir = TempDir::new().expect("could not create temp dir"); + let mut params: lsp::InitializeParams = + serde_json::from_value(load_fixture("initialize_params.json")).unwrap(); + + let deno_import_map_jsonc = + serde_json::to_vec_pretty(&load_fixture("deno.import_map.jsonc")).unwrap(); + fs::write( + temp_dir.path().join("deno.import_map.jsonc"), + deno_import_map_jsonc, + ) + .unwrap(); + + params.root_uri = Some(Url::from_file_path(temp_dir.path()).unwrap()); + if let Some(Value::Object(mut map)) = params.initialization_options { + map.insert("config".to_string(), json!("./deno.import_map.jsonc")); + params.initialization_options = Some(Value::Object(map)); + } + let import_map = + serde_json::to_vec_pretty(&load_fixture("import-map.json")).unwrap(); + fs::write(temp_dir.path().join("import-map.json"), import_map).unwrap(); + fs::create_dir(temp_dir.path().join("lib")).unwrap(); + fs::write( + temp_dir.path().join("lib").join("b.ts"), + r#"export const b = "b";"#, + ) + .unwrap(); + + let deno_exe = deno_exe_path(); + let mut client = LspClient::new(&deno_exe).unwrap(); + client + .write_request::<_, _, Value>("initialize", params) + .unwrap(); + + client.write_notification("initialized", json!({})).unwrap(); + let uri = Url::from_file_path(temp_dir.path().join("a.ts")).unwrap(); + + let diagnostics = did_open( + &mut client, + json!({ + "textDocument": { + "uri": uri, + "languageId": "typescript", + "version": 1, + "text": "import { b } from \"/~/b.ts\";\n\nconsole.log(b);\n" + } + }), + ); + + let diagnostics = diagnostics.into_iter().flat_map(|x| x.diagnostics); + assert_eq!(diagnostics.count(), 0); + + let (maybe_res, maybe_err) = client + .write_request::<_, _, Value>( + "textDocument/hover", + json!({ + "textDocument": { + "uri": uri + }, + "position": { + "line": 2, + "character": 12 + } + }), + ) + .unwrap(); + assert!(maybe_err.is_none()); + assert_eq!( + maybe_res, + Some(json!({ + "contents": [ + { + "language": "typescript", + "value":"(alias) const b: \"b\"\nimport b" + }, + "" + ], + "range": { + "start": { + "line": 2, + "character": 12 + }, + "end": { + "line": 2, + "character": 13 + } + } + })) + ); + shutdown(&mut client); +} + #[test] fn lsp_import_assertions() { let mut client = init("initialize_params_import_map.json"); diff --git a/cli/tests/testdata/lsp/deno.import_map.jsonc b/cli/tests/testdata/lsp/deno.import_map.jsonc new file mode 100644 index 0000000000..cee56432e2 --- /dev/null +++ b/cli/tests/testdata/lsp/deno.import_map.jsonc @@ -0,0 +1,3 @@ +{ + "importMap": "import-map.json" +} \ No newline at end of file