From c678550a176ce5592d4b6e7ffb918e6926858a45 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Thu, 5 Sep 2024 16:22:13 +0200 Subject: [PATCH] BREAKING: remove "emit" and "map" from deno info output (#25468) The map field has been empty for years now and we don't want the emit file to be exposed so it allows us to iterate on making the cache faster. Additionally, it's racy/unreliable to rely on this information. Instead, people should emit the TS files themselves using tools like deno_emit, typescript, esbuild, etc. Closes https://github.com/denoland/deno/issues/17703 --- Cargo.lock | 4 +- cli/Cargo.toml | 2 +- cli/cache/emit.rs | 13 ---- cli/cache/mod.rs | 60 ++++++++----------- cli/factory.rs | 1 - cli/graph_util.rs | 4 -- cli/lsp/documents.rs | 10 +++- cli/schemas/module-graph.json | 8 --- cli/tools/info.rs | 16 ----- cli/util/path.rs | 27 +++++++++ tests/integration/info_tests.rs | 25 -------- tests/specs/info/multiple_redirects/main.out | 4 -- .../npm/cjs_with_deps/main_info_json.out | 2 - .../main_info_json.out | 2 - 14 files changed, 63 insertions(+), 115 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index dd3cc350c9..4ba940ad5a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1582,9 +1582,9 @@ dependencies = [ [[package]] name = "deno_graph" -version = "0.82.0" +version = "0.82.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "646757b109993751f618d20de9bafc17f8f886fa910fb82c2c89b9e1df220ac6" +checksum = "78b63015c73aa203da206b5d35b4c1eaa23bc7fed37ab325da62d525a5524a04" dependencies = [ "anyhow", "async-trait", diff --git a/cli/Cargo.toml b/cli/Cargo.toml index d9c3e21ad3..efe3f2ce50 100644 --- a/cli/Cargo.toml +++ b/cli/Cargo.toml @@ -68,7 +68,7 @@ deno_cache_dir = { workspace = true } deno_config = { version = "=0.33.2", features = ["workspace", "sync"] } deno_core = { workspace = true, features = ["include_js_files_for_snapshotting"] } deno_doc = { version = "0.148.0", features = ["html", "syntect"] } -deno_graph = { version = "=0.82.0" } +deno_graph = { version = "=0.82.1" } deno_lint = { version = "=0.64.0", features = ["docs"] } deno_lockfile.workspace = true deno_npm = "=0.25.0" diff --git a/cli/cache/emit.rs b/cli/cache/emit.rs index 5e89f9a90a..6807f06c10 100644 --- a/cli/cache/emit.rs +++ b/cli/cache/emit.rs @@ -82,19 +82,6 @@ impl EmitCache { Ok(()) } - /// Gets the filepath which stores the emit. - pub fn get_emit_filepath( - &self, - specifier: &ModuleSpecifier, - ) -> Option { - Some( - self - .disk_cache - .location - .join(self.get_emit_filename(specifier)?), - ) - } - fn get_emit_filename(&self, specifier: &ModuleSpecifier) -> Option { self .disk_cache diff --git a/cli/cache/mod.rs b/cli/cache/mod.rs index 4500e3a4ac..3b4e277607 100644 --- a/cli/cache/mod.rs +++ b/cli/cache/mod.rs @@ -8,6 +8,7 @@ use crate::file_fetcher::FileFetcher; use crate::file_fetcher::FileOrRedirect; use crate::npm::CliNpmResolver; use crate::util::fs::atomic_write_file_with_retries; +use crate::util::path::specifier_has_extension; use deno_ast::MediaType; use deno_core::futures; @@ -106,7 +107,6 @@ pub use deno_cache_dir::HttpCache; /// A "wrapper" for the FileFetcher and DiskCache for the Deno CLI that provides /// a concise interface to the DENO_DIR when building module graphs. pub struct FetchCacher { - emit_cache: Arc, file_fetcher: Arc, file_header_overrides: HashMap>, global_http_cache: Arc, @@ -118,7 +118,6 @@ pub struct FetchCacher { impl FetchCacher { pub fn new( - emit_cache: Arc, file_fetcher: Arc, file_header_overrides: HashMap>, global_http_cache: Arc, @@ -127,7 +126,6 @@ impl FetchCacher { permissions: PermissionsContainer, ) -> Self { Self { - emit_cache, file_fetcher, file_header_overrides, global_http_cache, @@ -144,15 +142,7 @@ impl FetchCacher { self.cache_info_enabled = true; } - // DEPRECATED: Where the file is stored and how it's stored should be an implementation - // detail of the cache. - // - // todo(dsheret): remove once implementing - // * https://github.com/denoland/deno/issues/17707 - // * https://github.com/denoland/deno/issues/17703 - #[deprecated( - note = "There should not be a way to do this because the file may not be cached at a local path in the future." - )] + /// Only use this for `deno info`. fn get_local_path(&self, specifier: &ModuleSpecifier) -> Option { // TODO(@kitsonk) fix when deno_graph does not query cache for synthetic // modules @@ -179,15 +169,7 @@ impl Loader for FetchCacher { #[allow(deprecated)] let local = self.get_local_path(specifier)?; if local.is_file() { - let emit = self - .emit_cache - .get_emit_filepath(specifier) - .filter(|p| p.is_file()); - Some(CacheInfo { - local: Some(local), - emit, - map: None, - }) + Some(CacheInfo { local: Some(local) }) } else { None } @@ -200,19 +182,28 @@ impl Loader for FetchCacher { ) -> LoadFuture { use deno_graph::source::CacheSetting as LoaderCacheSetting; - if specifier.scheme() == "file" - && specifier.path().contains("/node_modules/") - { - // The specifier might be in a completely different symlinked tree than - // what the node_modules url is in (ex. `/my-project-1/node_modules` - // symlinked to `/my-project-2/node_modules`), so first we checked if the path - // is in a node_modules dir to avoid needlessly canonicalizing, then now compare - // against the canonicalized specifier. - let specifier = - crate::node::resolve_specifier_into_node_modules(specifier); - if self.npm_resolver.in_npm_package(&specifier) { + if specifier.scheme() == "file" { + if specifier.path().contains("/node_modules/") { + // The specifier might be in a completely different symlinked tree than + // what the node_modules url is in (ex. `/my-project-1/node_modules` + // symlinked to `/my-project-2/node_modules`), so first we checked if the path + // is in a node_modules dir to avoid needlessly canonicalizing, then now compare + // against the canonicalized specifier. + let specifier = + crate::node::resolve_specifier_into_node_modules(specifier); + if self.npm_resolver.in_npm_package(&specifier) { + return Box::pin(futures::future::ready(Ok(Some( + LoadResponse::External { specifier }, + )))); + } + } + + // make local CJS modules external to the graph + if specifier_has_extension(specifier, "cjs") { return Box::pin(futures::future::ready(Ok(Some( - LoadResponse::External { specifier }, + LoadResponse::External { + specifier: specifier.clone(), + }, )))); } } @@ -293,6 +284,7 @@ impl Loader for FetchCacher { fn cache_module_info( &self, specifier: &ModuleSpecifier, + media_type: MediaType, source: &Arc<[u8]>, module_info: &deno_graph::ModuleInfo, ) { @@ -300,7 +292,7 @@ impl Loader for FetchCacher { let source_hash = CacheDBHash::from_source(source); let result = self.module_info_cache.set_module_info( specifier, - MediaType::from_specifier(specifier), + media_type, source_hash, module_info, ); diff --git a/cli/factory.rs b/cli/factory.rs index 81212c7885..a51bc771ea 100644 --- a/cli/factory.rs +++ b/cli/factory.rs @@ -616,7 +616,6 @@ impl CliFactory { self.parsed_source_cache().clone(), cli_options.maybe_lockfile().cloned(), self.maybe_file_watcher_reporter().clone(), - self.emit_cache()?.clone(), self.file_fetcher()?.clone(), self.global_http_cache()?.clone(), ))) diff --git a/cli/graph_util.rs b/cli/graph_util.rs index d7f007a7c3..d73733123f 100644 --- a/cli/graph_util.rs +++ b/cli/graph_util.rs @@ -368,7 +368,6 @@ pub struct ModuleGraphBuilder { parsed_source_cache: Arc, lockfile: Option>, maybe_file_watcher_reporter: Option, - emit_cache: Arc, file_fetcher: Arc, global_http_cache: Arc, } @@ -385,7 +384,6 @@ impl ModuleGraphBuilder { parsed_source_cache: Arc, lockfile: Option>, maybe_file_watcher_reporter: Option, - emit_cache: Arc, file_fetcher: Arc, global_http_cache: Arc, ) -> Self { @@ -399,7 +397,6 @@ impl ModuleGraphBuilder { parsed_source_cache, lockfile, maybe_file_watcher_reporter, - emit_cache, file_fetcher, global_http_cache, } @@ -681,7 +678,6 @@ impl ModuleGraphBuilder { permissions: PermissionsContainer, ) -> cache::FetchCacher { cache::FetchCacher::new( - self.emit_cache.clone(), self.file_fetcher.clone(), self.options.resolve_file_header_overrides(), self.global_http_cache.clone(), diff --git a/cli/lsp/documents.rs b/cli/lsp/documents.rs index ee9a522974..d6af96b238 100644 --- a/cli/lsp/documents.rs +++ b/cli/lsp/documents.rs @@ -1524,12 +1524,16 @@ impl<'a> deno_graph::source::Loader for OpenDocumentsGraphLoader<'a> { fn cache_module_info( &self, specifier: &deno_ast::ModuleSpecifier, + media_type: MediaType, source: &Arc<[u8]>, module_info: &deno_graph::ModuleInfo, ) { - self - .inner_loader - .cache_module_info(specifier, source, module_info) + self.inner_loader.cache_module_info( + specifier, + media_type, + source, + module_info, + ) } } diff --git a/cli/schemas/module-graph.json b/cli/schemas/module-graph.json index b761badded..ea32d843be 100644 --- a/cli/schemas/module-graph.json +++ b/cli/schemas/module-graph.json @@ -83,14 +83,6 @@ "type": "string", "description": "The checksum of the local source file. This can be used to validate if the current on disk version matches the version described here." }, - "emit": { - "type": "string", - "description": "The path to an emitted version of the module, if the module requires transpilation to be loaded into the Deno runtime." - }, - "map": { - "type": "string", - "description": "The path to an optionally emitted source map between the original and emitted version of the file." - }, "error": { "type": "string", "description": "If when resolving the module, Deno encountered an error and the module is unavailable, the text of that error will be indicated here." diff --git a/cli/tools/info.rs b/cli/tools/info.rs index 6aa044a924..d78b83cbe3 100644 --- a/cli/tools/info.rs +++ b/cli/tools/info.rs @@ -461,22 +461,6 @@ impl<'a> GraphDisplayContext<'a> { local.to_string_lossy() )?; } - if let Some(emit) = &cache_info.emit { - writeln!( - writer, - "{} {}", - colors::bold("emit:"), - emit.to_string_lossy() - )?; - } - if let Some(map) = &cache_info.map { - writeln!( - writer, - "{} {}", - colors::bold("map:"), - map.to_string_lossy() - )?; - } } if let Some(module) = root.js() { writeln!(writer, "{} {}", colors::bold("type:"), module.media_type)?; diff --git a/cli/util/path.rs b/cli/util/path.rs index 804b26f65f..6f09cf1eac 100644 --- a/cli/util/path.rs +++ b/cli/util/path.rs @@ -42,6 +42,21 @@ pub fn get_extension(file_path: &Path) -> Option { .map(|e| e.to_lowercase()); } +pub fn specifier_has_extension( + specifier: &ModuleSpecifier, + searching_ext: &str, +) -> bool { + let Some((_, ext)) = specifier.path().rsplit_once('.') else { + return false; + }; + let searching_ext = searching_ext.strip_prefix('.').unwrap_or(searching_ext); + debug_assert!(!searching_ext.contains('.')); // exts like .d.ts are not implemented here + if ext.len() != searching_ext.len() { + return false; + } + ext.eq_ignore_ascii_case(searching_ext) +} + pub fn get_atomic_dir_path(file_path: &Path) -> PathBuf { let rand = gen_rand_path_component(); let new_file_name = format!( @@ -377,6 +392,18 @@ mod test { } } + #[test] + fn test_specifier_has_extension() { + fn get(specifier: &str, ext: &str) -> bool { + specifier_has_extension(&ModuleSpecifier::parse(specifier).unwrap(), ext) + } + + assert!(get("file:///a/b/c.ts", "ts")); + assert!(get("file:///a/b/c.ts", ".ts")); + assert!(!get("file:///a/b/c.ts", ".cts")); + assert!(get("file:///a/b/c.CtS", ".cts")); + } + #[test] fn test_to_percent_decoded_str() { let str = to_percent_decoded_str("%F0%9F%A6%95"); diff --git a/tests/integration/info_tests.rs b/tests/integration/info_tests.rs index b22209a565..38dd9448f4 100644 --- a/tests/integration/info_tests.rs +++ b/tests/integration/info_tests.rs @@ -5,31 +5,6 @@ use test_util::itest; // use util::env_vars_for_npm_tests; use util::TestContextBuilder; -#[test] -fn info_with_compiled_source() { - let context = TestContextBuilder::new().use_http_server().build(); - let module_path = "http://127.0.0.1:4545/run/048_media_types_jsx.ts"; - - let output = context - .new_command() - .current_dir(util::testdata_path()) - .args_vec(["cache", module_path]) - .run(); - output.assert_exit_code(0); - output.skip_output_check(); - - let output = context - .new_command() - .current_dir(util::testdata_path()) - .args_vec(["info", module_path]) - .split_output() - .run(); - - // check the output of the test.ts program. - assert!(output.stdout().trim().contains("emit: ")); - assert_eq!(output.stderr(), ""); -} - #[test] fn info_lock_write() { let context = TestContextBuilder::new().use_http_server().build(); diff --git a/tests/specs/info/multiple_redirects/main.out b/tests/specs/info/multiple_redirects/main.out index 31123be778..9cc1066a93 100644 --- a/tests/specs/info/multiple_redirects/main.out +++ b/tests/specs/info/multiple_redirects/main.out @@ -27,8 +27,6 @@ Download http://localhost:4545/subdir/redirects/redirect1.js } ], "local": "[WILDLINE]main.ts", - "emit": null, - "map": null, "size": 97, "mediaType": "TypeScript", "specifier": "file:///[WILDLINE]/multiple_redirects/main.ts" @@ -36,8 +34,6 @@ Download http://localhost:4545/subdir/redirects/redirect1.js { "kind": "esm", "local": "[WILDLINE]", - "emit": null, - "map": null, "size": 27, "mediaType": "JavaScript", "specifier": "http://localhost:4545/subdir/redirects/redirect1.js" diff --git a/tests/testdata/npm/cjs_with_deps/main_info_json.out b/tests/testdata/npm/cjs_with_deps/main_info_json.out index 4d8c1a5bea..af1ef13518 100644 --- a/tests/testdata/npm/cjs_with_deps/main_info_json.out +++ b/tests/testdata/npm/cjs_with_deps/main_info_json.out @@ -42,8 +42,6 @@ } ], "local": "[WILDCARD]main.js", - "emit": null, - "map": null, "size": 325, "mediaType": "JavaScript", "specifier": "[WILDCARD]/main.js" diff --git a/tests/testdata/npm/peer_deps_with_copied_folders/main_info_json.out b/tests/testdata/npm/peer_deps_with_copied_folders/main_info_json.out index 48cb1f9920..df6541aa8e 100644 --- a/tests/testdata/npm/peer_deps_with_copied_folders/main_info_json.out +++ b/tests/testdata/npm/peer_deps_with_copied_folders/main_info_json.out @@ -42,8 +42,6 @@ } ], "local": "[WILDCARD]main.ts", - "emit": null, - "map": null, "size": 171, "mediaType": "TypeScript", "specifier": "file://[WILDCARD]/main.ts"