From bcfad0d5348d79aac36b88e70e00f1fe3ad4475a Mon Sep 17 00:00:00 2001 From: David Sherret Date: Sun, 20 Oct 2024 16:20:14 -0400 Subject: [PATCH 01/22] fix(node): requiring a .js file in Deno code should be treated as ESM --- cli/cache/mod.rs | 57 ++++++------------- cli/cache/parsed_source.rs | 29 ++++++++-- cli/factory.rs | 18 +++--- cli/graph_util.rs | 5 -- cli/lsp/resolver.rs | 2 + cli/standalone/mod.rs | 1 + cli/util/text_encoding.rs | 8 --- ext/node/lib.rs | 2 +- ext/node/ops/require.rs | 40 +++++++------ ext/node/polyfills/01_require.js | 13 ++--- resolvers/node/lib.rs | 1 + resolvers/node/resolution.rs | 39 +++++++++++-- .../specs/run/import_common_js/__test__.jsonc | 20 ++++--- tests/specs/run/import_common_js/a.js | 7 --- tests/specs/run/import_common_js/index.cjs | 2 - tests/specs/run/import_common_js/index.out | 2 +- .../npm_pkg_requires_esm_js/__test__.jsonc | 5 ++ .../specs/run/npm_pkg_requires_esm_js/file.js | 1 + .../npm_pkg_requires_esm_js/logs_require.js | 1 + .../specs/run/npm_pkg_requires_esm_js/main.js | 5 ++ .../node_modules/package/index.js | 3 + .../node_modules/package/package.json | 4 ++ .../run/npm_pkg_requires_esm_js/output.out | 13 +++++ .../run/npm_pkg_requires_esm_js/package.json | 0 24 files changed, 164 insertions(+), 114 deletions(-) delete mode 100644 tests/specs/run/import_common_js/a.js create mode 100644 tests/specs/run/npm_pkg_requires_esm_js/__test__.jsonc create mode 100644 tests/specs/run/npm_pkg_requires_esm_js/file.js create mode 100644 tests/specs/run/npm_pkg_requires_esm_js/logs_require.js create mode 100644 tests/specs/run/npm_pkg_requires_esm_js/main.js create mode 100644 tests/specs/run/npm_pkg_requires_esm_js/node_modules/package/index.js create mode 100644 tests/specs/run/npm_pkg_requires_esm_js/node_modules/package/package.json create mode 100644 tests/specs/run/npm_pkg_requires_esm_js/output.out create mode 100644 tests/specs/run/npm_pkg_requires_esm_js/package.json diff --git a/cli/cache/mod.rs b/cli/cache/mod.rs index ded163b4e48a12..9cea40a28383f5 100644 --- a/cli/cache/mod.rs +++ b/cli/cache/mod.rs @@ -14,8 +14,6 @@ use crate::util::fs::atomic_write_file_with_retries; use crate::util::fs::atomic_write_file_with_retries_and_fs; use crate::util::fs::AtomicWriteFileFsAdapter; use crate::util::path::specifier_has_extension; -use crate::util::text_encoding::arc_str_to_bytes; -use crate::util::text_encoding::from_utf8_lossy_owned; use deno_ast::MediaType; use deno_core::futures; @@ -60,7 +58,7 @@ pub use fast_check::FastCheckCache; pub use incremental::IncrementalCache; pub use module_info::ModuleInfoCache; pub use node::NodeAnalysisCache; -pub use parsed_source::EsmOrCjsChecker; +pub use parsed_source::CliContentIsEsmAnalyzer; pub use parsed_source::LazyGraphSourceParser; pub use parsed_source::ParsedSourceCache; @@ -188,7 +186,6 @@ pub struct FetchCacherOptions { /// a concise interface to the DENO_DIR when building module graphs. pub struct FetchCacher { pub file_header_overrides: HashMap>, - esm_or_cjs_checker: Arc, file_fetcher: Arc, global_http_cache: Arc, node_resolver: Arc, @@ -202,7 +199,6 @@ pub struct FetchCacher { impl FetchCacher { pub fn new( - esm_or_cjs_checker: Arc, file_fetcher: Arc, global_http_cache: Arc, node_resolver: Arc, @@ -212,7 +208,6 @@ impl FetchCacher { ) -> Self { Self { file_fetcher, - esm_or_cjs_checker, global_http_cache, node_resolver, npm_resolver, @@ -297,42 +292,24 @@ impl Loader for FetchCacher { } if self.unstable_detect_cjs && specifier_has_extension(specifier, "js") { - if let Ok(Some(pkg_json)) = - self.node_resolver.get_closest_package_json(specifier) - { - if pkg_json.typ == "commonjs" { - if let Ok(path) = specifier.to_file_path() { - if let Ok(bytes) = std::fs::read(&path) { - let text: Arc = from_utf8_lossy_owned(bytes).into(); - let is_es_module = match self.esm_or_cjs_checker.is_esm( - specifier, - text.clone(), - MediaType::JavaScript, - ) { - Ok(value) => value, - Err(err) => { - return Box::pin(futures::future::ready(Err(err.into()))); - } - }; - if !is_es_module { - self.node_resolver.mark_cjs_resolution(specifier.clone()); - return Box::pin(futures::future::ready(Ok(Some( - LoadResponse::External { - specifier: specifier.clone(), - }, - )))); - } else { - return Box::pin(futures::future::ready(Ok(Some( - LoadResponse::Module { - specifier: specifier.clone(), - content: arc_str_to_bytes(text), - maybe_headers: None, - }, - )))); - } - } + let resolution = + match self.node_resolver.url_to_node_resolution(specifier.clone()) { + Ok(resolution) => resolution, + Err(err) => { + return Box::pin(futures::future::ready(Err(err.into()))); } + }; + match resolution { + node_resolver::NodeResolution::CommonJs(specifier) => { + self.node_resolver.mark_cjs_resolution(specifier.clone()); + return Box::pin(futures::future::ready(Ok(Some( + LoadResponse::External { + specifier: specifier, + }, + )))); } + node_resolver::NodeResolution::Esm(_) => {} + node_resolver::NodeResolution::BuiltIn(_) => {} } } } diff --git a/cli/cache/parsed_source.rs b/cli/cache/parsed_source.rs index df6e45c35e565b..9112a9837877f8 100644 --- a/cli/cache/parsed_source.rs +++ b/cli/cache/parsed_source.rs @@ -7,12 +7,15 @@ use deno_ast::MediaType; use deno_ast::ModuleSpecifier; use deno_ast::ParseDiagnostic; use deno_ast::ParsedSource; +use deno_core::error::AnyError; use deno_core::parking_lot::Mutex; use deno_graph::CapturingModuleParser; use deno_graph::DefaultModuleParser; use deno_graph::ModuleParser; use deno_graph::ParseOptions; use deno_graph::ParsedSourceStore; +use deno_runtime::deno_fs::FileSystem; +use node_resolver::ContentIsEsmAnalyzer; /// Lazily parses JS/TS sources from a `deno_graph::ModuleGraph` given /// a `ParsedSourceCache`. Note that deno_graph doesn't necessarily cause @@ -47,7 +50,7 @@ impl<'a> LazyGraphSourceParser<'a> { } } -#[derive(Default)] +#[derive(Default, Debug)] pub struct ParsedSourceCache { sources: Mutex>, } @@ -151,13 +154,19 @@ impl deno_graph::ParsedSourceStore for ParsedSourceCache { } } -pub struct EsmOrCjsChecker { +#[derive(Debug)] +pub struct CliContentIsEsmAnalyzer { + fs: Arc, parsed_source_cache: Arc, } -impl EsmOrCjsChecker { - pub fn new(parsed_source_cache: Arc) -> Self { +impl CliContentIsEsmAnalyzer { + pub fn new( + fs: Arc, + parsed_source_cache: Arc, + ) -> Self { Self { + fs, parsed_source_cache, } } @@ -189,3 +198,15 @@ impl EsmOrCjsChecker { Ok(source.is_module()) } } + +impl ContentIsEsmAnalyzer for CliContentIsEsmAnalyzer { + fn analyze_content_is_esm( + &self, + specifier: &ModuleSpecifier, + ) -> Result { + let file_path = deno_path_util::url_to_file_path(specifier)?; + let content: Arc = + self.fs.read_text_file_lossy_sync(&file_path, None)?.into(); + Ok(self.is_esm(specifier, content, MediaType::from_specifier(specifier))?) + } +} diff --git a/cli/factory.rs b/cli/factory.rs index 25f3551102253f..e59a8a4b75f4a8 100644 --- a/cli/factory.rs +++ b/cli/factory.rs @@ -10,11 +10,11 @@ use crate::args::NpmInstallDepsProvider; use crate::args::StorageKeyResolver; use crate::args::TsConfigType; use crate::cache::Caches; +use crate::cache::CliContentIsEsmAnalyzer; use crate::cache::CodeCache; use crate::cache::DenoDir; use crate::cache::DenoDirProvider; use crate::cache::EmitCache; -use crate::cache::EsmOrCjsChecker; use crate::cache::GlobalHttpCache; use crate::cache::HttpCache; use crate::cache::LocalHttpCache; @@ -172,7 +172,6 @@ struct CliFactoryServices { http_client_provider: Deferred>, emit_cache: Deferred>, emitter: Deferred>, - esm_or_cjs_checker: Deferred>, fs: Deferred>, main_graph_container: Deferred>, maybe_inspector_server: Deferred>>, @@ -300,12 +299,6 @@ impl CliFactory { .get_or_init(|| ProgressBar::new(ProgressBarStyle::TextOnly)) } - pub fn esm_or_cjs_checker(&self) -> &Arc { - self.services.esm_or_cjs_checker.get_or_init(|| { - Arc::new(EsmOrCjsChecker::new(self.parsed_source_cache().clone())) - }) - } - pub fn global_http_cache(&self) -> Result<&Arc, AnyError> { self.services.global_http_cache.get_or_try_init(|| { Ok(Arc::new(GlobalHttpCache::new( @@ -565,6 +558,14 @@ impl CliFactory { Ok(Arc::new(NodeResolver::new( DenoFsNodeResolverEnv::new(self.fs().clone()), self.npm_resolver().await?.clone().into_npm_resolver(), + if self.cli_options()?.unstable_detect_cjs() { + Some(Box::new(CliContentIsEsmAnalyzer::new( + self.fs().clone(), + self.parsed_source_cache().clone(), + ))) + } else { + None + }, ))) } .boxed_local(), @@ -628,7 +629,6 @@ impl CliFactory { Ok(Arc::new(ModuleGraphBuilder::new( cli_options.clone(), self.caches()?.clone(), - self.esm_or_cjs_checker().clone(), self.fs().clone(), self.resolver().await?.clone(), self.cli_node_resolver().await?.clone(), diff --git a/cli/graph_util.rs b/cli/graph_util.rs index e67ae7821b6e35..e3842d06c0f913 100644 --- a/cli/graph_util.rs +++ b/cli/graph_util.rs @@ -6,7 +6,6 @@ use crate::args::CliLockfile; use crate::args::CliOptions; use crate::args::DENO_DISABLE_PEDANTIC_NODE_WARNINGS; use crate::cache; -use crate::cache::EsmOrCjsChecker; use crate::cache::GlobalHttpCache; use crate::cache::ModuleInfoCache; use crate::cache::ParsedSourceCache; @@ -381,7 +380,6 @@ pub struct BuildFastCheckGraphOptions<'a> { pub struct ModuleGraphBuilder { options: Arc, caches: Arc, - esm_or_cjs_checker: Arc, fs: Arc, resolver: Arc, node_resolver: Arc, @@ -400,7 +398,6 @@ impl ModuleGraphBuilder { pub fn new( options: Arc, caches: Arc, - esm_or_cjs_checker: Arc, fs: Arc, resolver: Arc, node_resolver: Arc, @@ -416,7 +413,6 @@ impl ModuleGraphBuilder { Self { options, caches, - esm_or_cjs_checker, fs, resolver, node_resolver, @@ -699,7 +695,6 @@ impl ModuleGraphBuilder { permissions: PermissionsContainer, ) -> cache::FetchCacher { cache::FetchCacher::new( - self.esm_or_cjs_checker.clone(), self.file_fetcher.clone(), self.global_http_cache.clone(), self.node_resolver.clone(), diff --git a/cli/lsp/resolver.rs b/cli/lsp/resolver.rs index c89273147afb8a..36997874ea5949 100644 --- a/cli/lsp/resolver.rs +++ b/cli/lsp/resolver.rs @@ -521,6 +521,8 @@ fn create_node_resolver( let node_resolver_inner = Arc::new(NodeResolver::new( deno_runtime::deno_node::DenoFsNodeResolverEnv::new(fs.clone()), npm_resolver.clone().into_npm_resolver(), + // todo(THIS PR): hook this up + None, )); Some(Arc::new(CliNodeResolver::new( CJS_RESOLUTIONS.clone(), diff --git a/cli/standalone/mod.rs b/cli/standalone/mod.rs index 60018228b7cb72..e8c16ed2fcf0a6 100644 --- a/cli/standalone/mod.rs +++ b/cli/standalone/mod.rs @@ -572,6 +572,7 @@ pub async fn run( let node_resolver = Arc::new(NodeResolver::new( deno_runtime::deno_node::DenoFsNodeResolverEnv::new(fs.clone()), npm_resolver.clone().into_npm_resolver(), + None, )); let cjs_resolutions = Arc::new(CjsResolutionStore::default()); let cache_db = Caches::new(deno_dir_provider.clone()); diff --git a/cli/util/text_encoding.rs b/cli/util/text_encoding.rs index 0b7601cb9c1608..9778b006b686fc 100644 --- a/cli/util/text_encoding.rs +++ b/cli/util/text_encoding.rs @@ -2,7 +2,6 @@ use std::borrow::Cow; use std::ops::Range; -use std::sync::Arc; use base64::prelude::BASE64_STANDARD; use base64::Engine; @@ -96,13 +95,6 @@ fn find_source_map_range(code: &[u8]) -> Option> { } } -/// Converts an `Arc` to an `Arc<[u8]>`. -pub fn arc_str_to_bytes(arc_str: Arc) -> Arc<[u8]> { - let raw = Arc::into_raw(arc_str); - // SAFETY: This is safe because they have the same memory layout. - unsafe { Arc::from_raw(raw as *const [u8]) } -} - #[cfg(test)] mod tests { use std::sync::Arc; diff --git a/ext/node/lib.rs b/ext/node/lib.rs index 9b22add4538aff..eb14e96e77a598 100644 --- a/ext/node/lib.rs +++ b/ext/node/lib.rs @@ -372,8 +372,8 @@ deno_core::extension!(deno_node, ops::require::op_require_path_basename, ops::require::op_require_read_file

, ops::require::op_require_as_file_path, + ops::require::op_require_module_format

, ops::require::op_require_resolve_exports

, - ops::require::op_require_read_closest_package_json

, ops::require::op_require_read_package_scope

, ops::require::op_require_package_imports_resolve

, ops::require::op_require_break_on_next_statement, diff --git a/ext/node/ops/require.rs b/ext/node/ops/require.rs index 7d85ee85328df2..649672a68371bf 100644 --- a/ext/node/ops/require.rs +++ b/ext/node/ops/require.rs @@ -1,5 +1,6 @@ // Copyright 2018-2024 the Deno authors. All rights reserved. MIT license. +use deno_core::anyhow::anyhow; use deno_core::anyhow::Context; use deno_core::error::generic_error; use deno_core::error::AnyError; @@ -191,17 +192,19 @@ pub fn op_require_resolve_deno_dir( state: &mut OpState, #[string] request: String, #[string] parent_filename: String, -) -> Option { +) -> Result, AnyError> { let resolver = state.borrow::(); - resolver - .resolve_package_folder_from_package( - &request, - &ModuleSpecifier::from_file_path(&parent_filename).unwrap_or_else(|_| { - panic!("Url::from_file_path: [{:?}]", parent_filename) - }), - ) - .ok() - .map(|p| p.to_string_lossy().into_owned()) + Ok( + resolver + .resolve_package_folder_from_package( + &request, + &ModuleSpecifier::from_file_path(&parent_filename).map_err(|_| { + anyhow!("Url::from_file_path: [{:?}]", parent_filename) + })?, + ) + .ok() + .map(|p| p.to_string_lossy().into_owned()), + ) } #[op2(fast)] @@ -521,20 +524,23 @@ where } #[op2] -#[serde] -pub fn op_require_read_closest_package_json

( +#[string] +pub fn op_require_module_format

( state: &mut OpState, #[string] filename: String, -) -> Result, AnyError> +) -> Result<&'static str, AnyError> where P: NodePermissions + 'static, { let filename = PathBuf::from(filename); - // permissions: allow reading the closest package.json files + let url = ModuleSpecifier::from_file_path(&filename) + .map_err(|_| anyhow!("Url::from_file_path: [{:?}]", filename))?; let node_resolver = state.borrow::().clone(); - node_resolver - .get_closest_package_json_from_path(&filename) - .map_err(AnyError::from) + match node_resolver.url_to_node_resolution(url)? { + node_resolver::NodeResolution::Esm(_) => Ok("module"), + node_resolver::NodeResolution::CommonJs(_) + | node_resolver::NodeResolution::BuiltIn(_) => Ok("commonjs"), + } } #[op2] diff --git a/ext/node/polyfills/01_require.js b/ext/node/polyfills/01_require.js index 5b0980c310f6cf..a4edbd7a3e3c02 100644 --- a/ext/node/polyfills/01_require.js +++ b/ext/node/polyfills/01_require.js @@ -12,6 +12,7 @@ import { op_require_init_paths, op_require_is_deno_dir_package, op_require_is_request_relative, + op_require_module_format, op_require_node_module_paths, op_require_package_imports_resolve, op_require_path_basename, @@ -19,7 +20,6 @@ import { op_require_path_is_absolute, op_require_path_resolve, op_require_proxy_path, - op_require_read_closest_package_json, op_require_read_file, op_require_read_package_scope, op_require_real_path, @@ -1068,15 +1068,10 @@ Module._extensions[".js"] = function (module, filename) { const content = op_require_read_file(filename); let format; - if (StringPrototypeEndsWith(filename, ".js")) { - const pkg = op_require_read_closest_package_json(filename); - if (pkg?.type === "module") { - format = "module"; - } else if (pkg?.type === "commonjs") { - format = "commonjs"; - } - } else if (StringPrototypeEndsWith(filename, ".cjs")) { + if (StringPrototypeEndsWith(filename, ".cjs")) { format = "commonjs"; + } else { + format = op_require_module_format(filename); } module._compile(content, filename, format); diff --git a/resolvers/node/lib.rs b/resolvers/node/lib.rs index f03f7704869fe5..fa82994e953bef 100644 --- a/resolvers/node/lib.rs +++ b/resolvers/node/lib.rs @@ -19,6 +19,7 @@ pub use package_json::load_pkg_json; pub use package_json::PackageJsonThreadLocalCache; pub use path::PathClean; pub use resolution::parse_npm_pkg_name; +pub use resolution::ContentIsEsmAnalyzer; pub use resolution::NodeModuleKind; pub use resolution::NodeResolution; pub use resolution::NodeResolutionMode; diff --git a/resolvers/node/resolution.rs b/resolvers/node/resolution.rs index 811583a5ee0474..391f3847a474f6 100644 --- a/resolvers/node/resolution.rs +++ b/resolvers/node/resolution.rs @@ -130,6 +130,11 @@ impl NodeResolution { } } +pub trait ContentIsEsmAnalyzer: std::fmt::Debug + Send + Sync { + /// Returns whether the content of the specified module specifier is ESM. + fn analyze_content_is_esm(&self, specifier: &Url) -> Result; +} + #[allow(clippy::disallowed_types)] pub type NodeResolverRc = crate::sync::MaybeArc>; @@ -137,11 +142,20 @@ pub type NodeResolverRc = crate::sync::MaybeArc>; pub struct NodeResolver { env: TEnv, npm_resolver: NpmResolverRc, + content_is_esm_analyzer: Option>, } impl NodeResolver { - pub fn new(env: TEnv, npm_resolver: NpmResolverRc) -> Self { - Self { env, npm_resolver } + pub fn new( + env: TEnv, + npm_resolver: NpmResolverRc, + content_is_esm_analyzer: Option>, + ) -> Self { + Self { + env, + npm_resolver, + content_is_esm_analyzer, + } } pub fn in_npm_package(&self, specifier: &Url) -> bool { @@ -406,14 +420,31 @@ impl NodeResolver { &self, url: Url, ) -> Result { - let url_str = url.as_str().to_lowercase(); + let url_str = url.path().to_lowercase(); if url_str.starts_with("http") || url_str.ends_with(".json") { Ok(NodeResolution::Esm(url)) } else if url_str.ends_with(".js") || url_str.ends_with(".d.ts") { let maybe_package_config = self.get_closest_package_json(&url)?; match maybe_package_config { Some(c) if c.typ == "module" => Ok(NodeResolution::Esm(url)), - Some(_) => Ok(NodeResolution::CommonJs(url)), + Some(c) => { + if self.in_npm_package(&url) { + Ok(NodeResolution::CommonJs(url)) + } else if c.typ == "commonjs" { + if let Some(analyzer) = &self.content_is_esm_analyzer { + let is_esm = + analyzer.analyze_content_is_esm(&url).unwrap_or(true); + match is_esm { + true => Ok(NodeResolution::Esm(url)), + false => Ok(NodeResolution::CommonJs(url)), + } + } else { + Ok(NodeResolution::Esm(url)) + } + } else { + Ok(NodeResolution::Esm(url)) + } + } None => Ok(NodeResolution::Esm(url)), } } else if url_str.ends_with(".mjs") || url_str.ends_with(".d.mts") { diff --git a/tests/specs/run/import_common_js/__test__.jsonc b/tests/specs/run/import_common_js/__test__.jsonc index 6510dbad7bcd80..0602a09baa6d69 100644 --- a/tests/specs/run/import_common_js/__test__.jsonc +++ b/tests/specs/run/import_common_js/__test__.jsonc @@ -1,21 +1,27 @@ { - "steps": [ - { "args": "run -R index.cjs", "output": "index.out" }, - { "args": "run -R main.ts", "output": "main.out" }, - { + "tests": { + "cjs_entrypoint": { + "args": "run -R index.cjs", + "output": "index.out" + }, + "esm_entrypoint": { + "args": "run -R main.ts", + "output": "main.out" + }, + "module_error": { "args": "run module_error.js", "output": "module_error.out", "exitCode": 1 }, - { + "exports_error": { "args": "run exports_error.js", "output": "exports_error.out", "exitCode": 1 }, - { + "require_error": { "args": "run require_error.js", "output": "require_error.out", "exitCode": 1 } - ] + } } diff --git a/tests/specs/run/import_common_js/a.js b/tests/specs/run/import_common_js/a.js deleted file mode 100644 index c465ab588b3a6d..00000000000000 --- a/tests/specs/run/import_common_js/a.js +++ /dev/null @@ -1,7 +0,0 @@ -function foobar() { - console.log("foobar"); -} - -module.exports = { - foobar, -}; diff --git a/tests/specs/run/import_common_js/index.cjs b/tests/specs/run/import_common_js/index.cjs index 18caf81e941ec7..0026e237d1c21a 100644 --- a/tests/specs/run/import_common_js/index.cjs +++ b/tests/specs/run/import_common_js/index.cjs @@ -1,9 +1,7 @@ const process = require("process"); -const a = require("./a"); console.log(process.cwd()); module.exports = { cwd: process.cwd, - foobar: a.foobar, }; diff --git a/tests/specs/run/import_common_js/index.out b/tests/specs/run/import_common_js/index.out index 3650631b7a2320..6a734b9948c861 100644 --- a/tests/specs/run/import_common_js/index.out +++ b/tests/specs/run/import_common_js/index.out @@ -1 +1 @@ -[WILDCARD]import_common_js +[WILDLINE]import_common_js diff --git a/tests/specs/run/npm_pkg_requires_esm_js/__test__.jsonc b/tests/specs/run/npm_pkg_requires_esm_js/__test__.jsonc new file mode 100644 index 00000000000000..3da8db40485e31 --- /dev/null +++ b/tests/specs/run/npm_pkg_requires_esm_js/__test__.jsonc @@ -0,0 +1,5 @@ +{ + "args": "run -A main.js", + "output": "output.out", + "exitCode": 1 +} diff --git a/tests/specs/run/npm_pkg_requires_esm_js/file.js b/tests/specs/run/npm_pkg_requires_esm_js/file.js new file mode 100644 index 00000000000000..d9536a69b3f87d --- /dev/null +++ b/tests/specs/run/npm_pkg_requires_esm_js/file.js @@ -0,0 +1 @@ +console.log(import.meta.url); diff --git a/tests/specs/run/npm_pkg_requires_esm_js/logs_require.js b/tests/specs/run/npm_pkg_requires_esm_js/logs_require.js new file mode 100644 index 00000000000000..984e1f3e74541b --- /dev/null +++ b/tests/specs/run/npm_pkg_requires_esm_js/logs_require.js @@ -0,0 +1 @@ +console.log(require); diff --git a/tests/specs/run/npm_pkg_requires_esm_js/main.js b/tests/specs/run/npm_pkg_requires_esm_js/main.js new file mode 100644 index 00000000000000..3704c8bf68031f --- /dev/null +++ b/tests/specs/run/npm_pkg_requires_esm_js/main.js @@ -0,0 +1,5 @@ +import doRequire from "package"; +import path from "node:path"; + +doRequire(path.resolve(import.meta.dirname, "file.js")); +doRequire(path.resolve(import.meta.dirname, "logs_require.js")); diff --git a/tests/specs/run/npm_pkg_requires_esm_js/node_modules/package/index.js b/tests/specs/run/npm_pkg_requires_esm_js/node_modules/package/index.js new file mode 100644 index 00000000000000..5d787237139888 --- /dev/null +++ b/tests/specs/run/npm_pkg_requires_esm_js/node_modules/package/index.js @@ -0,0 +1,3 @@ +module.exports = (file) => { + return require(file); +}; diff --git a/tests/specs/run/npm_pkg_requires_esm_js/node_modules/package/package.json b/tests/specs/run/npm_pkg_requires_esm_js/node_modules/package/package.json new file mode 100644 index 00000000000000..5723987e9f4279 --- /dev/null +++ b/tests/specs/run/npm_pkg_requires_esm_js/node_modules/package/package.json @@ -0,0 +1,4 @@ +{ + "name": "package", + "version": "1.0.0" +} \ No newline at end of file diff --git a/tests/specs/run/npm_pkg_requires_esm_js/output.out b/tests/specs/run/npm_pkg_requires_esm_js/output.out new file mode 100644 index 00000000000000..667920d31f54b3 --- /dev/null +++ b/tests/specs/run/npm_pkg_requires_esm_js/output.out @@ -0,0 +1,13 @@ +file:///[WILDLINE]/file.js +error: Uncaught (in promise) ReferenceError: require is not defined +console.log(require); + ^ + at [WILDCARD] + + info: Deno supports CommonJS modules in .cjs files, or when there's a package.json + with "type": "commonjs" option and --unstable-detect-cjs flag is used. + hint: Rewrite this module to ESM, + or change the file extension to .cjs, + or add package.json next to the file with "type": "commonjs" option + and pass --unstable-detect-cjs flag. + hint: See https://docs.deno.com/go/commonjs for details diff --git a/tests/specs/run/npm_pkg_requires_esm_js/package.json b/tests/specs/run/npm_pkg_requires_esm_js/package.json new file mode 100644 index 00000000000000..e69de29bb2d1d6 From d69a3e962d01cbe19afc629598d824ce13ac83a8 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Sun, 20 Oct 2024 16:21:39 -0400 Subject: [PATCH 02/22] remove todo --- cli/lsp/resolver.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/cli/lsp/resolver.rs b/cli/lsp/resolver.rs index 36997874ea5949..306a4c95e79723 100644 --- a/cli/lsp/resolver.rs +++ b/cli/lsp/resolver.rs @@ -521,7 +521,6 @@ fn create_node_resolver( let node_resolver_inner = Arc::new(NodeResolver::new( deno_runtime::deno_node::DenoFsNodeResolverEnv::new(fs.clone()), npm_resolver.clone().into_npm_resolver(), - // todo(THIS PR): hook this up None, )); Some(Arc::new(CliNodeResolver::new( From ef56f5e4e415df812638a8000f5a70cfa6e2c0da Mon Sep 17 00:00:00 2001 From: David Sherret Date: Sun, 20 Oct 2024 16:40:51 -0400 Subject: [PATCH 03/22] lint --- cli/cache/mod.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/cli/cache/mod.rs b/cli/cache/mod.rs index 9cea40a28383f5..ca3a5df99a0187 100644 --- a/cli/cache/mod.rs +++ b/cli/cache/mod.rs @@ -303,9 +303,7 @@ impl Loader for FetchCacher { node_resolver::NodeResolution::CommonJs(specifier) => { self.node_resolver.mark_cjs_resolution(specifier.clone()); return Box::pin(futures::future::ready(Ok(Some( - LoadResponse::External { - specifier: specifier, - }, + LoadResponse::External { specifier }, )))); } node_resolver::NodeResolution::Esm(_) => {} From 3afd732b47754ba47e640c4f368068d70074868f Mon Sep 17 00:00:00 2001 From: David Sherret Date: Sun, 20 Oct 2024 17:13:25 -0400 Subject: [PATCH 04/22] fix node compat tests probably --- tests/config/deno.json | 5 ++++- tests/node_compat/deno.json | 5 ++++- tests/node_compat/test/common/package.json | 4 +++- tests/node_compat/test/fixtures/package.json | 4 +++- tests/node_compat/test/internet/package.json | 4 +++- tests/node_compat/test/parallel/package.json | 4 +++- tests/node_compat/test/pseudo-tty/package.json | 4 +++- tests/node_compat/test/pummel/package.json | 4 +++- tests/node_compat/test/sequential/package.json | 4 +++- 9 files changed, 29 insertions(+), 9 deletions(-) diff --git a/tests/config/deno.json b/tests/config/deno.json index c1c6f5112007a9..cf3920fdb46c26 100644 --- a/tests/config/deno.json +++ b/tests/config/deno.json @@ -1,4 +1,7 @@ { "lock": false, - "importMap": "../../import_map.json" + "importMap": "../../import_map.json", + "unstable": [ + "detect-cjs" + ] } diff --git a/tests/node_compat/deno.json b/tests/node_compat/deno.json index 315d5acda8e732..3deb8f047785c4 100644 --- a/tests/node_compat/deno.json +++ b/tests/node_compat/deno.json @@ -1,3 +1,6 @@ { - "importMap": "../../import_map.json" + "importMap": "../../import_map.json", + "unstable": [ + "detect-cjs" + ] } diff --git a/tests/node_compat/test/common/package.json b/tests/node_compat/test/common/package.json index 0967ef424bce67..5bbefffbabee39 100644 --- a/tests/node_compat/test/common/package.json +++ b/tests/node_compat/test/common/package.json @@ -1 +1,3 @@ -{} +{ + "type": "commonjs" +} diff --git a/tests/node_compat/test/fixtures/package.json b/tests/node_compat/test/fixtures/package.json index 0967ef424bce67..5bbefffbabee39 100644 --- a/tests/node_compat/test/fixtures/package.json +++ b/tests/node_compat/test/fixtures/package.json @@ -1 +1,3 @@ -{} +{ + "type": "commonjs" +} diff --git a/tests/node_compat/test/internet/package.json b/tests/node_compat/test/internet/package.json index 0967ef424bce67..5bbefffbabee39 100644 --- a/tests/node_compat/test/internet/package.json +++ b/tests/node_compat/test/internet/package.json @@ -1 +1,3 @@ -{} +{ + "type": "commonjs" +} diff --git a/tests/node_compat/test/parallel/package.json b/tests/node_compat/test/parallel/package.json index 0967ef424bce67..5bbefffbabee39 100644 --- a/tests/node_compat/test/parallel/package.json +++ b/tests/node_compat/test/parallel/package.json @@ -1 +1,3 @@ -{} +{ + "type": "commonjs" +} diff --git a/tests/node_compat/test/pseudo-tty/package.json b/tests/node_compat/test/pseudo-tty/package.json index 0967ef424bce67..5bbefffbabee39 100644 --- a/tests/node_compat/test/pseudo-tty/package.json +++ b/tests/node_compat/test/pseudo-tty/package.json @@ -1 +1,3 @@ -{} +{ + "type": "commonjs" +} diff --git a/tests/node_compat/test/pummel/package.json b/tests/node_compat/test/pummel/package.json index 0967ef424bce67..5bbefffbabee39 100644 --- a/tests/node_compat/test/pummel/package.json +++ b/tests/node_compat/test/pummel/package.json @@ -1 +1,3 @@ -{} +{ + "type": "commonjs" +} diff --git a/tests/node_compat/test/sequential/package.json b/tests/node_compat/test/sequential/package.json index 0967ef424bce67..5bbefffbabee39 100644 --- a/tests/node_compat/test/sequential/package.json +++ b/tests/node_compat/test/sequential/package.json @@ -1 +1,3 @@ -{} +{ + "type": "commonjs" +} From 6953eab3791be11c7d1f04728bfff2288e323ea3 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Sun, 20 Oct 2024 17:28:54 -0400 Subject: [PATCH 05/22] Maybe fix. --- tests/node_compat/{test/common => }/package.json | 0 tests/node_compat/test/fixtures/package.json | 3 --- tests/node_compat/test/internet/package.json | 3 --- tests/node_compat/test/parallel/package.json | 3 --- tests/node_compat/test/pseudo-tty/package.json | 3 --- tests/node_compat/test/pummel/package.json | 3 --- tests/node_compat/test/sequential/package.json | 3 --- 7 files changed, 18 deletions(-) rename tests/node_compat/{test/common => }/package.json (100%) delete mode 100644 tests/node_compat/test/fixtures/package.json delete mode 100644 tests/node_compat/test/internet/package.json delete mode 100644 tests/node_compat/test/parallel/package.json delete mode 100644 tests/node_compat/test/pseudo-tty/package.json delete mode 100644 tests/node_compat/test/pummel/package.json delete mode 100644 tests/node_compat/test/sequential/package.json diff --git a/tests/node_compat/test/common/package.json b/tests/node_compat/package.json similarity index 100% rename from tests/node_compat/test/common/package.json rename to tests/node_compat/package.json diff --git a/tests/node_compat/test/fixtures/package.json b/tests/node_compat/test/fixtures/package.json deleted file mode 100644 index 5bbefffbabee39..00000000000000 --- a/tests/node_compat/test/fixtures/package.json +++ /dev/null @@ -1,3 +0,0 @@ -{ - "type": "commonjs" -} diff --git a/tests/node_compat/test/internet/package.json b/tests/node_compat/test/internet/package.json deleted file mode 100644 index 5bbefffbabee39..00000000000000 --- a/tests/node_compat/test/internet/package.json +++ /dev/null @@ -1,3 +0,0 @@ -{ - "type": "commonjs" -} diff --git a/tests/node_compat/test/parallel/package.json b/tests/node_compat/test/parallel/package.json deleted file mode 100644 index 5bbefffbabee39..00000000000000 --- a/tests/node_compat/test/parallel/package.json +++ /dev/null @@ -1,3 +0,0 @@ -{ - "type": "commonjs" -} diff --git a/tests/node_compat/test/pseudo-tty/package.json b/tests/node_compat/test/pseudo-tty/package.json deleted file mode 100644 index 5bbefffbabee39..00000000000000 --- a/tests/node_compat/test/pseudo-tty/package.json +++ /dev/null @@ -1,3 +0,0 @@ -{ - "type": "commonjs" -} diff --git a/tests/node_compat/test/pummel/package.json b/tests/node_compat/test/pummel/package.json deleted file mode 100644 index 5bbefffbabee39..00000000000000 --- a/tests/node_compat/test/pummel/package.json +++ /dev/null @@ -1,3 +0,0 @@ -{ - "type": "commonjs" -} diff --git a/tests/node_compat/test/sequential/package.json b/tests/node_compat/test/sequential/package.json deleted file mode 100644 index 5bbefffbabee39..00000000000000 --- a/tests/node_compat/test/sequential/package.json +++ /dev/null @@ -1,3 +0,0 @@ -{ - "type": "commonjs" -} From ea9002ab8880199bec7f7892ee7cca0aa7aad340 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Sun, 20 Oct 2024 17:48:52 -0400 Subject: [PATCH 06/22] fix spec failures --- tests/specs/npm/permissions_outside_package/__test__.jsonc | 4 ++-- .../{permissions_outside_package => }/foo/config.js | 0 tests/specs/npm/permissions_outside_package/foo/package.json | 5 +++++ .../{permissions_outside_package => }/main.out | 0 .../{permissions_outside_package => }/main.ts | 2 +- .../permissions_outside_package/foo/package.json | 4 ---- tests/specs/run/import_common_js/main.out | 2 -- tests/specs/run/import_common_js/node_modules/foo/index.mjs | 1 - 8 files changed, 8 insertions(+), 10 deletions(-) rename tests/specs/npm/permissions_outside_package/{permissions_outside_package => }/foo/config.js (100%) create mode 100644 tests/specs/npm/permissions_outside_package/foo/package.json rename tests/specs/npm/permissions_outside_package/{permissions_outside_package => }/main.out (100%) rename tests/specs/npm/permissions_outside_package/{permissions_outside_package => }/main.ts (64%) delete mode 100644 tests/specs/npm/permissions_outside_package/permissions_outside_package/foo/package.json diff --git a/tests/specs/npm/permissions_outside_package/__test__.jsonc b/tests/specs/npm/permissions_outside_package/__test__.jsonc index 56228296b3ed95..727f5b75947f6e 100644 --- a/tests/specs/npm/permissions_outside_package/__test__.jsonc +++ b/tests/specs/npm/permissions_outside_package/__test__.jsonc @@ -1,4 +1,4 @@ { - "args": "run --allow-read permissions_outside_package/main.ts", - "output": "permissions_outside_package/main.out" + "args": "run --allow-read --unstable-detect-cjs main.ts", + "output": "main.out" } diff --git a/tests/specs/npm/permissions_outside_package/permissions_outside_package/foo/config.js b/tests/specs/npm/permissions_outside_package/foo/config.js similarity index 100% rename from tests/specs/npm/permissions_outside_package/permissions_outside_package/foo/config.js rename to tests/specs/npm/permissions_outside_package/foo/config.js diff --git a/tests/specs/npm/permissions_outside_package/foo/package.json b/tests/specs/npm/permissions_outside_package/foo/package.json new file mode 100644 index 00000000000000..95b43077e4ab30 --- /dev/null +++ b/tests/specs/npm/permissions_outside_package/foo/package.json @@ -0,0 +1,5 @@ +{ + "name": "foobar", + "version": "0.0.1", + "type": "commonjs" +} diff --git a/tests/specs/npm/permissions_outside_package/permissions_outside_package/main.out b/tests/specs/npm/permissions_outside_package/main.out similarity index 100% rename from tests/specs/npm/permissions_outside_package/permissions_outside_package/main.out rename to tests/specs/npm/permissions_outside_package/main.out diff --git a/tests/specs/npm/permissions_outside_package/permissions_outside_package/main.ts b/tests/specs/npm/permissions_outside_package/main.ts similarity index 64% rename from tests/specs/npm/permissions_outside_package/permissions_outside_package/main.ts rename to tests/specs/npm/permissions_outside_package/main.ts index 934a3eebcfaf7c..a80713fa7233d7 100644 --- a/tests/specs/npm/permissions_outside_package/permissions_outside_package/main.ts +++ b/tests/specs/npm/permissions_outside_package/main.ts @@ -1,5 +1,5 @@ import { loadConfigFile } from "npm:@denotest/permissions-outside-package"; -const fileName = `${Deno.cwd()}/permissions_outside_package/foo/config.js`; +const fileName = `${Deno.cwd()}/foo/config.js`; const config = loadConfigFile(fileName); console.log(config); diff --git a/tests/specs/npm/permissions_outside_package/permissions_outside_package/foo/package.json b/tests/specs/npm/permissions_outside_package/permissions_outside_package/foo/package.json deleted file mode 100644 index cc049e6ce909c0..00000000000000 --- a/tests/specs/npm/permissions_outside_package/permissions_outside_package/foo/package.json +++ /dev/null @@ -1,4 +0,0 @@ -{ - "name": "foobar", - "version": "0.0.1" -} diff --git a/tests/specs/run/import_common_js/main.out b/tests/specs/run/import_common_js/main.out index 03301b36209602..9df31297548e70 100644 --- a/tests/specs/run/import_common_js/main.out +++ b/tests/specs/run/import_common_js/main.out @@ -1,5 +1,3 @@ hello from foo node module [WILDCARD]import_common_js cjsModule.cwd() [WILDCARD]import_common_js -foobar -cjsModule.foobar() undefined diff --git a/tests/specs/run/import_common_js/node_modules/foo/index.mjs b/tests/specs/run/import_common_js/node_modules/foo/index.mjs index cc93554c73c3fd..7a11d39ae60266 100644 --- a/tests/specs/run/import_common_js/node_modules/foo/index.mjs +++ b/tests/specs/run/import_common_js/node_modules/foo/index.mjs @@ -10,5 +10,4 @@ export default async function () { const cjsModule = await import(url.pathToFileURL(cjsFileToImport)); console.log("cjsModule.cwd()", cjsModule.cwd()); - console.log("cjsModule.foobar()", cjsModule.foobar()); } From ebd12a5ccf9ff2576fb62d188d092e7e149855e9 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Fri, 8 Nov 2024 12:48:10 -0500 Subject: [PATCH 07/22] fixes --- cli/cache/mod.rs | 1 + cli/cache/parsed_source.rs | 3 -- cli/util/text_encoding.rs | 1 + ext/node/ops/require.rs | 35 ++++++++++++++----- ext/node/polyfills/01_require.js | 26 ++------------ resolvers/node/lib.rs | 1 - resolvers/node/resolution.rs | 5 --- .../jsx/#jsx-runtime_62ac8.js | 21 +++++------ .../run/npm_pkg_requires_esm_js/output.out | 2 +- tests/specs/run/require_esm/main.out | 11 +----- 10 files changed, 45 insertions(+), 61 deletions(-) diff --git a/cli/cache/mod.rs b/cli/cache/mod.rs index 192ec02dd0132c..50fc135ddffcd3 100644 --- a/cli/cache/mod.rs +++ b/cli/cache/mod.rs @@ -204,6 +204,7 @@ impl FetchCacher { ) -> Self { Self { file_fetcher, + fs, global_http_cache, in_npm_pkg_checker, module_info_cache, diff --git a/cli/cache/parsed_source.rs b/cli/cache/parsed_source.rs index 65c221074dbb30..7e819ae9985ba3 100644 --- a/cli/cache/parsed_source.rs +++ b/cli/cache/parsed_source.rs @@ -6,15 +6,12 @@ use std::sync::Arc; use deno_ast::MediaType; use deno_ast::ModuleSpecifier; use deno_ast::ParsedSource; -use deno_core::error::AnyError; use deno_core::parking_lot::Mutex; use deno_graph::CapturingEsParser; use deno_graph::DefaultEsParser; use deno_graph::EsParser; use deno_graph::ParseOptions; use deno_graph::ParsedSourceStore; -use deno_runtime::deno_fs::FileSystem; -use node_resolver::ContentIsEsmAnalyzer; /// Lazily parses JS/TS sources from a `deno_graph::ModuleGraph` given /// a `ParsedSourceCache`. Note that deno_graph doesn't necessarily cause diff --git a/cli/util/text_encoding.rs b/cli/util/text_encoding.rs index 9c2d15859e16a4..8524e63ebb32c2 100644 --- a/cli/util/text_encoding.rs +++ b/cli/util/text_encoding.rs @@ -2,6 +2,7 @@ use std::borrow::Cow; use std::ops::Range; +use std::sync::Arc; use base64::prelude::BASE64_STANDARD; use base64::Engine; diff --git a/ext/node/ops/require.rs b/ext/node/ops/require.rs index cd1314202f832e..97d1f993fd9d50 100644 --- a/ext/node/ops/require.rs +++ b/ext/node/ops/require.rs @@ -1,14 +1,15 @@ // Copyright 2018-2024 the Deno authors. All rights reserved. MIT license. +use deno_core::error::AnyError; use deno_core::op2; use deno_core::url::Url; use deno_core::v8; use deno_core::JsRuntimeInspector; -use deno_core::ModuleSpecifier; use deno_core::OpState; use deno_fs::FileSystemRc; use deno_package_json::PackageJsonRc; use deno_path_util::normalize_path; +use deno_path_util::url_from_file_path; use deno_path_util::url_to_file_path; use node_resolver::NodeModuleKind; use node_resolver::NodeResolutionMode; @@ -223,9 +224,7 @@ pub fn op_require_resolve_deno_dir( resolver .resolve_package_folder_from_package( &request, - &ModuleSpecifier::from_file_path(&parent_filename).map_err(|_| { - anyhow!("Url::from_file_path: [{:?}]", parent_filename) - })?, + &url_from_file_path(&PathBuf::from(parent_filename))?, ) .ok() .map(|p| p.to_string_lossy().into_owned()), @@ -571,15 +570,35 @@ where pub fn op_require_module_format

( state: &mut OpState, #[string] filename: String, -) -> Result, node_resolver::errors::ClosestPkgJsonError> +) -> Result, node_resolver::errors::ClosestPkgJsonError> where P: NodePermissions + 'static, { let filename = PathBuf::from(filename); - let url = ModuleSpecifier::from_file_path(&filename) - .map_err(|_| anyhow!("Url::from_file_path: [{:?}]", filename))?; let pkg_json_resolver = state.borrow::(); - pkg_json_resolver.get_closest_package_json_from_path(&filename) + let pkg_json = + pkg_json_resolver.get_closest_package_json_from_path(&filename)?; + let Some(pkg_json) = pkg_json else { + return Ok(None); + }; + match pkg_json.typ.as_str() { + "commonjs" => Ok(Some("commonjs")), + "module" => Ok(Some("module")), + "none" => { + let resolver = state.borrow::(); + match deno_path_util::url_from_file_path(&filename) { + Ok(specifier) => { + if resolver.in_npm_package(&specifier) { + Ok(None) + } else { + Ok(Some("module")) + } + } + Err(_) => Ok(Some("module")), + } + } + _ => Ok(None), + } } #[op2] diff --git a/ext/node/polyfills/01_require.js b/ext/node/polyfills/01_require.js index 3334df9e8bb5c7..bacd88090d0a25 100644 --- a/ext/node/polyfills/01_require.js +++ b/ext/node/polyfills/01_require.js @@ -1060,33 +1060,13 @@ Module.prototype._compile = function (content, filename, format) { return result; }; -Module._extensions[".js"] = function (module, filename) { - const content = op_require_read_file(filename); - - let format; - if (StringPrototypeEndsWith(filename, ".cjs")) { - format = "commonjs"; - } else { - format = op_require_module_format(filename); - } - - module._compile(content, filename, format); -}; - -Module._extensions[".ts"] = +Module._extensions[".js"] = + Module._extensions[".ts"] = Module._extensions[".jsx"] = Module._extensions[".tsx"] = function (module, filename) { const content = op_require_read_file(filename); - - let format; - const pkg = op_require_read_closest_package_json(filename); - if (pkg?.type === "module") { - format = "module"; - } else if (pkg?.type === "commonjs") { - format = "commonjs"; - } - + const format = op_require_module_format(filename); module._compile(content, filename, format); }; diff --git a/resolvers/node/lib.rs b/resolvers/node/lib.rs index e42404c4390242..18b0a85363f456 100644 --- a/resolvers/node/lib.rs +++ b/resolvers/node/lib.rs @@ -22,7 +22,6 @@ pub use package_json::PackageJsonResolverRc; pub use package_json::PackageJsonThreadLocalCache; pub use path::PathClean; pub use resolution::parse_npm_pkg_name; -pub use resolution::ContentIsEsmAnalyzer; pub use resolution::NodeModuleKind; pub use resolution::NodeResolution; pub use resolution::NodeResolutionMode; diff --git a/resolvers/node/resolution.rs b/resolvers/node/resolution.rs index 2bef719c8ae2a4..d44539e9786ad9 100644 --- a/resolvers/node/resolution.rs +++ b/resolvers/node/resolution.rs @@ -85,11 +85,6 @@ impl NodeResolution { } } -pub trait ContentIsEsmAnalyzer: std::fmt::Debug + Send + Sync { - /// Returns whether the content of the specified module specifier is ESM. - fn analyze_content_is_esm(&self, specifier: &Url) -> Result; -} - #[allow(clippy::disallowed_types)] pub type NodeResolverRc = crate::sync::MaybeArc>; diff --git a/tests/specs/run/jsx_import_source_pragma_with_config_vendor_dir/jsx/vendor/http_localhost_4545/jsx/#jsx-runtime_62ac8.js b/tests/specs/run/jsx_import_source_pragma_with_config_vendor_dir/jsx/vendor/http_localhost_4545/jsx/#jsx-runtime_62ac8.js index c8f0a908db2091..210df24164a6d1 100644 --- a/tests/specs/run/jsx_import_source_pragma_with_config_vendor_dir/jsx/vendor/http_localhost_4545/jsx/#jsx-runtime_62ac8.js +++ b/tests/specs/run/jsx_import_source_pragma_with_config_vendor_dir/jsx/vendor/http_localhost_4545/jsx/#jsx-runtime_62ac8.js @@ -1,11 +1,12 @@ export function jsx( - _type, - _props, - _key, - _source, - _self, -) {} -export const jsxs = jsx; -export const jsxDEV = jsx; -export const Fragment = Symbol("Fragment"); -console.log("imported", import.meta.url); + _type, + _props, + _key, + _source, + _self, + ) {} + export const jsxs = jsx; + export const jsxDEV = jsx; + export const Fragment = Symbol("Fragment"); + console.log("imported", import.meta.url); + \ No newline at end of file diff --git a/tests/specs/run/npm_pkg_requires_esm_js/output.out b/tests/specs/run/npm_pkg_requires_esm_js/output.out index 667920d31f54b3..4f15560403441e 100644 --- a/tests/specs/run/npm_pkg_requires_esm_js/output.out +++ b/tests/specs/run/npm_pkg_requires_esm_js/output.out @@ -10,4 +10,4 @@ console.log(require); or change the file extension to .cjs, or add package.json next to the file with "type": "commonjs" option and pass --unstable-detect-cjs flag. - hint: See https://docs.deno.com/go/commonjs for details + docs: https://docs.deno.com/go/commonjs diff --git a/tests/specs/run/require_esm/main.out b/tests/specs/run/require_esm/main.out index 57b842b345f791..4890e1a492de05 100644 --- a/tests/specs/run/require_esm/main.out +++ b/tests/specs/run/require_esm/main.out @@ -1,13 +1,4 @@ [Module: null prototype] { sync_js: 1 } [Module: null prototype] { sync_mjs: 1 } error: Uncaught (in promise) Error: Top-level await is not allowed in synchronous evaluation - at loadESMFromCJS (node:module:[WILDCARD]) - at Module._compile (node:module:[WILDCARD]) - at Object.Module._extensions..js (node:module:[WILDCARD]) - at Module.load (node:module:[WILDCARD]) - at Function.Module._load (node:module:[WILDCARD]) - at Module.require (node:module:[WILDCARD]) - at require (node:module:[WILDCARD]) - at Object. (file:[WILDCARD]/tests/specs/run/require_esm/main.cjs:[WILDCARD]) - at Object. (file:[WILDCARD]/tests/specs/run/require_esm/main.cjs:[WILDCARD]) - at Module._compile (node:module:[WILDCARD]) + at [WILDCARD] From 72107f3f9d95b5d34bb27b89d33a3c05b9e5e99b Mon Sep 17 00:00:00 2001 From: David Sherret Date: Fri, 8 Nov 2024 12:58:40 -0500 Subject: [PATCH 08/22] stabilize detect-cjs --- cli/args/mod.rs | 4 ++-- cli/factory.rs | 2 +- cli/lsp/resolver.rs | 2 +- cli/resolver.rs | 4 ++-- cli/schemas/config-file.v1.json | 1 - cli/standalone/binary.rs | 2 +- cli/standalone/mod.rs | 2 +- runtime/fmt_errors.rs | 7 +++---- tests/config/deno.json | 5 +---- tests/node_compat/deno.json | 5 +---- tests/specs/compile/detect_cjs/deno.json | 5 ----- .../{detect_cjs => package_json_type}/__test__.jsonc | 0 .../specs/compile/{detect_cjs => package_json_type}/add.js | 0 .../compile/{detect_cjs => package_json_type}/compile.out | 0 .../compile/{detect_cjs => package_json_type}/main.js | 0 .../compile/{detect_cjs => package_json_type}/output.out | 0 .../compile/{detect_cjs => package_json_type}/package.json | 0 .../compile/{detect_cjs => package_json_type}/subtract.ts | 0 tests/specs/run/import_common_js/exports_error.out | 7 +++---- tests/specs/run/import_common_js/module_error.out | 7 +++---- tests/specs/run/import_common_js/require_error.out | 7 +++---- tests/specs/run/npm_pkg_requires_esm_js/output.out | 7 +++---- .../specs/run/package_json_type/commonjs/basic/deno.jsonc | 5 ----- .../run/package_json_type/commonjs/basic/main_mix.out | 7 +++---- tests/specs/run/package_json_type/commonjs/jsx/deno.jsonc | 5 +---- tests/specs/run/package_json_type/none/deno.jsonc | 5 ----- tests/specs/run/package_json_type/none/main_cjs.out | 7 +++---- 27 files changed, 32 insertions(+), 64 deletions(-) delete mode 100644 tests/specs/compile/detect_cjs/deno.json rename tests/specs/compile/{detect_cjs => package_json_type}/__test__.jsonc (100%) rename tests/specs/compile/{detect_cjs => package_json_type}/add.js (100%) rename tests/specs/compile/{detect_cjs => package_json_type}/compile.out (100%) rename tests/specs/compile/{detect_cjs => package_json_type}/main.js (100%) rename tests/specs/compile/{detect_cjs => package_json_type}/output.out (100%) rename tests/specs/compile/{detect_cjs => package_json_type}/package.json (100%) rename tests/specs/compile/{detect_cjs => package_json_type}/subtract.ts (100%) delete mode 100644 tests/specs/run/package_json_type/commonjs/basic/deno.jsonc delete mode 100644 tests/specs/run/package_json_type/none/deno.jsonc diff --git a/cli/args/mod.rs b/cli/args/mod.rs index e19025f8b10cb6..ca4042deec86b1 100644 --- a/cli/args/mod.rs +++ b/cli/args/mod.rs @@ -1589,9 +1589,10 @@ impl CliOptions { || self.workspace().has_unstable("bare-node-builtins") } - pub fn unstable_detect_cjs(&self) -> bool { + pub fn detect_cjs(&self) -> bool { self.flags.unstable_config.detect_cjs || self.workspace().has_unstable("detect-cjs") + || self.workspace().package_jsons().next().is_some() } fn byonm_enabled(&self) -> bool { @@ -1655,7 +1656,6 @@ impl CliOptions { "byonm", "bare-node-builtins", "fmt-component", - "detect-cjs", ]) .collect(); diff --git a/cli/factory.rs b/cli/factory.rs index 4a36c75ba2af12..53ae0aee660cf8 100644 --- a/cli/factory.rs +++ b/cli/factory.rs @@ -795,7 +795,7 @@ impl CliFactory { self.in_npm_pkg_checker()?.clone(), self.pkg_json_resolver().clone(), CjsTrackerOptions { - unstable_detect_cjs: options.unstable_detect_cjs(), + detect_cjs: options.detect_cjs(), }, ))) }) diff --git a/cli/lsp/resolver.rs b/cli/lsp/resolver.rs index f5df24d575b925..76796297f2923c 100644 --- a/cli/lsp/resolver.rs +++ b/cli/lsp/resolver.rs @@ -625,7 +625,7 @@ fn create_cjs_tracker( CjsTrackerOptions { // todo(dsherret): support in the lsp by stabilizing the feature // so that we don't have to pipe the config in here - unstable_detect_cjs: false, + detect_cjs: false, }, )) } diff --git a/cli/resolver.rs b/cli/resolver.rs index 710b97509345a6..9461b19adda2eb 100644 --- a/cli/resolver.rs +++ b/cli/resolver.rs @@ -420,7 +420,7 @@ impl NpmModuleLoader { } pub struct CjsTrackerOptions { - pub unstable_detect_cjs: bool, + pub detect_cjs: bool, } /// Keeps track of what module specifiers were resolved as CJS. @@ -445,7 +445,7 @@ impl CjsTracker { Self { in_npm_pkg_checker, pkg_json_resolver, - unstable_detect_cjs: options.unstable_detect_cjs, + unstable_detect_cjs: options.detect_cjs, known: Default::default(), } } diff --git a/cli/schemas/config-file.v1.json b/cli/schemas/config-file.v1.json index 27c8499ea21108..ed80eb17b12205 100644 --- a/cli/schemas/config-file.v1.json +++ b/cli/schemas/config-file.v1.json @@ -528,7 +528,6 @@ "bare-node-builtins", "byonm", "cron", - "detect-cjs", "ffi", "fs", "fmt-component", diff --git a/cli/standalone/binary.rs b/cli/standalone/binary.rs index 9e265122687f03..0ee5a3e5d1544b 100644 --- a/cli/standalone/binary.rs +++ b/cli/standalone/binary.rs @@ -718,7 +718,7 @@ impl<'a> DenoCompileBinaryWriter<'a> { unstable_config: UnstableConfig { legacy_flag_enabled: false, bare_node_builtins: cli_options.unstable_bare_node_builtins(), - detect_cjs: cli_options.unstable_detect_cjs(), + detect_cjs: cli_options.detect_cjs(), sloppy_imports: cli_options.unstable_sloppy_imports(), features: cli_options.unstable_features(), }, diff --git a/cli/standalone/mod.rs b/cli/standalone/mod.rs index 85610f4c20f1f7..1bfa1684ddec92 100644 --- a/cli/standalone/mod.rs +++ b/cli/standalone/mod.rs @@ -629,7 +629,7 @@ pub async fn run(data: StandaloneData) -> Result { in_npm_pkg_checker.clone(), pkg_json_resolver.clone(), CjsTrackerOptions { - unstable_detect_cjs: metadata.unstable_config.detect_cjs, + detect_cjs: metadata.unstable_config.detect_cjs, }, )); let cache_db = Caches::new(deno_dir_provider.clone()); diff --git a/runtime/fmt_errors.rs b/runtime/fmt_errors.rs index 4cd8a063450e96..28cd702966225d 100644 --- a/runtime/fmt_errors.rs +++ b/runtime/fmt_errors.rs @@ -310,14 +310,13 @@ fn get_suggestions_for_terminal_errors(e: &JsError) -> Vec { { return vec![ FixSuggestion::info_multiline(&[ - cstr!("Deno supports CommonJS modules in .cjs files, or when there's a package.json"), - cstr!("with \"type\": \"commonjs\" option and --unstable-detect-cjs flag is used.") + cstr!("Deno supports CommonJS modules in .cjs files, or when the closest"), + cstr!("package.json has a \"type\": \"commonjs\" option.") ]), FixSuggestion::hint_multiline(&[ "Rewrite this module to ESM,", cstr!("or change the file extension to .cjs,"), - cstr!("or add package.json next to the file with \"type\": \"commonjs\" option"), - cstr!("and pass --unstable-detect-cjs flag."), + cstr!("or add package.json next to the file with \"type\": \"commonjs\" option."), ]), FixSuggestion::docs("https://docs.deno.com/go/commonjs"), ]; diff --git a/tests/config/deno.json b/tests/config/deno.json index cf3920fdb46c26..c1c6f5112007a9 100644 --- a/tests/config/deno.json +++ b/tests/config/deno.json @@ -1,7 +1,4 @@ { "lock": false, - "importMap": "../../import_map.json", - "unstable": [ - "detect-cjs" - ] + "importMap": "../../import_map.json" } diff --git a/tests/node_compat/deno.json b/tests/node_compat/deno.json index 3deb8f047785c4..315d5acda8e732 100644 --- a/tests/node_compat/deno.json +++ b/tests/node_compat/deno.json @@ -1,6 +1,3 @@ { - "importMap": "../../import_map.json", - "unstable": [ - "detect-cjs" - ] + "importMap": "../../import_map.json" } diff --git a/tests/specs/compile/detect_cjs/deno.json b/tests/specs/compile/detect_cjs/deno.json deleted file mode 100644 index 35f64c86f49a8f..00000000000000 --- a/tests/specs/compile/detect_cjs/deno.json +++ /dev/null @@ -1,5 +0,0 @@ -{ - "unstable": [ - "detect-cjs" - ] -} diff --git a/tests/specs/compile/detect_cjs/__test__.jsonc b/tests/specs/compile/package_json_type/__test__.jsonc similarity index 100% rename from tests/specs/compile/detect_cjs/__test__.jsonc rename to tests/specs/compile/package_json_type/__test__.jsonc diff --git a/tests/specs/compile/detect_cjs/add.js b/tests/specs/compile/package_json_type/add.js similarity index 100% rename from tests/specs/compile/detect_cjs/add.js rename to tests/specs/compile/package_json_type/add.js diff --git a/tests/specs/compile/detect_cjs/compile.out b/tests/specs/compile/package_json_type/compile.out similarity index 100% rename from tests/specs/compile/detect_cjs/compile.out rename to tests/specs/compile/package_json_type/compile.out diff --git a/tests/specs/compile/detect_cjs/main.js b/tests/specs/compile/package_json_type/main.js similarity index 100% rename from tests/specs/compile/detect_cjs/main.js rename to tests/specs/compile/package_json_type/main.js diff --git a/tests/specs/compile/detect_cjs/output.out b/tests/specs/compile/package_json_type/output.out similarity index 100% rename from tests/specs/compile/detect_cjs/output.out rename to tests/specs/compile/package_json_type/output.out diff --git a/tests/specs/compile/detect_cjs/package.json b/tests/specs/compile/package_json_type/package.json similarity index 100% rename from tests/specs/compile/detect_cjs/package.json rename to tests/specs/compile/package_json_type/package.json diff --git a/tests/specs/compile/detect_cjs/subtract.ts b/tests/specs/compile/package_json_type/subtract.ts similarity index 100% rename from tests/specs/compile/detect_cjs/subtract.ts rename to tests/specs/compile/package_json_type/subtract.ts diff --git a/tests/specs/run/import_common_js/exports_error.out b/tests/specs/run/import_common_js/exports_error.out index b979cce5c78e65..baa44682be52f6 100644 --- a/tests/specs/run/import_common_js/exports_error.out +++ b/tests/specs/run/import_common_js/exports_error.out @@ -3,10 +3,9 @@ Object.defineProperty(exports, "__esModule", { value: true }); ^ at [WILDCARD]exports_error.js:1:23 - info: Deno supports CommonJS modules in .cjs files, or when there's a package.json - with "type": "commonjs" option and --unstable-detect-cjs flag is used. + info: Deno supports CommonJS modules in .cjs files, or when the closest + package.json has a "type": "commonjs" option. hint: Rewrite this module to ESM, or change the file extension to .cjs, - or add package.json next to the file with "type": "commonjs" option - and pass --unstable-detect-cjs flag. + or add package.json next to the file with "type": "commonjs" option. docs: https://docs.deno.com/go/commonjs diff --git a/tests/specs/run/import_common_js/module_error.out b/tests/specs/run/import_common_js/module_error.out index 654ee838dd13b4..957b19cb1e6857 100644 --- a/tests/specs/run/import_common_js/module_error.out +++ b/tests/specs/run/import_common_js/module_error.out @@ -3,10 +3,9 @@ module.exports = { ^ at [WILDCARD]module_error.js:1:1 - info: Deno supports CommonJS modules in .cjs files, or when there's a package.json - with "type": "commonjs" option and --unstable-detect-cjs flag is used. + info: Deno supports CommonJS modules in .cjs files, or when the closest + package.json has a "type": "commonjs" option. hint: Rewrite this module to ESM, or change the file extension to .cjs, - or add package.json next to the file with "type": "commonjs" option - and pass --unstable-detect-cjs flag. + or add package.json next to the file with "type": "commonjs" option. docs: https://docs.deno.com/go/commonjs diff --git a/tests/specs/run/import_common_js/require_error.out b/tests/specs/run/import_common_js/require_error.out index 81ffd6591f570e..e13db85e8e85df 100644 --- a/tests/specs/run/import_common_js/require_error.out +++ b/tests/specs/run/import_common_js/require_error.out @@ -3,10 +3,9 @@ const process = require("process"); ^ at [WILDCARD]require_error.js:1:17 - info: Deno supports CommonJS modules in .cjs files, or when there's a package.json - with "type": "commonjs" option and --unstable-detect-cjs flag is used. + info: Deno supports CommonJS modules in .cjs files, or when the closest + package.json has a "type": "commonjs" option. hint: Rewrite this module to ESM, or change the file extension to .cjs, - or add package.json next to the file with "type": "commonjs" option - and pass --unstable-detect-cjs flag. + or add package.json next to the file with "type": "commonjs" option. docs: https://docs.deno.com/go/commonjs diff --git a/tests/specs/run/npm_pkg_requires_esm_js/output.out b/tests/specs/run/npm_pkg_requires_esm_js/output.out index 4f15560403441e..2cae7108b073d8 100644 --- a/tests/specs/run/npm_pkg_requires_esm_js/output.out +++ b/tests/specs/run/npm_pkg_requires_esm_js/output.out @@ -4,10 +4,9 @@ console.log(require); ^ at [WILDCARD] - info: Deno supports CommonJS modules in .cjs files, or when there's a package.json - with "type": "commonjs" option and --unstable-detect-cjs flag is used. + info: Deno supports CommonJS modules in .cjs files, or when the closest + package.json has a "type": "commonjs" option. hint: Rewrite this module to ESM, or change the file extension to .cjs, - or add package.json next to the file with "type": "commonjs" option - and pass --unstable-detect-cjs flag. + or add package.json next to the file with "type": "commonjs" option. docs: https://docs.deno.com/go/commonjs diff --git a/tests/specs/run/package_json_type/commonjs/basic/deno.jsonc b/tests/specs/run/package_json_type/commonjs/basic/deno.jsonc deleted file mode 100644 index 35f64c86f49a8f..00000000000000 --- a/tests/specs/run/package_json_type/commonjs/basic/deno.jsonc +++ /dev/null @@ -1,5 +0,0 @@ -{ - "unstable": [ - "detect-cjs" - ] -} diff --git a/tests/specs/run/package_json_type/commonjs/basic/main_mix.out b/tests/specs/run/package_json_type/commonjs/basic/main_mix.out index 78f421644cd519..65671fd618a9a2 100644 --- a/tests/specs/run/package_json_type/commonjs/basic/main_mix.out +++ b/tests/specs/run/package_json_type/commonjs/basic/main_mix.out @@ -4,10 +4,9 @@ console.log(require("./add").add(1, 2)); ^ at file:///[WILDLINE]main_mix.js:[WILDLINE] - info: Deno supports CommonJS modules in .cjs files, or when there's a package.json - with "type": "commonjs" option and --unstable-detect-cjs flag is used. + info: Deno supports CommonJS modules in .cjs files, or when the closest + package.json has a "type": "commonjs" option. hint: Rewrite this module to ESM, or change the file extension to .cjs, - or add package.json next to the file with "type": "commonjs" option - and pass --unstable-detect-cjs flag. + or add package.json next to the file with "type": "commonjs" option. docs: https://docs.deno.com/go/commonjs diff --git a/tests/specs/run/package_json_type/commonjs/jsx/deno.jsonc b/tests/specs/run/package_json_type/commonjs/jsx/deno.jsonc index 192ddb98c4130f..31d05ffb7482e1 100644 --- a/tests/specs/run/package_json_type/commonjs/jsx/deno.jsonc +++ b/tests/specs/run/package_json_type/commonjs/jsx/deno.jsonc @@ -3,8 +3,5 @@ "compilerOptions": { "jsx": "react-jsx", "jsxImportSource": "react" - }, - "unstable": [ - "detect-cjs" - ] + } } diff --git a/tests/specs/run/package_json_type/none/deno.jsonc b/tests/specs/run/package_json_type/none/deno.jsonc deleted file mode 100644 index 35f64c86f49a8f..00000000000000 --- a/tests/specs/run/package_json_type/none/deno.jsonc +++ /dev/null @@ -1,5 +0,0 @@ -{ - "unstable": [ - "detect-cjs" - ] -} diff --git a/tests/specs/run/package_json_type/none/main_cjs.out b/tests/specs/run/package_json_type/none/main_cjs.out index 8d34808fb113f9..afa5028f4f814f 100644 --- a/tests/specs/run/package_json_type/none/main_cjs.out +++ b/tests/specs/run/package_json_type/none/main_cjs.out @@ -3,10 +3,9 @@ const { add } = require("./add"); ^ at file:///[WILDLINE] - info: Deno supports CommonJS modules in .cjs files, or when there's a package.json - with "type": "commonjs" option and --unstable-detect-cjs flag is used. + info: Deno supports CommonJS modules in .cjs files, or when the closest + package.json has a "type": "commonjs" option. hint: Rewrite this module to ESM, or change the file extension to .cjs, - or add package.json next to the file with "type": "commonjs" option - and pass --unstable-detect-cjs flag. + or add package.json next to the file with "type": "commonjs" option. docs: https://docs.deno.com/go/commonjs From 848dc745cb186ad2eb753ef55c0eeddad06c1f5d Mon Sep 17 00:00:00 2001 From: David Sherret Date: Fri, 8 Nov 2024 13:07:18 -0500 Subject: [PATCH 09/22] Some fixes for detect-cjs --- cli/args/flags.rs | 2 -- cli/args/mod.rs | 4 +--- cli/standalone/binary.rs | 1 - cli/standalone/mod.rs | 2 +- tests/specs/npm/permissions_outside_package/__test__.jsonc | 3 ++- tests/specs/npm/permissions_outside_package/package.json | 2 ++ 6 files changed, 6 insertions(+), 8 deletions(-) create mode 100644 tests/specs/npm/permissions_outside_package/package.json diff --git a/cli/args/flags.rs b/cli/args/flags.rs index eb77971748a335..37f58993729b8b 100644 --- a/cli/args/flags.rs +++ b/cli/args/flags.rs @@ -576,7 +576,6 @@ pub struct UnstableConfig { // TODO(bartlomieju): remove in Deno 2.5 pub legacy_flag_enabled: bool, // --unstable pub bare_node_builtins: bool, - pub detect_cjs: bool, pub sloppy_imports: bool, pub features: Vec, // --unstabe-kv --unstable-cron } @@ -5720,7 +5719,6 @@ fn unstable_args_parse( flags.unstable_config.bare_node_builtins = matches.get_flag("unstable-bare-node-builtins"); - flags.unstable_config.detect_cjs = matches.get_flag("unstable-detect-cjs"); flags.unstable_config.sloppy_imports = matches.get_flag("unstable-sloppy-imports"); diff --git a/cli/args/mod.rs b/cli/args/mod.rs index ca4042deec86b1..e2e6fe88718155 100644 --- a/cli/args/mod.rs +++ b/cli/args/mod.rs @@ -1590,9 +1590,7 @@ impl CliOptions { } pub fn detect_cjs(&self) -> bool { - self.flags.unstable_config.detect_cjs - || self.workspace().has_unstable("detect-cjs") - || self.workspace().package_jsons().next().is_some() + self.workspace().package_jsons().next().is_some() } fn byonm_enabled(&self) -> bool { diff --git a/cli/standalone/binary.rs b/cli/standalone/binary.rs index 0ee5a3e5d1544b..1762571249524b 100644 --- a/cli/standalone/binary.rs +++ b/cli/standalone/binary.rs @@ -718,7 +718,6 @@ impl<'a> DenoCompileBinaryWriter<'a> { unstable_config: UnstableConfig { legacy_flag_enabled: false, bare_node_builtins: cli_options.unstable_bare_node_builtins(), - detect_cjs: cli_options.detect_cjs(), sloppy_imports: cli_options.unstable_sloppy_imports(), features: cli_options.unstable_features(), }, diff --git a/cli/standalone/mod.rs b/cli/standalone/mod.rs index 1bfa1684ddec92..774dda62f0d285 100644 --- a/cli/standalone/mod.rs +++ b/cli/standalone/mod.rs @@ -629,7 +629,7 @@ pub async fn run(data: StandaloneData) -> Result { in_npm_pkg_checker.clone(), pkg_json_resolver.clone(), CjsTrackerOptions { - detect_cjs: metadata.unstable_config.detect_cjs, + detect_cjs: !metadata.workspace_resolver.package_jsons.is_empty(), }, )); let cache_db = Caches::new(deno_dir_provider.clone()); diff --git a/tests/specs/npm/permissions_outside_package/__test__.jsonc b/tests/specs/npm/permissions_outside_package/__test__.jsonc index 727f5b75947f6e..d5f6bf49081153 100644 --- a/tests/specs/npm/permissions_outside_package/__test__.jsonc +++ b/tests/specs/npm/permissions_outside_package/__test__.jsonc @@ -1,4 +1,5 @@ { - "args": "run --allow-read --unstable-detect-cjs main.ts", + "tempDir": true, + "args": "run --allow-read --node-modules-dir=none main.ts", "output": "main.out" } diff --git a/tests/specs/npm/permissions_outside_package/package.json b/tests/specs/npm/permissions_outside_package/package.json new file mode 100644 index 00000000000000..2c63c0851048d8 --- /dev/null +++ b/tests/specs/npm/permissions_outside_package/package.json @@ -0,0 +1,2 @@ +{ +} From 9f4457609fa1864d5086b2340621be8e5646bf33 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Fri, 8 Nov 2024 14:44:34 -0500 Subject: [PATCH 10/22] remove npm main --- cli/args/mod.rs | 8 -------- cli/factory.rs | 1 - cli/module_loader.rs | 17 +++++------------ cli/node.rs | 12 ++---------- cli/standalone/mod.rs | 1 - 5 files changed, 7 insertions(+), 32 deletions(-) diff --git a/cli/args/mod.rs b/cli/args/mod.rs index e2e6fe88718155..4d87fd257eb965 100644 --- a/cli/args/mod.rs +++ b/cli/args/mod.rs @@ -1207,14 +1207,6 @@ impl CliOptions { } } - // If the main module should be treated as being in an npm package. - // This is triggered via a secret environment variable which is used - // for functionality like child_process.fork. Users should NOT depend - // on this functionality. - pub fn is_npm_main(&self) -> bool { - NPM_PROCESS_STATE.is_some() - } - pub fn has_node_modules_dir(&self) -> bool { self.maybe_node_modules_folder.is_some() } diff --git a/cli/factory.rs b/cli/factory.rs index 53ae0aee660cf8..b6ec0d109c99ab 100644 --- a/cli/factory.rs +++ b/cli/factory.rs @@ -652,7 +652,6 @@ impl CliFactory { self.cjs_tracker()?.clone(), self.fs().clone(), Some(self.parsed_source_cache().clone()), - self.cli_options()?.is_npm_main(), ); Ok(Arc::new(NodeCodeTranslator::new( diff --git a/cli/module_loader.rs b/cli/module_loader.rs index 43c9e1aa0750e0..dbcfd8b6c8c6d3 100644 --- a/cli/module_loader.rs +++ b/cli/module_loader.rs @@ -206,7 +206,6 @@ struct SharedCliModuleLoaderState { lib_worker: TsTypeLib, initial_cwd: PathBuf, is_inspecting: bool, - is_npm_main: bool, is_repl: bool, cjs_tracker: Arc, code_cache: Option>, @@ -252,7 +251,6 @@ impl CliModuleLoaderFactory { lib_worker: options.ts_type_lib_worker(), initial_cwd: options.initial_cwd().to_path_buf(), is_inspecting: options.is_inspecting(), - is_npm_main: options.is_npm_main(), is_repl: matches!( options.sub_command(), DenoSubcommand::Repl(_) | DenoSubcommand::Jupyter(_) @@ -286,7 +284,6 @@ impl CliModuleLoaderFactory { Rc::new(CliModuleLoader(Rc::new(CliModuleLoaderInner { lib, is_worker, - is_npm_main: self.shared.is_npm_main, parent_permissions, permissions, graph_container: graph_container.clone(), @@ -343,7 +340,6 @@ impl ModuleLoaderFactory for CliModuleLoaderFactory { struct CliModuleLoaderInner { lib: TsTypeLib, - is_npm_main: bool, is_worker: bool, /// The initial set of permissions used to resolve the static imports in the /// worker. These are "allow all" for main worker, and parent thread @@ -668,14 +664,11 @@ impl is_script, .. })) => { - // todo(dsherret): revert in https://github.com/denoland/deno/pull/26439 - if self.is_npm_main && *is_script - || self.shared.cjs_tracker.is_cjs_with_known_is_script( - specifier, - *media_type, - *is_script, - )? - { + if self.shared.cjs_tracker.is_cjs_with_known_is_script( + specifier, + *media_type, + *is_script, + )? { return Ok(Some(CodeOrDeferredEmit::Cjs { specifier, media_type: *media_type, diff --git a/cli/node.rs b/cli/node.rs index 1d410a726af601..8235745a9114c3 100644 --- a/cli/node.rs +++ b/cli/node.rs @@ -62,10 +62,6 @@ pub struct CliCjsCodeAnalyzer { cjs_tracker: Arc, fs: deno_fs::FileSystemRc, parsed_source_cache: Option>, - // todo(dsherret): hack, remove in https://github.com/denoland/deno/pull/26439 - // For example, this does not properly handle if cjs analysis was already done - // and has been cached. - is_npm_main: bool, } impl CliCjsCodeAnalyzer { @@ -74,14 +70,12 @@ impl CliCjsCodeAnalyzer { cjs_tracker: Arc, fs: deno_fs::FileSystemRc, parsed_source_cache: Option>, - is_npm_main: bool, ) -> Self { Self { cache, cjs_tracker, fs, parsed_source_cache, - is_npm_main, } } @@ -106,9 +100,7 @@ impl CliCjsCodeAnalyzer { } let cjs_tracker = self.cjs_tracker.clone(); - let is_npm_main = self.is_npm_main; - let is_maybe_cjs = - cjs_tracker.is_maybe_cjs(specifier, media_type)? || is_npm_main; + let is_maybe_cjs = cjs_tracker.is_maybe_cjs(specifier, media_type)?; let analysis = if is_maybe_cjs { let maybe_parsed_source = self .parsed_source_cache @@ -135,7 +127,7 @@ impl CliCjsCodeAnalyzer { parsed_source.specifier(), media_type, is_script, - )? || is_script && is_npm_main; + )?; if is_cjs { let analysis = parsed_source.analyze_cjs(); Ok(CliCjsAnalysis::Cjs { diff --git a/cli/standalone/mod.rs b/cli/standalone/mod.rs index 774dda62f0d285..c9413b02ca7b9d 100644 --- a/cli/standalone/mod.rs +++ b/cli/standalone/mod.rs @@ -646,7 +646,6 @@ pub async fn run(data: StandaloneData) -> Result { cjs_tracker.clone(), fs.clone(), None, - false, ); let node_code_translator = Arc::new(NodeCodeTranslator::new( cjs_esm_code_analyzer, From d509fda334bc8b14c9dfd8aeab9e8ddcac3ef64e Mon Sep 17 00:00:00 2001 From: David Sherret Date: Fri, 8 Nov 2024 14:57:57 -0500 Subject: [PATCH 11/22] Fix. --- .../jsx/#jsx-runtime_62ac8.js | 21 +++++++++--------- tests/util/server/src/servers/mod.rs | 22 +++++++++---------- 2 files changed, 21 insertions(+), 22 deletions(-) diff --git a/tests/specs/run/jsx_import_source_pragma_with_config_vendor_dir/jsx/vendor/http_localhost_4545/jsx/#jsx-runtime_62ac8.js b/tests/specs/run/jsx_import_source_pragma_with_config_vendor_dir/jsx/vendor/http_localhost_4545/jsx/#jsx-runtime_62ac8.js index 210df24164a6d1..c8f0a908db2091 100644 --- a/tests/specs/run/jsx_import_source_pragma_with_config_vendor_dir/jsx/vendor/http_localhost_4545/jsx/#jsx-runtime_62ac8.js +++ b/tests/specs/run/jsx_import_source_pragma_with_config_vendor_dir/jsx/vendor/http_localhost_4545/jsx/#jsx-runtime_62ac8.js @@ -1,12 +1,11 @@ export function jsx( - _type, - _props, - _key, - _source, - _self, - ) {} - export const jsxs = jsx; - export const jsxDEV = jsx; - export const Fragment = Symbol("Fragment"); - console.log("imported", import.meta.url); - \ No newline at end of file + _type, + _props, + _key, + _source, + _self, +) {} +export const jsxs = jsx; +export const jsxDEV = jsx; +export const Fragment = Symbol("Fragment"); +console.log("imported", import.meta.url); diff --git a/tests/util/server/src/servers/mod.rs b/tests/util/server/src/servers/mod.rs index d9adde542033fe..0b1d99aeb92751 100644 --- a/tests/util/server/src/servers/mod.rs +++ b/tests/util/server/src/servers/mod.rs @@ -807,17 +807,17 @@ async fn main_server( (_, "/jsx/jsx-runtime") | (_, "/jsx/jsx-dev-runtime") => { let mut res = Response::new(string_body( r#"export function jsx( - _type, - _props, - _key, - _source, - _self, - ) {} - export const jsxs = jsx; - export const jsxDEV = jsx; - export const Fragment = Symbol("Fragment"); - console.log("imported", import.meta.url); - "#, + _type, + _props, + _key, + _source, + _self, +) {} +export const jsxs = jsx; +export const jsxDEV = jsx; +export const Fragment = Symbol("Fragment"); +console.log("imported", import.meta.url); +"#, )); res.headers_mut().insert( "Content-type", From 16b3b4a5f9437f65a711f838ca9a2ccf8388fb2e Mon Sep 17 00:00:00 2001 From: David Sherret Date: Mon, 11 Nov 2024 11:38:57 -0500 Subject: [PATCH 12/22] Starting to separate out cjs tracker, but realizing that graph resolver needs to go --- cli/emit.rs | 8 +- cli/factory.rs | 3 +- cli/lsp/diagnostics.rs | 7 +- cli/lsp/documents.rs | 29 ++--- cli/lsp/language_server.rs | 5 + cli/lsp/resolver.rs | 113 ++++++++++++------- cli/module_loader.rs | 10 +- cli/resolver.rs | 130 ++++++++++++++-------- cli/standalone/mod.rs | 22 +++- cli/tools/lint/rules/no_sloppy_imports.rs | 1 + cli/tools/run/hmr.rs | 7 -- cli/tsc/99_main_compiler.js | 41 +++---- cli/tsc/mod.rs | 59 +++++----- cli/worker.rs | 2 + resolvers/node/package_json.rs | 4 +- resolvers/node/resolution.rs | 4 +- runtime/web_worker.rs | 7 ++ 17 files changed, 283 insertions(+), 169 deletions(-) diff --git a/cli/emit.rs b/cli/emit.rs index 8c4f2091cf021f..3cd23b7abbcaa6 100644 --- a/cli/emit.rs +++ b/cli/emit.rs @@ -181,7 +181,6 @@ impl Emitter { pub async fn load_and_emit_for_hmr( &self, specifier: &ModuleSpecifier, - module_kind: deno_ast::ModuleKind, ) -> Result { let media_type = MediaType::from_specifier(specifier); let source_code = tokio::fs::read_to_string( @@ -203,11 +202,16 @@ impl Emitter { // this statement is probably wrong) let mut options = self.transpile_and_emit_options.1.clone(); options.source_map = SourceMapOption::None; + let is_cjs = self.cjs_tracker.is_cjs_with_known_is_script( + specifier, + media_type, + parsed_source.compute_is_script(), + )?; let transpiled_source = parsed_source .transpile( &self.transpile_and_emit_options.0, &deno_ast::TranspileModuleOptions { - module_kind: Some(module_kind), + module_kind: Some(ModuleKind::from_is_cjs(is_cjs)), }, &options, )? diff --git a/cli/factory.rs b/cli/factory.rs index b6ec0d109c99ab..1adf2639ad9af0 100644 --- a/cli/factory.rs +++ b/cli/factory.rs @@ -531,6 +531,7 @@ impl CliFactory { async { let cli_options = self.cli_options()?; Ok(Arc::new(CliGraphResolver::new(CliGraphResolverOptions { + cjs_tracker: self.cjs_tracker()?.clone(), sloppy_imports_resolver: self.sloppy_imports_resolver()?.cloned(), node_resolver: Some(self.cli_node_resolver().await?.clone()), npm_resolver: if cli_options.no_npm() { @@ -948,10 +949,8 @@ impl CliFactory { let create_hmr_runner = if cli_options.has_hmr() { let watcher_communicator = self.watcher_communicator.clone().unwrap(); let emitter = self.emitter()?.clone(); - let cjs_tracker = self.cjs_tracker()?.clone(); let fn_: crate::worker::CreateHmrRunnerCb = Box::new(move |session| { Box::new(HmrRunner::new( - cjs_tracker.clone(), emitter.clone(), session, watcher_communicator.clone(), diff --git a/cli/lsp/diagnostics.rs b/cli/lsp/diagnostics.rs index 83c00d27edadea..a23851e0d2e8bf 100644 --- a/cli/lsp/diagnostics.rs +++ b/cli/lsp/diagnostics.rs @@ -1638,6 +1638,7 @@ mod tests { use crate::lsp::documents::Documents; use crate::lsp::documents::LanguageId; use crate::lsp::language_server::StateSnapshot; + use crate::lsp::resolver::LspCjsTracker; use crate::lsp::resolver::LspResolver; use deno_config::deno_json::ConfigFile; @@ -1686,8 +1687,10 @@ mod tests { .unwrap(); config.tree.inject_config_file(config_file).await; } - let resolver = - Arc::new(LspResolver::from_config(&config, &cache, None).await); + let cjs_tracker = Arc::new(LspCjsTracker::new(&cache)); + let resolver = Arc::new( + LspResolver::from_config(&config, &cache, cjs_tracker, None).await, + ); let mut documents = Documents::default(); documents.update_config(&config, &resolver, &cache, &Default::default()); for (relative_path, source, version, language_id) in sources { diff --git a/cli/lsp/documents.rs b/cli/lsp/documents.rs index ce13c32157b119..5f1544042dde51 100644 --- a/cli/lsp/documents.rs +++ b/cli/lsp/documents.rs @@ -1281,6 +1281,9 @@ impl Documents { self.config = Arc::new(config.clone()); self.cache = Arc::new(cache.clone()); self.resolver = resolver.clone(); + + node_resolver::PackageJsonThreadLocalCache::clear(); + { let fs_docs = &self.file_system_docs; // Clean up non-existent documents. @@ -1541,20 +1544,18 @@ fn analyze_module( match parsed_source_result { Ok(parsed_source) => { let npm_resolver = resolver.create_graph_npm_resolver(file_referrer); - Ok(deno_graph::parse_module_from_ast( - deno_graph::ParseModuleFromAstOptions { - graph_kind: deno_graph::GraphKind::TypesOnly, - specifier, - maybe_headers, - parsed_source, - // use a null file system because there's no need to bother resolving - // dynamic imports like import(`./dir/${something}`) in the LSP - file_system: &deno_graph::source::NullFileSystem, - jsr_url_provider: &CliJsrUrlProvider, - maybe_resolver: Some(resolver.as_graph_resolver(file_referrer)), - maybe_npm_resolver: Some(&npm_resolver), - }, - )) + deno_graph::parse_module_from_ast(deno_graph::ParseModuleFromAstOptions { + graph_kind: deno_graph::GraphKind::TypesOnly, + specifier, + maybe_headers, + parsed_source, + // use a null file system because there's no need to bother resolving + // dynamic imports like import(`./dir/${something}`) in the LSP + file_system: &deno_graph::source::NullFileSystem, + jsr_url_provider: &CliJsrUrlProvider, + maybe_resolver: Some(resolver.as_graph_resolver(file_referrer)), + maybe_npm_resolver: Some(&npm_resolver), + }) } Err(err) => Err(deno_graph::ModuleGraphError::ModuleError( deno_graph::ModuleError::ParseErr(specifier, err.clone()), diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index 2554fa34b1853f..3aa8195dfe5bf1 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -77,6 +77,7 @@ use super::parent_process_checker; use super::performance::Performance; use super::refactor; use super::registries::ModuleRegistry; +use super::resolver::LspCjsTracker; use super::resolver::LspResolver; use super::testing; use super::text; @@ -194,6 +195,7 @@ pub struct Inner { cache: LspCache, /// The LSP client that this LSP server is connected to. pub client: Client, + cjs_tracker: Arc, /// Configuration information. pub config: Config, diagnostics_state: Arc, @@ -462,6 +464,7 @@ impl Inner { cache.deno_dir().registries_folder_path(), http_client_provider.clone(), ); + let cjs_tracker = Arc::new(LspCjsTracker::new(&cache)); let jsr_search_api = CliJsrSearchApi::new(module_registry.file_fetcher.clone()); let npm_search_api = @@ -484,6 +487,7 @@ impl Inner { Self { assets, cache, + cjs_tracker, client, config, diagnostics_state, @@ -1011,6 +1015,7 @@ impl Inner { LspResolver::from_config( &self.config, &self.cache, + self.cjs_tracker.clone(), Some(&self.http_client_provider), ) .await, diff --git a/cli/lsp/resolver.rs b/cli/lsp/resolver.rs index 76796297f2923c..66d7ae80a2cbc8 100644 --- a/cli/lsp/resolver.rs +++ b/cli/lsp/resolver.rs @@ -1,6 +1,7 @@ // Copyright 2018-2024 the Deno authors. All rights reserved. MIT license. use dashmap::DashMap; +use deno_ast::view::Module; use deno_ast::MediaType; use deno_ast::ParsedSource; use deno_cache_dir::npm::NpmCacheDir; @@ -12,6 +13,7 @@ use deno_graph::source::Resolver; use deno_graph::GraphImport; use deno_graph::ModuleSpecifier; use deno_npm::NpmSystemInfo; +use deno_path_util::url_from_directory_path; use deno_path_util::url_to_file_path; use deno_runtime::deno_fs; use deno_runtime::deno_node::NodeResolver; @@ -24,6 +26,7 @@ use deno_semver::package::PackageReq; use indexmap::IndexMap; use node_resolver::errors::ClosestPkgJsonError; use node_resolver::InNpmPackageChecker; +use node_resolver::NodeModuleKind; use node_resolver::NodeResolutionMode; use std::borrow::Cow; use std::collections::BTreeMap; @@ -66,7 +69,7 @@ use crate::util::progress_bar::ProgressBarStyle; #[derive(Debug, Clone)] struct LspScopeResolver { - cjs_tracker: Option>, + cjs_tracker: Arc, graph_resolver: Arc, jsr_resolver: Option>, npm_resolver: Option>, @@ -80,9 +83,11 @@ struct LspScopeResolver { impl Default for LspScopeResolver { fn default() -> Self { + let cjs_tracker = Arc::new(LspCjsTracker::new(&LspCache::default())); + let graph_resolver = create_graph_resolver(&cjs_tracker, None, None, None); Self { - cjs_tracker: None, - graph_resolver: create_graph_resolver(None, None, None), + cjs_tracker, + graph_resolver, jsr_resolver: None, npm_resolver: None, node_resolver: None, @@ -99,6 +104,7 @@ impl LspScopeResolver { async fn from_config_data( config_data: Option<&Arc>, cache: &LspCache, + cjs_tracker: Arc, http_client_provider: Option<&Arc>, ) -> Self { let mut npm_resolver = None; @@ -118,14 +124,7 @@ impl LspScopeResolver { .await; if let Some(npm_resolver) = &npm_resolver { let in_npm_pkg_checker = create_in_npm_pkg_checker(npm_resolver); - let cjs_tracker = create_cjs_tracker( - in_npm_pkg_checker.clone(), - pkg_json_resolver.clone(), - ); - lsp_cjs_tracker = - Some(Arc::new(LspCjsTracker::new(cjs_tracker.clone()))); node_resolver = Some(create_node_resolver( - cjs_tracker, fs.clone(), in_npm_pkg_checker, npm_resolver, @@ -134,6 +133,7 @@ impl LspScopeResolver { } } let graph_resolver = create_graph_resolver( + cjs_tracker.inner(), config_data.map(|d| d.as_ref()), npm_resolver.as_ref(), node_resolver.as_ref(), @@ -182,6 +182,7 @@ impl LspScopeResolver { .resolve_req_reference( &req_ref, &referrer, + referrer_kind, NodeResolutionMode::Types, ) .ok()?, @@ -195,7 +196,7 @@ impl LspScopeResolver { let package_json_deps_by_resolution = Arc::new(package_json_deps_by_resolution.unwrap_or_default()); Self { - cjs_tracker: lsp_cjs_tracker, + cjs_tracker, graph_resolver, jsr_resolver, npm_resolver, @@ -216,16 +217,9 @@ impl LspScopeResolver { deno_runtime::deno_node::DenoFsNodeResolverEnv::new(fs.clone()), )); let mut node_resolver = None; - let mut lsp_cjs_tracker = None; if let Some(npm_resolver) = &npm_resolver { let in_npm_pkg_checker = create_in_npm_pkg_checker(npm_resolver); - let cjs_tracker = create_cjs_tracker( - in_npm_pkg_checker.clone(), - pkg_json_resolver.clone(), - ); - lsp_cjs_tracker = Some(Arc::new(LspCjsTracker::new(cjs_tracker.clone()))); node_resolver = Some(create_node_resolver( - cjs_tracker, fs, in_npm_pkg_checker, npm_resolver, @@ -238,7 +232,7 @@ impl LspScopeResolver { node_resolver.as_ref(), ); Arc::new(Self { - cjs_tracker: lsp_cjs_tracker, + cjs_tracker: self.cjs_tracker.clone(), graph_resolver, jsr_resolver: self.jsr_resolver.clone(), npm_resolver, @@ -264,6 +258,7 @@ impl LspResolver { pub async fn from_config( config: &Config, cache: &LspCache, + cjs_tracker: Arc, http_client_provider: Option<&Arc>, ) -> Self { let mut by_scope = BTreeMap::new(); @@ -274,6 +269,7 @@ impl LspResolver { LspScopeResolver::from_config_data( Some(config_data), cache, + cjs_tracker.clone(), http_client_provider, ) .await, @@ -282,8 +278,13 @@ impl LspResolver { } Self { unscoped: Arc::new( - LspScopeResolver::from_config_data(None, cache, http_client_provider) - .await, + LspScopeResolver::from_config_data( + None, + cache, + cjs_tracker, + http_client_provider, + ) + .await, ), by_scope, } @@ -350,14 +351,6 @@ impl LspResolver { resolver.graph_resolver.create_graph_npm_resolver() } - pub fn maybe_cjs_tracker( - &self, - file_referrer: Option<&ModuleSpecifier>, - ) -> Option<&Arc> { - let resolver = self.get_scope_resolver(file_referrer); - resolver.cjs_tracker.as_ref() - } - pub fn maybe_node_resolver( &self, file_referrer: Option<&ModuleSpecifier>, @@ -478,6 +471,7 @@ impl LspResolver { &self, specifier_text: &str, referrer: &ModuleSpecifier, + referrer_kind: NodeModuleKind, ) -> bool { let resolver = self.get_scope_resolver(Some(referrer)); let Some(node_resolver) = resolver.node_resolver.as_ref() else { @@ -487,6 +481,7 @@ impl LspResolver { .resolve_if_for_npm_pkg( specifier_text, referrer, + referrer_kind, NodeResolutionMode::Types, ) .ok() @@ -619,15 +614,6 @@ fn create_cjs_tracker( in_npm_pkg_checker: Arc, pkg_json_resolver: Arc, ) -> Arc { - Arc::new(CjsTracker::new( - in_npm_pkg_checker, - pkg_json_resolver, - CjsTrackerOptions { - // todo(dsherret): support in the lsp by stabilizing the feature - // so that we don't have to pipe the config in here - detect_cjs: false, - }, - )) } fn create_in_npm_pkg_checker( @@ -649,7 +635,6 @@ fn create_in_npm_pkg_checker( } fn create_node_resolver( - cjs_tracker: Arc, fs: Arc, in_npm_pkg_checker: Arc, npm_resolver: &Arc, @@ -662,7 +647,6 @@ fn create_node_resolver( pkg_json_resolver.clone(), )); Arc::new(CliNodeResolver::new( - cjs_tracker.clone(), fs, in_npm_pkg_checker, node_resolver_inner, @@ -671,12 +655,14 @@ fn create_node_resolver( } fn create_graph_resolver( + cjs_tracker: &Arc, config_data: Option<&ConfigData>, npm_resolver: Option<&Arc>, node_resolver: Option<&Arc>, ) -> Arc { let workspace = config_data.map(|d| &d.member_dir.workspace); Arc::new(CliGraphResolver::new(CliGraphResolverOptions { + cjs_tracker: cjs_tracker.clone(), node_resolver: node_resolver.cloned(), npm_resolver: npm_resolver.cloned(), workspace_resolver: config_data.map(|d| d.resolver.clone()).unwrap_or_else( @@ -842,16 +828,59 @@ impl RedirectResolver { } } +#[derive(Debug)] +pub struct LspInNpmPackageChecker { + global_cache_dir: ModuleSpecifier, +} + +impl LspInNpmPackageChecker { + pub fn new(cache: &LspCache) -> Self { + Self { + global_cache_dir: url_from_directory_path( + &cache.deno_dir().npm_folder_path(), + ) + .unwrap_or_else(|_| ModuleSpecifier::parse("file:///invalid/").unwrap()), + } + } +} + +impl InNpmPackageChecker for LspInNpmPackageChecker { + fn in_npm_package(&self, specifier: &ModuleSpecifier) -> bool { + if specifier.scheme() != "file" { + return false; + } + if specifier + .as_str() + .starts_with(self.global_cache_dir.as_str()) + { + return true; + } + specifier.as_str().contains("/node_modules/") + } +} + #[derive(Debug)] pub struct LspCjsTracker { cjs_tracker: Arc, } impl LspCjsTracker { - pub fn new(cjs_tracker: Arc) -> Self { + pub fn new(cache: &LspCache) -> Self { + let fs = Arc::new(deno_fs::RealFs); + let cjs_tracker = Arc::new(CjsTracker::new( + Arc::new(LspInNpmPackageChecker::new(cache)), + Arc::new(PackageJsonResolver::new( + deno_runtime::deno_node::DenoFsNodeResolverEnv::new(fs.clone()), + )), + CjsTrackerOptions { detect_cjs: true }, + )); Self { cjs_tracker } } + pub fn inner(&self) -> &Arc { + &self.cjs_tracker + } + pub fn is_cjs( &self, specifier: &ModuleSpecifier, @@ -861,7 +890,7 @@ impl LspCjsTracker { if let Some(module_kind) = self.cjs_tracker.get_known_kind(specifier, media_type) { - module_kind.is_cjs() + module_kind == NodeModuleKind::Cjs } else { let maybe_is_script = maybe_parsed_source.map(|p| p.compute_is_script()); maybe_is_script diff --git a/cli/module_loader.rs b/cli/module_loader.rs index dbcfd8b6c8c6d3..112717c3ec258c 100644 --- a/cli/module_loader.rs +++ b/cli/module_loader.rs @@ -74,6 +74,7 @@ use deno_runtime::deno_node::NodeRequireLoader; use deno_runtime::deno_permissions::PermissionsContainer; use deno_semver::npm::NpmPackageReqReference; use node_resolver::InNpmPackageChecker; +use node_resolver::NodeModuleKind; use node_resolver::NodeResolutionMode; pub struct ModuleLoadPreparer { @@ -474,7 +475,12 @@ impl self .shared .node_resolver - .resolve(raw_specifier, referrer, NodeResolutionMode::Execution)? + .resolve( + raw_specifier, + referrer, + self.shared.cjs_tracker.get_referrer_kind(referrer), + NodeResolutionMode::Execution, + )? .into_url(), ); } @@ -514,6 +520,7 @@ impl return self.shared.node_resolver.resolve_req_reference( &reference, referrer, + self.shared.cjs_tracker.get_referrer_kind(referrer), NodeResolutionMode::Execution, ); } @@ -534,6 +541,7 @@ impl &package_folder, module.nv_reference.sub_path(), Some(referrer), + self.shared.cjs_tracker.get_referrer_kind(referrer), NodeResolutionMode::Execution, ) .with_context(|| { diff --git a/cli/resolver.rs b/cli/resolver.rs index 9461b19adda2eb..58be27a1ead00d 100644 --- a/cli/resolver.rs +++ b/cli/resolver.rs @@ -4,7 +4,6 @@ use async_trait::async_trait; use dashmap::DashMap; use dashmap::DashSet; use deno_ast::MediaType; -use deno_ast::ModuleKind; use deno_config::workspace::MappedResolution; use deno_config::workspace::MappedResolutionDiagnostic; use deno_config::workspace::MappedResolutionError; @@ -108,7 +107,6 @@ impl deno_resolver::fs::DenoResolverFs for CliDenoResolverFs { #[derive(Debug)] pub struct CliNodeResolver { - cjs_tracker: Arc, fs: Arc, in_npm_pkg_checker: Arc, node_resolver: Arc, @@ -117,14 +115,12 @@ pub struct CliNodeResolver { impl CliNodeResolver { pub fn new( - cjs_tracker: Arc, fs: Arc, in_npm_pkg_checker: Arc, node_resolver: Arc, npm_resolver: Arc, ) -> Self { Self { - cjs_tracker, fs, in_npm_pkg_checker, node_resolver, @@ -140,9 +136,11 @@ impl CliNodeResolver { &self, specifier: &str, referrer: &ModuleSpecifier, + referrer_kind: NodeModuleKind, mode: NodeResolutionMode, ) -> Result, AnyError> { - let resolution_result = self.resolve(specifier, referrer, mode); + let resolution_result = + self.resolve(specifier, referrer, referrer_kind, mode); match resolution_result { Ok(res) => Ok(Some(res)), Err(err) => { @@ -213,35 +211,26 @@ impl CliNodeResolver { &self, specifier: &str, referrer: &ModuleSpecifier, + referrer_kind: NodeModuleKind, mode: NodeResolutionMode, ) -> Result { - let referrer_kind = if self - .cjs_tracker - .is_maybe_cjs(referrer, MediaType::from_specifier(referrer)) - .map_err(|err| NodeResolveErrorKind::PackageResolve(err.into()))? - { - NodeModuleKind::Cjs - } else { - NodeModuleKind::Esm - }; - - let res = - self - .node_resolver - .resolve(specifier, referrer, referrer_kind, mode)?; - Ok(res) + self + .node_resolver + .resolve(specifier, referrer, referrer_kind, mode) } pub fn resolve_req_reference( &self, req_ref: &NpmPackageReqReference, referrer: &ModuleSpecifier, + referrer_kind: NodeModuleKind, mode: NodeResolutionMode, ) -> Result { self.resolve_req_with_sub_path( req_ref.req(), req_ref.sub_path(), referrer, + referrer_kind, mode, ) } @@ -251,6 +240,7 @@ impl CliNodeResolver { req: &PackageReq, sub_path: Option<&str>, referrer: &ModuleSpecifier, + referrer_kind: NodeModuleKind, mode: NodeResolutionMode, ) -> Result { let package_folder = self @@ -260,6 +250,7 @@ impl CliNodeResolver { &package_folder, sub_path, Some(referrer), + referrer_kind, mode, ); match resolution_result { @@ -284,12 +275,14 @@ impl CliNodeResolver { package_folder: &Path, sub_path: Option<&str>, maybe_referrer: Option<&ModuleSpecifier>, + referrer_kind: NodeModuleKind, mode: NodeResolutionMode, ) -> Result { self.node_resolver.resolve_package_subpath_from_deno_module( package_folder, sub_path, maybe_referrer, + referrer_kind, mode, ) } @@ -432,8 +425,8 @@ pub struct CjsTrackerOptions { pub struct CjsTracker { in_npm_pkg_checker: Arc, pkg_json_resolver: Arc, - unstable_detect_cjs: bool, - known: DashMap, + detect_cjs: bool, + known: DashMap, } impl CjsTracker { @@ -445,7 +438,7 @@ impl CjsTracker { Self { in_npm_pkg_checker, pkg_json_resolver, - unstable_detect_cjs: options.detect_cjs, + detect_cjs: options.detect_cjs, known: Default::default(), } } @@ -487,30 +480,46 @@ impl CjsTracker { Some(kind) => kind, None => self.check_based_on_pkg_json(specifier)?, }; - Ok(kind.is_cjs()) + Ok(kind == NodeModuleKind::Cjs) } pub fn get_known_kind( &self, specifier: &ModuleSpecifier, media_type: MediaType, - ) -> Option { + ) -> Option { self.get_known_kind_with_is_script(specifier, media_type, None) } + pub fn get_referrer_kind( + &self, + specifier: &ModuleSpecifier, + ) -> NodeModuleKind { + self + .get_known_kind(specifier, MediaType::from_specifier(specifier)) + .unwrap_or_else(|| { + debug_assert!( + false, + "referrer '{}' should always have a known kind", + specifier + ); + NodeModuleKind::Esm + }) + } + fn get_known_kind_with_is_script( &self, specifier: &ModuleSpecifier, media_type: MediaType, is_script: Option, - ) -> Option { + ) -> Option { if specifier.scheme() != "file" { - return Some(ModuleKind::Esm); + return Some(NodeModuleKind::Esm); } match media_type { - MediaType::Mts | MediaType::Mjs | MediaType::Dmts => Some(ModuleKind::Esm), - MediaType::Cjs | MediaType::Cts | MediaType::Dcts => Some(ModuleKind::Cjs), + MediaType::Mts | MediaType::Mjs | MediaType::Dmts => Some(NodeModuleKind::Esm), + MediaType::Cjs | MediaType::Cts | MediaType::Dcts => Some(NodeModuleKind::Cjs), MediaType::Dts => { // dts files are always determined based on the package.json because // they contain imports/exports even when considered CJS @@ -521,11 +530,11 @@ impl CjsTracker { if let Some(value) = value { self.known.insert(specifier.clone(), value); } - Some(value.unwrap_or(ModuleKind::Esm)) + Some(value.unwrap_or(NodeModuleKind::Esm)) } } MediaType::Wasm | - MediaType::Json => Some(ModuleKind::Esm), + MediaType::Json => Some(NodeModuleKind::Esm), MediaType::JavaScript | MediaType::Jsx | MediaType::TypeScript @@ -535,17 +544,17 @@ impl CjsTracker { | MediaType::SourceMap | MediaType::Unknown => { if let Some(value) = self.known.get(specifier).map(|v| *v) { - if value.is_cjs() && is_script == Some(false) { + if value == NodeModuleKind::Cjs && is_script == Some(false) { // we now know this is actually esm - self.known.insert(specifier.clone(), ModuleKind::Esm); - Some(ModuleKind::Esm) + self.known.insert(specifier.clone(), NodeModuleKind::Esm); + Some(NodeModuleKind::Esm) } else { Some(value) } } else if is_script == Some(false) { // we know this is esm - self.known.insert(specifier.clone(), ModuleKind::Esm); - Some(ModuleKind::Esm) + self.known.insert(specifier.clone(), NodeModuleKind::Esm); + Some(NodeModuleKind::Esm) } else { None } @@ -556,27 +565,35 @@ impl CjsTracker { fn check_based_on_pkg_json( &self, specifier: &ModuleSpecifier, - ) -> Result { + ) -> Result { if self.in_npm_pkg_checker.in_npm_package(specifier) { if let Some(pkg_json) = self.pkg_json_resolver.get_closest_package_json(specifier)? { let is_file_location_cjs = pkg_json.typ != "module"; - Ok(ModuleKind::from_is_cjs(is_file_location_cjs)) + Ok(if is_file_location_cjs { + NodeModuleKind::Cjs + } else { + NodeModuleKind::Esm + }) } else { - Ok(ModuleKind::Cjs) + Ok(NodeModuleKind::Cjs) } - } else if self.unstable_detect_cjs { + } else if self.detect_cjs { if let Some(pkg_json) = self.pkg_json_resolver.get_closest_package_json(specifier)? { let is_cjs_type = pkg_json.typ == "commonjs"; - Ok(ModuleKind::from_is_cjs(is_cjs_type)) + Ok(if is_cjs_type { + NodeModuleKind::Cjs + } else { + NodeModuleKind::Esm + }) } else { - Ok(ModuleKind::Esm) + Ok(NodeModuleKind::Esm) } } else { - Ok(ModuleKind::Esm) + Ok(NodeModuleKind::Esm) } } } @@ -588,6 +605,7 @@ pub type CliSloppyImportsResolver = /// import map, JSX settings. #[derive(Debug)] pub struct CliGraphResolver { + cjs_tracker: Arc, node_resolver: Option>, npm_resolver: Option>, sloppy_imports_resolver: Option>, @@ -602,6 +620,7 @@ pub struct CliGraphResolver { } pub struct CliGraphResolverOptions<'a> { + pub cjs_tracker: Arc, pub node_resolver: Option>, pub npm_resolver: Option>, pub sloppy_imports_resolver: Option>, @@ -614,6 +633,7 @@ pub struct CliGraphResolverOptions<'a> { impl CliGraphResolver { pub fn new(options: CliGraphResolverOptions) -> Self { Self { + cjs_tracker: options.cjs_tracker, node_resolver: options.node_resolver, npm_resolver: options.npm_resolver, sloppy_imports_resolver: options.sloppy_imports_resolver, @@ -686,7 +706,12 @@ impl Resolver for CliGraphResolver { if let Some(node_resolver) = self.node_resolver.as_ref() { if referrer.scheme() == "file" && node_resolver.in_npm_package(referrer) { return node_resolver - .resolve(raw_specifier, referrer, to_node_mode(mode)) + .resolve( + raw_specifier, + referrer, + self.cjs_tracker.get_referrer_kind(referrer), + to_node_mode(mode), + ) .map(|res| res.into_url()) .map_err(|e| ResolveError::Other(e.into())); } @@ -759,6 +784,7 @@ impl Resolver for CliGraphResolver { pkg_json.dir_path(), sub_path.as_deref(), Some(referrer), + self.cjs_tracker.get_referrer_kind(referrer), to_node_mode(mode), ) .map_err(|e| ResolveError::Other(e.into())), @@ -800,6 +826,7 @@ impl Resolver for CliGraphResolver { pkg_folder, sub_path.as_deref(), Some(referrer), + self.cjs_tracker.get_referrer_kind(referrer), to_node_mode(mode), ) .map_err(|e| ResolveError::Other(e.into())) @@ -847,6 +874,7 @@ impl Resolver for CliGraphResolver { pkg_folder, npm_req_ref.sub_path(), Some(referrer), + self.cjs_tracker.get_referrer_kind(referrer), to_node_mode(mode), ) .map_err(|e| ResolveError::Other(e.into())); @@ -855,7 +883,12 @@ impl Resolver for CliGraphResolver { // do npm resolution for byonm if is_byonm { return node_resolver - .resolve_req_reference(&npm_req_ref, referrer, to_node_mode(mode)) + .resolve_req_reference( + &npm_req_ref, + referrer, + self.cjs_tracker.get_referrer_kind(referrer), + to_node_mode(mode), + ) .map_err(|err| err.into()); } } @@ -869,7 +902,12 @@ impl Resolver for CliGraphResolver { // If byonm, check if the bare specifier resolves to an npm package if is_byonm && referrer.scheme() == "file" { let maybe_resolution = node_resolver - .resolve_if_for_npm_pkg(raw_specifier, referrer, to_node_mode(mode)) + .resolve_if_for_npm_pkg( + raw_specifier, + referrer, + self.cjs_tracker.get_referrer_kind(referrer), + to_node_mode(mode), + ) .map_err(ResolveError::Other)?; if let Some(res) = maybe_resolution { match res { diff --git a/cli/standalone/mod.rs b/cli/standalone/mod.rs index c9413b02ca7b9d..05d44a6c85c56b 100644 --- a/cli/standalone/mod.rs +++ b/cli/standalone/mod.rs @@ -45,6 +45,7 @@ use deno_runtime::WorkerLogLevel; use deno_semver::npm::NpmPackageReqReference; use import_map::parse_from_json; use node_resolver::analyze::NodeCodeTranslator; +use node_resolver::NodeModuleKind; use node_resolver::NodeResolutionMode; use serialization::DenoCompileModuleSource; use std::borrow::Cow; @@ -146,13 +147,27 @@ impl ModuleLoader for EmbeddedModuleLoader { type_error(format!("Referrer uses invalid specifier: {}", err)) })? }; + let referrer_kind = if self + .shared + .cjs_tracker + .is_maybe_cjs(&referrer, MediaType::from_specifier(&referrer))? + { + NodeModuleKind::Cjs + } else { + NodeModuleKind::Esm + }; if self.shared.node_resolver.in_npm_package(&referrer) { return Ok( self .shared .node_resolver - .resolve(raw_specifier, &referrer, NodeResolutionMode::Execution)? + .resolve( + raw_specifier, + &referrer, + referrer_kind, + NodeResolutionMode::Execution, + )? .into_url(), ); } @@ -178,6 +193,7 @@ impl ModuleLoader for EmbeddedModuleLoader { pkg_json.dir_path(), sub_path.as_deref(), Some(&referrer), + referrer_kind, NodeResolutionMode::Execution, )?, ), @@ -192,6 +208,7 @@ impl ModuleLoader for EmbeddedModuleLoader { req, sub_path.as_deref(), &referrer, + referrer_kind, NodeResolutionMode::Execution, ) } @@ -211,6 +228,7 @@ impl ModuleLoader for EmbeddedModuleLoader { pkg_folder, sub_path.as_deref(), Some(&referrer), + referrer_kind, NodeResolutionMode::Execution, )?, ) @@ -224,6 +242,7 @@ impl ModuleLoader for EmbeddedModuleLoader { return self.shared.node_resolver.resolve_req_reference( &reference, &referrer, + referrer_kind, NodeResolutionMode::Execution, ); } @@ -250,6 +269,7 @@ impl ModuleLoader for EmbeddedModuleLoader { let maybe_res = self.shared.node_resolver.resolve_if_for_npm_pkg( raw_specifier, &referrer, + referrer_kind, NodeResolutionMode::Execution, )?; if let Some(res) = maybe_res { diff --git a/cli/tools/lint/rules/no_sloppy_imports.rs b/cli/tools/lint/rules/no_sloppy_imports.rs index 2f60875885fc0d..94bf9a7c676685 100644 --- a/cli/tools/lint/rules/no_sloppy_imports.rs +++ b/cli/tools/lint/rules/no_sloppy_imports.rs @@ -87,6 +87,7 @@ impl LintRule for NoSloppyImportsRule { captures: Default::default(), }; + // fill this and capture the sloppy imports in the resolver deno_graph::parse_module_from_ast(deno_graph::ParseModuleFromAstOptions { graph_kind: deno_graph::GraphKind::All, specifier: context.specifier().clone(), diff --git a/cli/tools/run/hmr.rs b/cli/tools/run/hmr.rs index 6cebedd0127c5f..373c207d6991ee 100644 --- a/cli/tools/run/hmr.rs +++ b/cli/tools/run/hmr.rs @@ -4,8 +4,6 @@ use std::collections::HashMap; use std::path::PathBuf; use std::sync::Arc; -use deno_ast::MediaType; -use deno_ast::ModuleKind; use deno_core::error::generic_error; use deno_core::error::AnyError; use deno_core::futures::StreamExt; @@ -18,7 +16,6 @@ use tokio::select; use crate::cdp; use crate::emit::Emitter; -use crate::resolver::CjsTracker; use crate::util::file_watcher::WatcherCommunicator; use crate::util::file_watcher::WatcherRestartMode; @@ -63,7 +60,6 @@ pub struct HmrRunner { session: LocalInspectorSession, watcher_communicator: Arc, script_ids: HashMap, - cjs_tracker: Arc, emitter: Arc, } @@ -146,7 +142,6 @@ impl crate::worker::HmrRunner for HmrRunner { let source_code = self.emitter.load_and_emit_for_hmr( &module_url, - ModuleKind::from_is_cjs(self.cjs_tracker.is_maybe_cjs(&module_url, MediaType::from_specifier(&module_url))?), ).await?; let mut tries = 1; @@ -179,14 +174,12 @@ impl crate::worker::HmrRunner for HmrRunner { impl HmrRunner { pub fn new( - cjs_tracker: Arc, emitter: Arc, session: LocalInspectorSession, watcher_communicator: Arc, ) -> Self { Self { session, - cjs_tracker, emitter, watcher_communicator, script_ids: HashMap::new(), diff --git a/cli/tsc/99_main_compiler.js b/cli/tsc/99_main_compiler.js index 52c9134dad871c..68d099253a5f4e 100644 --- a/cli/tsc/99_main_compiler.js +++ b/cli/tsc/99_main_compiler.js @@ -121,8 +121,8 @@ delete Object.prototype.__proto__; /** @type {Map} */ const sourceFileCache = new Map(); - /** @type {Map} */ - const sourceTextCache = new Map(); + /** @type {Map} */ + const scriptSnapshotCache = new Map(); /** @type {Map} */ const sourceRefCounts = new Map(); @@ -133,9 +133,6 @@ delete Object.prototype.__proto__; /** @type {Map} */ const isNodeSourceFileCache = new Map(); - /** @type {Map} */ - const isCjsCache = new Map(); - // Maps asset specifiers to the first scope that the asset was loaded into. /** @type {Map} */ const assetScopes = new Map(); @@ -210,12 +207,13 @@ delete Object.prototype.__proto__; const mapKey = path + key; let sourceFile = documentRegistrySourceFileCache.get(mapKey); if (!sourceFile || sourceFile.version !== version) { + const isCjs = /** @type {any} */ (scriptSnapshot).isCjs; sourceFile = ts.createLanguageServiceSourceFile( fileName, scriptSnapshot, { ...getCreateSourceFileOptions(sourceFileOptions), - impliedNodeFormat: (isCjsCache.get(fileName) ?? false) + impliedNodeFormat: isCjs ? ts.ModuleKind.CommonJS : ts.ModuleKind.ESNext, // in the lsp we want to be able to show documentation @@ -320,7 +318,7 @@ delete Object.prototype.__proto__; if (lastRequestMethod != "cleanupSemanticCache") { const mapKey = path + key; documentRegistrySourceFileCache.delete(mapKey); - sourceTextCache.delete(path); + scriptSnapshotCache.delete(path); ops.op_release(path); } } else { @@ -624,8 +622,6 @@ delete Object.prototype.__proto__; `"data" is unexpectedly null for "${specifier}".`, ); - isCjsCache.set(specifier, isCjs); - sourceFile = ts.createSourceFile( specifier, data, @@ -699,7 +695,7 @@ delete Object.prototype.__proto__; /** @type {[string, ts.Extension] | undefined} */ const resolved = ops.op_resolve( containingFilePath, - isCjsCache.get(containingFilePath) ?? false, + containingFileMode === ts.ModuleKind.CommonJS, [fileReference.fileName], )?.[0]; if (resolved) { @@ -723,7 +719,14 @@ delete Object.prototype.__proto__; } }); }, - resolveModuleNames(specifiers, base) { + resolveModuleNames( + specifiers, + base, + _reusedNames, + _redirectedReference, + _options, + containingSourceFile, + ) { if (logDebug) { debug(`host.resolveModuleNames()`); debug(` base: ${base}`); @@ -732,7 +735,7 @@ delete Object.prototype.__proto__; /** @type {Array<[string, ts.Extension] | undefined>} */ const resolved = ops.op_resolve( base, - isCjsCache.get(base) ?? false, + containingSourceFile?.impliedNodeFormat === ts.ModuleKind.CommonJS, specifiers, ); if (resolved) { @@ -814,19 +817,19 @@ delete Object.prototype.__proto__; return ts.ScriptSnapshot.fromString(sourceFile.text); } } - let sourceText = sourceTextCache.get(specifier); - if (sourceText == undefined) { + let scriptSnapshot = scriptSnapshotCache.get(specifier); + if (scriptSnapshot == undefined) { /** @type {{ data: string, version: string, isCjs: boolean }} */ const fileInfo = ops.op_load(specifier); if (!fileInfo) { return undefined; } - isCjsCache.set(specifier, fileInfo.isCjs); - sourceTextCache.set(specifier, fileInfo.data); + scriptSnapshot = ts.ScriptSnapshot.fromString(fileInfo.data); + scriptSnapshot.isCjs = fileInfo.isCjs; + scriptSnapshotCache.set(specifier, scriptSnapshot); scriptVersionCache.set(specifier, fileInfo.version); - sourceText = fileInfo.data; } - return ts.ScriptSnapshot.fromString(sourceText); + return scriptSnapshot; }, }; @@ -1238,7 +1241,7 @@ delete Object.prototype.__proto__; closed = true; } scriptVersionCache.delete(script); - sourceTextCache.delete(script); + scriptSnapshotCache.delete(script); } if (newConfigsByScope || opened || closed) { diff --git a/cli/tsc/mod.rs b/cli/tsc/mod.rs index dc7fc38f7a0172..cd9b86cd71cd84 100644 --- a/cli/tsc/mod.rs +++ b/cli/tsc/mod.rs @@ -343,31 +343,25 @@ impl TypeCheckingCjsTracker { media_type: MediaType, code: &Arc, ) -> bool { - if let Some(module_kind) = - self.cjs_tracker.get_known_kind(specifier, media_type) - { - module_kind.is_cjs() - } else { - let maybe_is_script = self - .module_info_cache - .as_module_analyzer() - .analyze_sync(specifier, media_type, code) - .ok() - .map(|info| info.is_script); - maybe_is_script - .and_then(|is_script| { - self - .cjs_tracker - .is_cjs_with_known_is_script(specifier, media_type, is_script) - .ok() - }) - .unwrap_or_else(|| { - self - .cjs_tracker - .is_maybe_cjs(specifier, media_type) - .unwrap_or(false) - }) - } + let maybe_is_script = self + .module_info_cache + .as_module_analyzer() + .analyze_sync(specifier, media_type, code) + .ok() + .map(|info| info.is_script); + maybe_is_script + .and_then(|is_script| { + self + .cjs_tracker + .is_cjs_with_known_is_script(specifier, media_type, is_script) + .ok() + }) + .unwrap_or_else(|| { + self + .cjs_tracker + .is_maybe_cjs(specifier, media_type) + .unwrap_or(false) + }) } } @@ -737,6 +731,7 @@ fn op_resolve_inner( "Error converting a string module specifier for \"op_resolve\".", )? }; + let referrer_module = state.graph.get(&referrer); for specifier in args.specifiers { if specifier.starts_with("node:") { resolved.push(( @@ -752,16 +747,19 @@ fn op_resolve_inner( continue; } - let graph = &state.graph; - let resolved_dep = graph - .get(&referrer) + let resolved_dep = referrer_module .and_then(|m| m.js()) .and_then(|m| m.dependencies_prefer_fast_check().get(&specifier)) .and_then(|d| d.maybe_type.ok().or_else(|| d.maybe_code.ok())); let maybe_result = match resolved_dep { Some(ResolutionResolved { specifier, .. }) => { - resolve_graph_specifier_types(specifier, &referrer, state)? + resolve_graph_specifier_types( + specifier, + &referrer, + referrer_kind, + state, + )? } _ => { match resolve_non_graph_specifier_types( @@ -834,6 +832,7 @@ fn op_resolve_inner( fn resolve_graph_specifier_types( specifier: &ModuleSpecifier, referrer: &ModuleSpecifier, + referrer_kind: NodeModuleKind, state: &State, ) -> Result, AnyError> { let graph = &state.graph; @@ -886,6 +885,7 @@ fn resolve_graph_specifier_types( &package_folder, module.nv_reference.sub_path(), Some(referrer), + referrer_kind, NodeResolutionMode::Types, ); let maybe_url = match res_result { @@ -965,6 +965,7 @@ fn resolve_non_graph_specifier_types( &package_folder, npm_req_ref.sub_path(), Some(referrer), + referrer_kind, NodeResolutionMode::Types, ); let maybe_url = match res_result { diff --git a/cli/worker.rs b/cli/worker.rs index baacd681a16764..7ee794c1eacd85 100644 --- a/cli/worker.rs +++ b/cli/worker.rs @@ -43,6 +43,7 @@ use deno_runtime::WorkerExecutionMode; use deno_runtime::WorkerLogLevel; use deno_semver::npm::NpmPackageReqReference; use deno_terminal::colors; +use node_resolver::NodeModuleKind; use node_resolver::NodeResolutionMode; use tokio::select; @@ -675,6 +676,7 @@ impl CliMainWorkerFactory { package_folder, sub_path, /* referrer */ None, + NodeModuleKind::Esm, NodeResolutionMode::Execution, )?; if specifier diff --git a/resolvers/node/package_json.rs b/resolvers/node/package_json.rs index 6967779e5d9113..ae016ebe3ec3c8 100644 --- a/resolvers/node/package_json.rs +++ b/resolvers/node/package_json.rs @@ -15,8 +15,8 @@ use crate::errors::CanonicalizingPkgJsonDirError; use crate::errors::ClosestPkgJsonError; use crate::errors::PackageJsonLoadError; -// todo(dsherret): this isn't exactly correct and we should change it to instead -// be created per worker and passed down as a ctor arg to the pkg json resolver +// it would be nice if this was passed down as a ctor arg to the package.json resolver, +// but it's a little bit complicated to do that, so we just maintain a thread local cache thread_local! { static CACHE: RefCell> = RefCell::new(HashMap::new()); } diff --git a/resolvers/node/resolution.rs b/resolvers/node/resolution.rs index d44539e9786ad9..c1d18646227173 100644 --- a/resolvers/node/resolution.rs +++ b/resolvers/node/resolution.rs @@ -299,9 +299,9 @@ impl NodeResolver { package_dir: &Path, package_subpath: Option<&str>, maybe_referrer: Option<&Url>, + referrer_kind: NodeModuleKind, mode: NodeResolutionMode, ) -> Result { - let node_module_kind = NodeModuleKind::Esm; let package_subpath = package_subpath .map(|s| format!("./{s}")) .unwrap_or_else(|| ".".to_string()); @@ -309,7 +309,7 @@ impl NodeResolver { package_dir, &package_subpath, maybe_referrer, - node_module_kind, + referrer_kind, DEFAULT_CONDITIONS, mode, )?; diff --git a/runtime/web_worker.rs b/runtime/web_worker.rs index 61e5c770299285..a1bb0ee7174a73 100644 --- a/runtime/web_worker.rs +++ b/runtime/web_worker.rs @@ -393,6 +393,13 @@ pub struct WebWorker { maybe_worker_metadata: Option, } +impl Drop for WebWorker { + fn drop(&mut self) { + // clean up the package.json thread local cache + node_resolver::PackageJsonThreadLocalCache::clear(); + } +} + impl WebWorker { pub fn bootstrap_from_options( services: WebWorkerServiceOptions, From 982d8c4d2d0b6ac316526cedd934e71069e33737 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Mon, 11 Nov 2024 12:32:26 -0500 Subject: [PATCH 13/22] Figuring this out... CjsTracker should not be used in the lsp --- cli/factory.rs | 13 ++-- cli/graph_util.rs | 79 +++++++++++++++++-- cli/lsp/analysis.rs | 4 +- cli/lsp/completions.rs | 15 ++-- cli/lsp/diagnostics.rs | 7 +- cli/lsp/documents.rs | 80 ++++++++++++++------ cli/lsp/language_server.rs | 5 +- cli/lsp/resolver.rs | 150 +++++++------------------------------ cli/module_loader.rs | 7 +- cli/resolver.rs | 70 ++++------------- cli/standalone/mod.rs | 1 - cli/tools/repl/session.rs | 6 +- 12 files changed, 201 insertions(+), 236 deletions(-) diff --git a/cli/factory.rs b/cli/factory.rs index 1adf2639ad9af0..146d3bab49377b 100644 --- a/cli/factory.rs +++ b/cli/factory.rs @@ -44,9 +44,9 @@ use crate::npm::CreateInNpmPkgCheckerOptions; use crate::resolver::CjsTracker; use crate::resolver::CjsTrackerOptions; use crate::resolver::CliDenoResolverFs; -use crate::resolver::CliGraphResolver; -use crate::resolver::CliGraphResolverOptions; use crate::resolver::CliNodeResolver; +use crate::resolver::CliResolver; +use crate::resolver::CliResolverOptions; use crate::resolver::CliSloppyImportsResolver; use crate::resolver::NpmModuleLoader; use crate::resolver::SloppyImportsCachedFs; @@ -201,7 +201,7 @@ struct CliFactoryServices { parsed_source_cache: Deferred>, permission_desc_parser: Deferred>, pkg_json_resolver: Deferred>, - resolver: Deferred>, + resolver: Deferred>, root_cert_store_provider: Deferred>, root_permissions_container: Deferred, sloppy_imports_resolver: Deferred>>, @@ -523,15 +523,14 @@ impl CliFactory { .await } - pub async fn resolver(&self) -> Result<&Arc, AnyError> { + pub async fn resolver(&self) -> Result<&Arc, AnyError> { self .services .resolver .get_or_try_init_async( async { let cli_options = self.cli_options()?; - Ok(Arc::new(CliGraphResolver::new(CliGraphResolverOptions { - cjs_tracker: self.cjs_tracker()?.clone(), + Ok(Arc::new(CliResolver::new(CliResolverOptions { sloppy_imports_resolver: self.sloppy_imports_resolver()?.cloned(), node_resolver: Some(self.cli_node_resolver().await?.clone()), npm_resolver: if cli_options.no_npm() { @@ -706,6 +705,7 @@ impl CliFactory { let cli_options = self.cli_options()?; Ok(Arc::new(ModuleGraphBuilder::new( self.caches()?.clone(), + self.cjs_tracker()?.clone(), cli_options.clone(), self.file_fetcher()?.clone(), self.fs().clone(), @@ -809,7 +809,6 @@ impl CliFactory { .cli_node_resolver .get_or_try_init_async(async { Ok(Arc::new(CliNodeResolver::new( - self.cjs_tracker()?.clone(), self.fs().clone(), self.in_npm_pkg_checker()?.clone(), self.node_resolver().await?.clone(), diff --git a/cli/graph_util.rs b/cli/graph_util.rs index 46257cf785bab4..3f48449bc5d59d 100644 --- a/cli/graph_util.rs +++ b/cli/graph_util.rs @@ -13,16 +13,19 @@ use crate::colors; use crate::errors::get_error_class_name; use crate::file_fetcher::FileFetcher; use crate::npm::CliNpmResolver; -use crate::resolver::CliGraphResolver; +use crate::resolver::CjsTracker; +use crate::resolver::CliResolver; use crate::resolver::CliSloppyImportsResolver; use crate::resolver::SloppyImportsCachedFs; use crate::tools::check; use crate::tools::check::TypeChecker; use crate::util::file_watcher::WatcherCommunicator; use crate::util::fs::canonicalize_path; +use deno_config::deno_json::JsxImportSourceConfig; use deno_config::workspace::JsrPackageConfig; use deno_core::anyhow::bail; use deno_graph::source::LoaderChecksum; +use deno_graph::source::ResolutionMode; use deno_graph::FillFromLockfileOptions; use deno_graph::JsrLoadError; use deno_graph::ModuleLoadError; @@ -379,6 +382,7 @@ pub struct BuildFastCheckGraphOptions<'a> { pub struct ModuleGraphBuilder { caches: Arc, + cjs_tracker: Arc, cli_options: Arc, file_fetcher: Arc, fs: Arc, @@ -389,7 +393,7 @@ pub struct ModuleGraphBuilder { module_info_cache: Arc, npm_resolver: Arc, parsed_source_cache: Arc, - resolver: Arc, + resolver: Arc, root_permissions_container: PermissionsContainer, } @@ -397,6 +401,7 @@ impl ModuleGraphBuilder { #[allow(clippy::too_many_arguments)] pub fn new( caches: Arc, + cjs_tracker: Arc, cli_options: Arc, file_fetcher: Arc, fs: Arc, @@ -407,11 +412,12 @@ impl ModuleGraphBuilder { module_info_cache: Arc, npm_resolver: Arc, parsed_source_cache: Arc, - resolver: Arc, + resolver: Arc, root_permissions_container: PermissionsContainer, ) -> Self { Self { caches, + cjs_tracker, cli_options, file_fetcher, fs, @@ -518,7 +524,7 @@ impl ModuleGraphBuilder { None => MutLoaderRef::Owned(self.create_graph_loader()), }; let cli_resolver = &self.resolver; - let graph_resolver = cli_resolver.as_graph_resolver(); + let graph_resolver = self.create_graph_resolver()?; let graph_npm_resolver = cli_resolver.create_graph_npm_resolver(); let maybe_file_watcher_reporter = self .maybe_file_watcher_reporter @@ -543,7 +549,7 @@ impl ModuleGraphBuilder { npm_resolver: Some(&graph_npm_resolver), module_analyzer: &analyzer, reporter: maybe_file_watcher_reporter, - resolver: Some(graph_resolver), + resolver: Some(&graph_resolver), locker: locker.as_mut().map(|l| l as _), }, ) @@ -666,7 +672,7 @@ impl ModuleGraphBuilder { }; let parser = self.parsed_source_cache.as_capturing_parser(); let cli_resolver = &self.resolver; - let graph_resolver = cli_resolver.as_graph_resolver(); + let graph_resolver = self.create_graph_resolver()?; let graph_npm_resolver = cli_resolver.create_graph_npm_resolver(); graph.build_fast_check_type_graph( @@ -675,7 +681,7 @@ impl ModuleGraphBuilder { fast_check_cache: fast_check_cache.as_ref().map(|c| c as _), fast_check_dts: false, jsr_url_provider: &CliJsrUrlProvider, - resolver: Some(graph_resolver), + resolver: Some(&graph_resolver), npm_resolver: Some(&graph_npm_resolver), workspace_fast_check: options.workspace_fast_check, }, @@ -739,6 +745,18 @@ impl ModuleGraphBuilder { }, ) } + + fn create_graph_resolver(&self) -> Result { + let jsx_import_source_config = self + .cli_options + .workspace() + .to_maybe_jsx_import_source_config()?; + Ok(CliGraphResolver { + cjs_tracker: &self.cjs_tracker, + resolver: &self.resolver, + jsx_import_source_config, + }) + } } /// Adds more explanatory information to a resolution error. @@ -1143,6 +1161,53 @@ fn format_deno_graph_error(err: &dyn Error) -> String { message } +#[derive(Debug)] +struct CliGraphResolver<'a> { + cjs_tracker: &'a CjsTracker, + resolver: &'a CliResolver, + jsx_import_source_config: Option, +} + +impl<'a> deno_graph::source::Resolver for CliGraphResolver<'a> { + fn default_jsx_import_source(&self) -> Option { + self + .jsx_import_source_config + .as_ref() + .and_then(|c| c.default_specifier.clone()) + } + + fn default_jsx_import_source_types(&self) -> Option { + self + .jsx_import_source_config + .as_ref() + .and_then(|c| c.default_types_specifier.clone()) + } + + fn jsx_import_source_module(&self) -> &str { + self + .jsx_import_source_config + .as_ref() + .map(|c| c.module.as_str()) + .unwrap_or(deno_graph::source::DEFAULT_JSX_IMPORT_SOURCE_MODULE) + } + + fn resolve( + &self, + raw_specifier: &str, + referrer_range: &deno_graph::Range, + mode: ResolutionMode, + ) -> Result { + self.resolver.resolve( + raw_specifier, + referrer_range, + self + .cjs_tracker + .get_referrer_kind(&referrer_range.specifier), + mode, + ) + } +} + #[cfg(test)] mod test { use std::sync::Arc; diff --git a/cli/lsp/analysis.rs b/cli/lsp/analysis.rs index 98215855c905e6..3b326226750aee 100644 --- a/cli/lsp/analysis.rs +++ b/cli/lsp/analysis.rs @@ -476,7 +476,7 @@ impl<'a> TsResponseImportMapper<'a> { for specifier in specifiers { if let Some(specifier) = self .resolver - .as_graph_resolver(Some(&self.file_referrer)) + .as_cli_resolver(Some(&self.file_referrer)) .resolve( &specifier, &deno_graph::Range { @@ -509,7 +509,7 @@ impl<'a> TsResponseImportMapper<'a> { ) -> bool { self .resolver - .as_graph_resolver(Some(&self.file_referrer)) + .as_cli_resolver(Some(&self.file_referrer)) .resolve( specifier_text, &deno_graph::Range { diff --git a/cli/lsp/completions.rs b/cli/lsp/completions.rs index 1590743b2b8f84..1ea5d722a01e4f 100644 --- a/cli/lsp/completions.rs +++ b/cli/lsp/completions.rs @@ -35,6 +35,7 @@ use deno_semver::package::PackageNv; use import_map::ImportMap; use indexmap::IndexSet; use lsp_types::CompletionList; +use node_resolver::NodeModuleKind; use once_cell::sync::Lazy; use regex::Regex; use tower_lsp::lsp_types as lsp; @@ -167,7 +168,7 @@ pub async fn get_import_completions( let (text, _, range) = document.get_maybe_dependency(position)?; let range = to_narrow_lsp_range(document.text_info(), &range); let resolved = resolver - .as_graph_resolver(file_referrer) + .as_cli_resolver(file_referrer) .resolve( &text, &Range { @@ -355,24 +356,26 @@ fn get_import_map_completions( /// Return local completions that are relative to the base specifier. fn get_local_completions( - base: &ModuleSpecifier, + referrer: &ModuleSpecifier, + referrer_kind: NodeModuleKind, text: &str, range: &lsp::Range, resolver: &LspResolver, ) -> Option { - if base.scheme() != "file" { + if referrer.scheme() != "file" { return None; } let parent = &text[..text.char_indices().rfind(|(_, c)| *c == '/')?.0 + 1]; let resolved_parent = resolver - .as_graph_resolver(Some(base)) + .as_cli_resolver(Some(referrer)) .resolve( parent, &Range { - specifier: base.clone(), + specifier: referrer.clone(), start: deno_graph::Position::zeroed(), end: deno_graph::Position::zeroed(), }, + referrer_kind, ResolutionMode::Execution, ) .ok()?; @@ -385,7 +388,7 @@ fn get_local_completions( let de = de.ok()?; let label = de.path().file_name()?.to_string_lossy().to_string(); let entry_specifier = resolve_path(de.path().to_str()?, &cwd).ok()?; - if entry_specifier == *base { + if entry_specifier == *referrer { return None; } let full_text = format!("{parent}{label}"); diff --git a/cli/lsp/diagnostics.rs b/cli/lsp/diagnostics.rs index a23851e0d2e8bf..83c00d27edadea 100644 --- a/cli/lsp/diagnostics.rs +++ b/cli/lsp/diagnostics.rs @@ -1638,7 +1638,6 @@ mod tests { use crate::lsp::documents::Documents; use crate::lsp::documents::LanguageId; use crate::lsp::language_server::StateSnapshot; - use crate::lsp::resolver::LspCjsTracker; use crate::lsp::resolver::LspResolver; use deno_config::deno_json::ConfigFile; @@ -1687,10 +1686,8 @@ mod tests { .unwrap(); config.tree.inject_config_file(config_file).await; } - let cjs_tracker = Arc::new(LspCjsTracker::new(&cache)); - let resolver = Arc::new( - LspResolver::from_config(&config, &cache, cjs_tracker, None).await, - ); + let resolver = + Arc::new(LspResolver::from_config(&config, &cache, None).await); let mut documents = Documents::default(); documents.update_config(&config, &resolver, &cache, &Default::default()); for (relative_path, source, version, language_id) in sources { diff --git a/cli/lsp/documents.rs b/cli/lsp/documents.rs index 5f1544042dde51..e00b0166aed548 100644 --- a/cli/lsp/documents.rs +++ b/cli/lsp/documents.rs @@ -26,6 +26,7 @@ use deno_core::parking_lot::Mutex; use deno_core::ModuleSpecifier; use deno_graph::source::ResolutionMode; use deno_graph::Resolution; +use deno_path_util::url_from_directory_path; use deno_path_util::url_to_file_path; use deno_runtime::deno_node; use deno_semver::jsr::JsrPackageReqReference; @@ -33,6 +34,8 @@ use deno_semver::npm::NpmPackageReqReference; use deno_semver::package::PackageReq; use indexmap::IndexMap; use indexmap::IndexSet; +use node_resolver::InNpmPackageChecker; +use node_resolver::NodeModuleKind; use std::borrow::Cow; use std::collections::BTreeMap; use std::collections::BTreeSet; @@ -422,8 +425,7 @@ impl Document { maybe_test_module_fut = get_maybe_test_module_fut(maybe_parsed_source.as_ref(), &config); } else { - let graph_resolver = - resolver.as_graph_resolver(self.file_referrer.as_ref()); + let cli_resolver = resolver.as_cli_resolver(self.file_referrer.as_ref()); let npm_resolver = resolver.create_graph_npm_resolver(self.file_referrer.as_ref()); dependencies = Arc::new( @@ -436,7 +438,7 @@ impl Document { d.with_new_resolver( s, &CliJsrUrlProvider, - Some(graph_resolver), + Some(cli_resolver), Some(&npm_resolver), ), ) @@ -446,7 +448,7 @@ impl Document { maybe_types_dependency = self.maybe_types_dependency.as_ref().map(|d| { Arc::new(d.with_new_resolver( &CliJsrUrlProvider, - Some(graph_resolver), + Some(cli_resolver), Some(&npm_resolver), )) }); @@ -1249,7 +1251,7 @@ impl Documents { results.push(None); } } else if let Ok(specifier) = - self.resolver.as_graph_resolver(file_referrer).resolve( + self.resolver.as_cli_resolver(file_referrer).resolve( raw_specifier, &deno_graph::Range { specifier: referrer.clone(), @@ -1412,6 +1414,7 @@ impl Documents { &self, specifier: &ModuleSpecifier, referrer: &ModuleSpecifier, + referrer_kind: NodeModuleKind, file_referrer: Option<&ModuleSpecifier>, ) -> Option<(ModuleSpecifier, MediaType)> { if let Some(module_name) = specifier.as_str().strip_prefix("node:") { @@ -1425,10 +1428,12 @@ impl Documents { let mut specifier = specifier.clone(); let mut media_type = None; if let Ok(npm_ref) = NpmPackageReqReference::from_specifier(&specifier) { - let (s, mt) = - self - .resolver - .npm_to_file_url(&npm_ref, referrer, file_referrer)?; + let (s, mt) = self.resolver.npm_to_file_url( + &npm_ref, + referrer, + referrer_kind, + file_referrer, + )?; specifier = s; media_type = Some(mt); } @@ -1544,18 +1549,20 @@ fn analyze_module( match parsed_source_result { Ok(parsed_source) => { let npm_resolver = resolver.create_graph_npm_resolver(file_referrer); - deno_graph::parse_module_from_ast(deno_graph::ParseModuleFromAstOptions { - graph_kind: deno_graph::GraphKind::TypesOnly, - specifier, - maybe_headers, - parsed_source, - // use a null file system because there's no need to bother resolving - // dynamic imports like import(`./dir/${something}`) in the LSP - file_system: &deno_graph::source::NullFileSystem, - jsr_url_provider: &CliJsrUrlProvider, - maybe_resolver: Some(resolver.as_graph_resolver(file_referrer)), - maybe_npm_resolver: Some(&npm_resolver), - }) + Ok(deno_graph::parse_module_from_ast( + deno_graph::ParseModuleFromAstOptions { + graph_kind: deno_graph::GraphKind::TypesOnly, + specifier, + maybe_headers, + parsed_source, + // use a null file system because there's no need to bother resolving + // dynamic imports like import(`./dir/${something}`) in the LSP + file_system: &deno_graph::source::NullFileSystem, + jsr_url_provider: &CliJsrUrlProvider, + maybe_resolver: Some(resolver.as_cli_resolver(file_referrer)), + maybe_npm_resolver: Some(&npm_resolver), + }, + )) } Err(err) => Err(deno_graph::ModuleGraphError::ModuleError( deno_graph::ModuleError::ParseErr(specifier, err.clone()), @@ -1563,6 +1570,37 @@ fn analyze_module( } } +#[derive(Debug)] +struct LspInNpmPackageChecker { + global_cache_dir: ModuleSpecifier, +} + +impl LspInNpmPackageChecker { + pub fn new(cache: &LspCache) -> Self { + Self { + global_cache_dir: url_from_directory_path( + &cache.deno_dir().npm_folder_path(), + ) + .unwrap_or_else(|_| ModuleSpecifier::parse("file:///invalid/").unwrap()), + } + } +} + +impl InNpmPackageChecker for LspInNpmPackageChecker { + fn in_npm_package(&self, specifier: &ModuleSpecifier) -> bool { + if specifier.scheme() != "file" { + return false; + } + if specifier + .as_str() + .starts_with(self.global_cache_dir.as_str()) + { + return true; + } + specifier.as_str().contains("/node_modules/") + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index 3aa8195dfe5bf1..1176cd41e3ad09 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -22,6 +22,7 @@ use deno_semver::jsr::JsrPackageReqReference; use indexmap::Equivalent; use indexmap::IndexSet; use log::error; +use node_resolver::NodeModuleKind; use serde::Deserialize; use serde_json::from_value; use std::collections::BTreeMap; @@ -986,7 +987,7 @@ impl Inner { spawn(async move { let specifier = { let inner = ls.inner.read().await; - let resolver = inner.resolver.as_graph_resolver(Some(&referrer)); + let resolver = inner.resolver.as_cli_resolver(Some(&referrer)); let Ok(specifier) = resolver.resolve( &specifier, &deno_graph::Range { @@ -994,6 +995,7 @@ impl Inner { start: deno_graph::Position::zeroed(), end: deno_graph::Position::zeroed(), }, + NodeModuleKind::Esm, deno_graph::source::ResolutionMode::Types, ) else { return; @@ -1015,7 +1017,6 @@ impl Inner { LspResolver::from_config( &self.config, &self.cache, - self.cjs_tracker.clone(), Some(&self.http_client_provider), ) .await, diff --git a/cli/lsp/resolver.rs b/cli/lsp/resolver.rs index 66d7ae80a2cbc8..8f8e20f3af48a7 100644 --- a/cli/lsp/resolver.rs +++ b/cli/lsp/resolver.rs @@ -59,9 +59,9 @@ use crate::npm::ManagedCliNpmResolver; use crate::resolver::CjsTracker; use crate::resolver::CjsTrackerOptions; use crate::resolver::CliDenoResolverFs; -use crate::resolver::CliGraphResolver; -use crate::resolver::CliGraphResolverOptions; use crate::resolver::CliNodeResolver; +use crate::resolver::CliResolver; +use crate::resolver::CliResolverOptions; use crate::resolver::WorkerCliNpmGraphResolver; use crate::tsc::into_specifier_and_media_type; use crate::util::progress_bar::ProgressBar; @@ -69,8 +69,7 @@ use crate::util::progress_bar::ProgressBarStyle; #[derive(Debug, Clone)] struct LspScopeResolver { - cjs_tracker: Arc, - graph_resolver: Arc, + resolver: Arc, jsr_resolver: Option>, npm_resolver: Option>, node_resolver: Option>, @@ -83,11 +82,8 @@ struct LspScopeResolver { impl Default for LspScopeResolver { fn default() -> Self { - let cjs_tracker = Arc::new(LspCjsTracker::new(&LspCache::default())); - let graph_resolver = create_graph_resolver(&cjs_tracker, None, None, None); Self { - cjs_tracker, - graph_resolver, + resolver: create_cli_resolver(None, None, None), jsr_resolver: None, npm_resolver: None, node_resolver: None, @@ -104,7 +100,6 @@ impl LspScopeResolver { async fn from_config_data( config_data: Option<&Arc>, cache: &LspCache, - cjs_tracker: Arc, http_client_provider: Option<&Arc>, ) -> Self { let mut npm_resolver = None; @@ -132,8 +127,7 @@ impl LspScopeResolver { )); } } - let graph_resolver = create_graph_resolver( - cjs_tracker.inner(), + let graph_resolver = create_cli_resolver( config_data.map(|d| d.as_ref()), npm_resolver.as_ref(), node_resolver.as_ref(), @@ -182,7 +176,8 @@ impl LspScopeResolver { .resolve_req_reference( &req_ref, &referrer, - referrer_kind, + // todo(dsherret): this is wrong because it doesn't consider CJS referrers + NodeModuleKind::Esm, NodeResolutionMode::Types, ) .ok()?, @@ -196,8 +191,7 @@ impl LspScopeResolver { let package_json_deps_by_resolution = Arc::new(package_json_deps_by_resolution.unwrap_or_default()); Self { - cjs_tracker, - graph_resolver, + resolver: graph_resolver, jsr_resolver, npm_resolver, node_resolver, @@ -226,14 +220,13 @@ impl LspScopeResolver { pkg_json_resolver.clone(), )); } - let graph_resolver = create_graph_resolver( + let graph_resolver = create_cli_resolver( self.config_data.as_deref(), npm_resolver.as_ref(), node_resolver.as_ref(), ); Arc::new(Self { - cjs_tracker: self.cjs_tracker.clone(), - graph_resolver, + resolver: graph_resolver, jsr_resolver: self.jsr_resolver.clone(), npm_resolver, node_resolver, @@ -258,7 +251,6 @@ impl LspResolver { pub async fn from_config( config: &Config, cache: &LspCache, - cjs_tracker: Arc, http_client_provider: Option<&Arc>, ) -> Self { let mut by_scope = BTreeMap::new(); @@ -269,7 +261,6 @@ impl LspResolver { LspScopeResolver::from_config_data( Some(config_data), cache, - cjs_tracker.clone(), http_client_provider, ) .await, @@ -278,13 +269,8 @@ impl LspResolver { } Self { unscoped: Arc::new( - LspScopeResolver::from_config_data( - None, - cache, - cjs_tracker, - http_client_provider, - ) - .await, + LspScopeResolver::from_config_data(None, cache, http_client_provider) + .await, ), by_scope, } @@ -335,12 +321,12 @@ impl LspResolver { } } - pub fn as_graph_resolver( + pub fn as_cli_resolver( &self, file_referrer: Option<&ModuleSpecifier>, - ) -> &dyn Resolver { + ) -> &CliResolver { let resolver = self.get_scope_resolver(file_referrer); - resolver.graph_resolver.as_ref() + resolver.resolver.as_ref() } pub fn create_graph_npm_resolver( @@ -348,7 +334,7 @@ impl LspResolver { file_referrer: Option<&ModuleSpecifier>, ) -> WorkerCliNpmGraphResolver { let resolver = self.get_scope_resolver(file_referrer); - resolver.graph_resolver.create_graph_npm_resolver() + resolver.resolver.create_graph_npm_resolver() } pub fn maybe_node_resolver( @@ -422,13 +408,19 @@ impl LspResolver { &self, req_ref: &NpmPackageReqReference, referrer: &ModuleSpecifier, + referrer_kind: NodeModuleKind, file_referrer: Option<&ModuleSpecifier>, ) -> Option<(ModuleSpecifier, MediaType)> { let resolver = self.get_scope_resolver(file_referrer); let node_resolver = resolver.node_resolver.as_ref()?; Some(into_specifier_and_media_type(Some( node_resolver - .resolve_req_reference(req_ref, referrer, NodeResolutionMode::Types) + .resolve_req_reference( + req_ref, + referrer, + referrer_kind, + NodeResolutionMode::Types, + ) .ok()?, ))) } @@ -610,12 +602,6 @@ async fn create_npm_resolver( Some(create_cli_npm_resolver_for_lsp(options).await) } -fn create_cjs_tracker( - in_npm_pkg_checker: Arc, - pkg_json_resolver: Arc, -) -> Arc { -} - fn create_in_npm_pkg_checker( npm_resolver: &Arc, ) -> Arc { @@ -654,15 +640,13 @@ fn create_node_resolver( )) } -fn create_graph_resolver( - cjs_tracker: &Arc, +fn create_cli_resolver( config_data: Option<&ConfigData>, npm_resolver: Option<&Arc>, node_resolver: Option<&Arc>, -) -> Arc { +) -> Arc { let workspace = config_data.map(|d| &d.member_dir.workspace); - Arc::new(CliGraphResolver::new(CliGraphResolverOptions { - cjs_tracker: cjs_tracker.clone(), + Arc::new(CliResolver::new(CliResolverOptions { node_resolver: node_resolver.cloned(), npm_resolver: npm_resolver.cloned(), workspace_resolver: config_data.map(|d| d.resolver.clone()).unwrap_or_else( @@ -828,88 +812,6 @@ impl RedirectResolver { } } -#[derive(Debug)] -pub struct LspInNpmPackageChecker { - global_cache_dir: ModuleSpecifier, -} - -impl LspInNpmPackageChecker { - pub fn new(cache: &LspCache) -> Self { - Self { - global_cache_dir: url_from_directory_path( - &cache.deno_dir().npm_folder_path(), - ) - .unwrap_or_else(|_| ModuleSpecifier::parse("file:///invalid/").unwrap()), - } - } -} - -impl InNpmPackageChecker for LspInNpmPackageChecker { - fn in_npm_package(&self, specifier: &ModuleSpecifier) -> bool { - if specifier.scheme() != "file" { - return false; - } - if specifier - .as_str() - .starts_with(self.global_cache_dir.as_str()) - { - return true; - } - specifier.as_str().contains("/node_modules/") - } -} - -#[derive(Debug)] -pub struct LspCjsTracker { - cjs_tracker: Arc, -} - -impl LspCjsTracker { - pub fn new(cache: &LspCache) -> Self { - let fs = Arc::new(deno_fs::RealFs); - let cjs_tracker = Arc::new(CjsTracker::new( - Arc::new(LspInNpmPackageChecker::new(cache)), - Arc::new(PackageJsonResolver::new( - deno_runtime::deno_node::DenoFsNodeResolverEnv::new(fs.clone()), - )), - CjsTrackerOptions { detect_cjs: true }, - )); - Self { cjs_tracker } - } - - pub fn inner(&self) -> &Arc { - &self.cjs_tracker - } - - pub fn is_cjs( - &self, - specifier: &ModuleSpecifier, - media_type: MediaType, - maybe_parsed_source: Option<&ParsedSource>, - ) -> bool { - if let Some(module_kind) = - self.cjs_tracker.get_known_kind(specifier, media_type) - { - module_kind == NodeModuleKind::Cjs - } else { - let maybe_is_script = maybe_parsed_source.map(|p| p.compute_is_script()); - maybe_is_script - .and_then(|is_script| { - self - .cjs_tracker - .is_cjs_with_known_is_script(specifier, media_type, is_script) - .ok() - }) - .unwrap_or_else(|| { - self - .cjs_tracker - .is_maybe_cjs(specifier, media_type) - .unwrap_or(false) - }) - } - } -} - #[cfg(test)] mod tests { use super::*; diff --git a/cli/module_loader.rs b/cli/module_loader.rs index 112717c3ec258c..7da070b00b7025 100644 --- a/cli/module_loader.rs +++ b/cli/module_loader.rs @@ -27,8 +27,8 @@ use crate::node; use crate::node::CliNodeCodeTranslator; use crate::npm::CliNpmResolver; use crate::resolver::CjsTracker; -use crate::resolver::CliGraphResolver; use crate::resolver::CliNodeResolver; +use crate::resolver::CliResolver; use crate::resolver::ModuleCodeStringSource; use crate::resolver::NotSupportedKindInNpmError; use crate::resolver::NpmModuleLoader; @@ -220,7 +220,7 @@ struct SharedCliModuleLoaderState { npm_resolver: Arc, npm_module_loader: NpmModuleLoader, parsed_source_cache: Arc, - resolver: Arc, + resolver: Arc, } pub struct CliModuleLoaderFactory { @@ -243,7 +243,7 @@ impl CliModuleLoaderFactory { npm_resolver: Arc, npm_module_loader: NpmModuleLoader, parsed_source_cache: Arc, - resolver: Arc, + resolver: Arc, ) -> Self { Self { shared: Arc::new(SharedCliModuleLoaderState { @@ -510,6 +510,7 @@ impl start: deno_graph::Position::zeroed(), end: deno_graph::Position::zeroed(), }, + self.shared.cjs_tracker.get_referrer_kind(referrer), ResolutionMode::Execution, )?), }; diff --git a/cli/resolver.rs b/cli/resolver.rs index 58be27a1ead00d..d11bf92131d5e5 100644 --- a/cli/resolver.rs +++ b/cli/resolver.rs @@ -18,7 +18,6 @@ use deno_graph::source::ResolutionMode; use deno_graph::source::ResolveError; use deno_graph::source::Resolver; use deno_graph::source::UnknownBuiltInNodeModuleError; -use deno_graph::source::DEFAULT_JSX_IMPORT_SOURCE_MODULE; use deno_graph::NpmLoadError; use deno_graph::NpmResolvePkgReqsResult; use deno_npm::resolution::NpmResolutionError; @@ -495,6 +494,9 @@ impl CjsTracker { &self, specifier: &ModuleSpecifier, ) -> NodeModuleKind { + if specifier.scheme() != "file" { + return NodeModuleKind::Esm; + } self .get_known_kind(specifier, MediaType::from_specifier(specifier)) .unwrap_or_else(|| { @@ -604,23 +606,18 @@ pub type CliSloppyImportsResolver = /// A resolver that takes care of resolution, taking into account loaded /// import map, JSX settings. #[derive(Debug)] -pub struct CliGraphResolver { - cjs_tracker: Arc, +pub struct CliResolver { node_resolver: Option>, npm_resolver: Option>, sloppy_imports_resolver: Option>, workspace_resolver: Arc, - maybe_default_jsx_import_source: Option, - maybe_default_jsx_import_source_types: Option, - maybe_jsx_import_source_module: Option, maybe_vendor_specifier: Option, found_package_json_dep_flag: AtomicFlag, bare_node_builtins_enabled: bool, warned_pkgs: DashSet, } -pub struct CliGraphResolverOptions<'a> { - pub cjs_tracker: Arc, +pub struct CliResolverOptions<'a> { pub node_resolver: Option>, pub npm_resolver: Option>, pub sloppy_imports_resolver: Option>, @@ -630,25 +627,13 @@ pub struct CliGraphResolverOptions<'a> { pub maybe_vendor_dir: Option<&'a PathBuf>, } -impl CliGraphResolver { - pub fn new(options: CliGraphResolverOptions) -> Self { +impl CliResolver { + pub fn new(options: CliResolverOptions) -> Self { Self { - cjs_tracker: options.cjs_tracker, node_resolver: options.node_resolver, npm_resolver: options.npm_resolver, sloppy_imports_resolver: options.sloppy_imports_resolver, workspace_resolver: options.workspace_resolver, - maybe_default_jsx_import_source: options - .maybe_jsx_import_source_config - .as_ref() - .and_then(|c| c.default_specifier.clone()), - maybe_default_jsx_import_source_types: options - .maybe_jsx_import_source_config - .as_ref() - .and_then(|c| c.default_types_specifier.clone()), - maybe_jsx_import_source_module: options - .maybe_jsx_import_source_config - .map(|c| c.module), maybe_vendor_specifier: options .maybe_vendor_dir .and_then(|v| ModuleSpecifier::from_directory_path(v).ok()), @@ -658,10 +643,6 @@ impl CliGraphResolver { } } - pub fn as_graph_resolver(&self) -> &dyn Resolver { - self - } - pub fn create_graph_npm_resolver(&self) -> WorkerCliNpmGraphResolver { WorkerCliNpmGraphResolver { npm_resolver: self.npm_resolver.as_ref(), @@ -669,28 +650,12 @@ impl CliGraphResolver { bare_node_builtins_enabled: self.bare_node_builtins_enabled, } } -} - -impl Resolver for CliGraphResolver { - fn default_jsx_import_source(&self) -> Option { - self.maybe_default_jsx_import_source.clone() - } - fn default_jsx_import_source_types(&self) -> Option { - self.maybe_default_jsx_import_source_types.clone() - } - - fn jsx_import_source_module(&self) -> &str { - self - .maybe_jsx_import_source_module - .as_deref() - .unwrap_or(DEFAULT_JSX_IMPORT_SOURCE_MODULE) - } - - fn resolve( + pub fn resolve( &self, raw_specifier: &str, referrer_range: &deno_graph::Range, + referrer_kind: NodeModuleKind, mode: ResolutionMode, ) -> Result { fn to_node_mode(mode: ResolutionMode) -> NodeResolutionMode { @@ -706,12 +671,7 @@ impl Resolver for CliGraphResolver { if let Some(node_resolver) = self.node_resolver.as_ref() { if referrer.scheme() == "file" && node_resolver.in_npm_package(referrer) { return node_resolver - .resolve( - raw_specifier, - referrer, - self.cjs_tracker.get_referrer_kind(referrer), - to_node_mode(mode), - ) + .resolve(raw_specifier, referrer, referrer_kind, to_node_mode(mode)) .map(|res| res.into_url()) .map_err(|e| ResolveError::Other(e.into())); } @@ -784,7 +744,7 @@ impl Resolver for CliGraphResolver { pkg_json.dir_path(), sub_path.as_deref(), Some(referrer), - self.cjs_tracker.get_referrer_kind(referrer), + referrer_kind, to_node_mode(mode), ) .map_err(|e| ResolveError::Other(e.into())), @@ -826,7 +786,7 @@ impl Resolver for CliGraphResolver { pkg_folder, sub_path.as_deref(), Some(referrer), - self.cjs_tracker.get_referrer_kind(referrer), + referrer_kind, to_node_mode(mode), ) .map_err(|e| ResolveError::Other(e.into())) @@ -874,7 +834,7 @@ impl Resolver for CliGraphResolver { pkg_folder, npm_req_ref.sub_path(), Some(referrer), - self.cjs_tracker.get_referrer_kind(referrer), + referrer_kind, to_node_mode(mode), ) .map_err(|e| ResolveError::Other(e.into())); @@ -886,7 +846,7 @@ impl Resolver for CliGraphResolver { .resolve_req_reference( &npm_req_ref, referrer, - self.cjs_tracker.get_referrer_kind(referrer), + referrer_kind, to_node_mode(mode), ) .map_err(|err| err.into()); @@ -905,7 +865,7 @@ impl Resolver for CliGraphResolver { .resolve_if_for_npm_pkg( raw_specifier, referrer, - self.cjs_tracker.get_referrer_kind(referrer), + referrer_kind, to_node_mode(mode), ) .map_err(ResolveError::Other)?; diff --git a/cli/standalone/mod.rs b/cli/standalone/mod.rs index 05d44a6c85c56b..ca65871dd874c5 100644 --- a/cli/standalone/mod.rs +++ b/cli/standalone/mod.rs @@ -655,7 +655,6 @@ pub async fn run(data: StandaloneData) -> Result { let cache_db = Caches::new(deno_dir_provider.clone()); let node_analysis_cache = NodeAnalysisCache::new(cache_db.node_analysis_db()); let cli_node_resolver = Arc::new(CliNodeResolver::new( - cjs_tracker.clone(), fs.clone(), in_npm_pkg_checker.clone(), node_resolver.clone(), diff --git a/cli/tools/repl/session.rs b/cli/tools/repl/session.rs index 23b0f11ac5266d..90284e97a5d46b 100644 --- a/cli/tools/repl/session.rs +++ b/cli/tools/repl/session.rs @@ -7,7 +7,7 @@ use crate::cdp; use crate::colors; use crate::lsp::ReplLanguageServer; use crate::npm::CliNpmResolver; -use crate::resolver::CliGraphResolver; +use crate::resolver::CliResolver; use crate::tools::test::report_tests; use crate::tools::test::reporters::PrettyTestReporter; use crate::tools::test::reporters::TestReporter; @@ -180,7 +180,7 @@ struct ReplJsxState { pub struct ReplSession { npm_resolver: Arc, - resolver: Arc, + resolver: Arc, pub worker: MainWorker, session: LocalInspectorSession, pub context_id: u64, @@ -199,7 +199,7 @@ impl ReplSession { pub async fn initialize( cli_options: &CliOptions, npm_resolver: Arc, - resolver: Arc, + resolver: Arc, mut worker: MainWorker, main_module: ModuleSpecifier, test_event_receiver: TestEventReceiver, From af5a3748396b6e031edf1152ca144864a4e1f885 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Mon, 11 Nov 2024 16:31:11 -0500 Subject: [PATCH 14/22] compiling --- cli/args/mod.rs | 1 - cli/factory.rs | 7 +- cli/lsp/analysis.rs | 33 ++++--- cli/lsp/completions.rs | 7 +- cli/lsp/config.rs | 12 +++ cli/lsp/diagnostics.rs | 1 + cli/lsp/documents.rs | 149 +++++++++++++++++++++----------- cli/lsp/language_server.rs | 40 +++++++-- cli/lsp/resolver.rs | 170 ++++++++++++++++++++++++++++++++++--- cli/lsp/tsc.rs | 34 ++++---- cli/module_loader.rs | 2 - cli/resolver.rs | 97 ++++++++++++++++----- cli/standalone/mod.rs | 4 +- cli/tools/repl/session.rs | 9 +- 14 files changed, 433 insertions(+), 133 deletions(-) diff --git a/cli/args/mod.rs b/cli/args/mod.rs index 4d87fd257eb965..0fdfe2c242dfc3 100644 --- a/cli/args/mod.rs +++ b/cli/args/mod.rs @@ -33,7 +33,6 @@ use import_map::resolve_import_map_value_from_specifier; pub use deno_config::deno_json::BenchConfig; pub use deno_config::deno_json::ConfigFile; pub use deno_config::deno_json::FmtOptionsConfig; -pub use deno_config::deno_json::JsxImportSourceConfig; pub use deno_config::deno_json::LintRulesConfig; pub use deno_config::deno_json::ProseWrap; pub use deno_config::deno_json::TsConfig; diff --git a/cli/factory.rs b/cli/factory.rs index 146d3bab49377b..1446997f26b4e5 100644 --- a/cli/factory.rs +++ b/cli/factory.rs @@ -42,12 +42,12 @@ use crate::npm::CliNpmResolverCreateOptions; use crate::npm::CliNpmResolverManagedSnapshotOption; use crate::npm::CreateInNpmPkgCheckerOptions; use crate::resolver::CjsTracker; -use crate::resolver::CjsTrackerOptions; use crate::resolver::CliDenoResolverFs; use crate::resolver::CliNodeResolver; use crate::resolver::CliResolver; use crate::resolver::CliResolverOptions; use crate::resolver::CliSloppyImportsResolver; +use crate::resolver::IsCjsResolverOptions; use crate::resolver::NpmModuleLoader; use crate::resolver::SloppyImportsCachedFs; use crate::standalone::DenoCompileBinaryWriter; @@ -541,9 +541,6 @@ impl CliFactory { workspace_resolver: self.workspace_resolver().await?.clone(), bare_node_builtins_enabled: cli_options .unstable_bare_node_builtins(), - maybe_jsx_import_source_config: cli_options - .workspace() - .to_maybe_jsx_import_source_config()?, maybe_vendor_dir: cli_options.vendor_dir_path(), }))) } @@ -794,7 +791,7 @@ impl CliFactory { Ok(Arc::new(CjsTracker::new( self.in_npm_pkg_checker()?.clone(), self.pkg_json_resolver().clone(), - CjsTrackerOptions { + IsCjsResolverOptions { detect_cjs: options.detect_cjs(), }, ))) diff --git a/cli/lsp/analysis.rs b/cli/lsp/analysis.rs index 3b326226750aee..97dccb1aab1044 100644 --- a/cli/lsp/analysis.rs +++ b/cli/lsp/analysis.rs @@ -38,6 +38,7 @@ use deno_semver::package::PackageReq; use deno_semver::package::PackageReqReference; use deno_semver::Version; use import_map::ImportMap; +use node_resolver::NodeModuleKind; use once_cell::sync::Lazy; use regex::Regex; use std::borrow::Cow; @@ -466,6 +467,7 @@ impl<'a> TsResponseImportMapper<'a> { &self, specifier: &str, referrer: &ModuleSpecifier, + referrer_kind: NodeModuleKind, ) -> Option { let specifier_stem = specifier.strip_suffix(".js").unwrap_or(specifier); let specifiers = std::iter::once(Cow::Borrowed(specifier)).chain( @@ -484,6 +486,7 @@ impl<'a> TsResponseImportMapper<'a> { start: deno_graph::Position::zeroed(), end: deno_graph::Position::zeroed(), }, + referrer_kind, ResolutionMode::Types, ) .ok() @@ -506,6 +509,7 @@ impl<'a> TsResponseImportMapper<'a> { &self, specifier_text: &str, referrer: &ModuleSpecifier, + referrer_kind: NodeModuleKind, ) -> bool { self .resolver @@ -517,6 +521,7 @@ impl<'a> TsResponseImportMapper<'a> { start: deno_graph::Position::zeroed(), end: deno_graph::Position::zeroed(), }, + referrer_kind, deno_graph::source::ResolutionMode::Types, ) .is_ok() @@ -585,6 +590,7 @@ fn try_reverse_map_package_json_exports( /// like an import and rewrite the import specifier to include the extension pub fn fix_ts_import_changes( referrer: &ModuleSpecifier, + referrer_kind: NodeModuleKind, changes: &[tsc::FileTextChanges], language_server: &language_server::Inner, ) -> Result, AnyError> { @@ -601,8 +607,8 @@ pub fn fix_ts_import_changes( if let Some(captures) = IMPORT_SPECIFIER_RE.captures(line) { let specifier = captures.iter().skip(1).find_map(|s| s).unwrap().as_str(); - if let Some(new_specifier) = - import_mapper.check_unresolved_specifier(specifier, referrer) + if let Some(new_specifier) = import_mapper + .check_unresolved_specifier(specifier, referrer, referrer_kind) { line.replace(specifier, &new_specifier) } else { @@ -632,6 +638,7 @@ pub fn fix_ts_import_changes( /// resolution by Deno (includes the extension). fn fix_ts_import_action<'a>( referrer: &ModuleSpecifier, + referrer_kind: NodeModuleKind, action: &'a tsc::CodeFixAction, language_server: &language_server::Inner, ) -> Option> { @@ -651,7 +658,7 @@ fn fix_ts_import_action<'a>( }; let import_mapper = language_server.get_ts_response_import_mapper(referrer); if let Some(new_specifier) = - import_mapper.check_unresolved_specifier(specifier, referrer) + import_mapper.check_unresolved_specifier(specifier, referrer, referrer_kind) { let description = action.description.replace(specifier, &new_specifier); let changes = action @@ -682,7 +689,7 @@ fn fix_ts_import_action<'a>( fix_id: None, fix_all_description: None, })) - } else if !import_mapper.is_valid_import(specifier, referrer) { + } else if !import_mapper.is_valid_import(specifier, referrer, referrer_kind) { None } else { Some(Cow::Borrowed(action)) @@ -1010,6 +1017,7 @@ impl CodeActionCollection { pub fn add_ts_fix_action( &mut self, specifier: &ModuleSpecifier, + specifier_kind: NodeModuleKind, action: &tsc::CodeFixAction, diagnostic: &lsp::Diagnostic, language_server: &language_server::Inner, @@ -1027,7 +1035,8 @@ impl CodeActionCollection { "The action returned from TypeScript is unsupported.", )); } - let Some(action) = fix_ts_import_action(specifier, action, language_server) + let Some(action) = + fix_ts_import_action(specifier, specifier_kind, action, language_server) else { return Ok(()); }; @@ -1269,6 +1278,9 @@ impl CodeActionCollection { import_start_from_specifier(document, i) })?; let referrer = document.specifier(); + let referrer_kind = language_server + .is_cjs_resolver + .get_doc_module_kind(document); let file_referrer = document.file_referrer(); let config_data = language_server .config @@ -1291,10 +1303,11 @@ impl CodeActionCollection { if !config_data.byonm { return None; } - if !language_server - .resolver - .is_bare_package_json_dep(&dep_key, referrer) - { + if !language_server.resolver.is_bare_package_json_dep( + &dep_key, + referrer, + referrer_kind, + ) { return None; } NpmPackageReqReference::from_str(&format!("npm:{}", &dep_key)).ok()? @@ -1313,7 +1326,7 @@ impl CodeActionCollection { } if language_server .resolver - .npm_to_file_url(&npm_ref, document.specifier(), file_referrer) + .npm_to_file_url(&npm_ref, referrer, referrer_kind, file_referrer) .is_some() { // The package import has types. diff --git a/cli/lsp/completions.rs b/cli/lsp/completions.rs index 1ea5d722a01e4f..3ee8ae93e410f6 100644 --- a/cli/lsp/completions.rs +++ b/cli/lsp/completions.rs @@ -9,6 +9,7 @@ use super::jsr::CliJsrSearchApi; use super::lsp_custom; use super::npm::CliNpmSearchApi; use super::registries::ModuleRegistry; +use super::resolver::LspIsCjsResolver; use super::resolver::LspResolver; use super::search::PackageSearchApi; use super::tsc; @@ -160,10 +161,12 @@ pub async fn get_import_completions( jsr_search_api: &CliJsrSearchApi, npm_search_api: &CliNpmSearchApi, documents: &Documents, + is_cjs_resolver: &LspIsCjsResolver, resolver: &LspResolver, maybe_import_map: Option<&ImportMap>, ) -> Option { let document = documents.get(specifier)?; + let specifier_kind = is_cjs_resolver.get_doc_module_kind(&document); let file_referrer = document.file_referrer(); let (text, _, range) = document.get_maybe_dependency(position)?; let range = to_narrow_lsp_range(document.text_info(), &range); @@ -176,6 +179,7 @@ pub async fn get_import_completions( start: deno_graph::Position::zeroed(), end: deno_graph::Position::zeroed(), }, + specifier_kind, ResolutionMode::Execution, ) .ok(); @@ -202,7 +206,7 @@ pub async fn get_import_completions( // completions for import map specifiers Some(lsp::CompletionResponse::List(completion_list)) } else if let Some(completion_list) = - get_local_completions(specifier, &text, &range, resolver) + get_local_completions(specifier, specifier_kind, &text, &range, resolver) { // completions for local relative modules Some(lsp::CompletionResponse::List(completion_list)) @@ -908,6 +912,7 @@ mod tests { ModuleSpecifier::from_file_path(file_c).expect("could not create"); let actual = get_local_completions( &specifier, + NodeModuleKind::Esm, "./", &lsp::Range { start: lsp::Position { diff --git a/cli/lsp/config.rs b/cli/lsp/config.rs index 34bf64446d0259..ea77e36bcfe1c1 100644 --- a/cli/lsp/config.rs +++ b/cli/lsp/config.rs @@ -4,6 +4,7 @@ use deno_ast::MediaType; use deno_config::deno_json::DenoJsonCache; use deno_config::deno_json::FmtConfig; use deno_config::deno_json::FmtOptionsConfig; +use deno_config::deno_json::JsxImportSourceConfig; use deno_config::deno_json::LintConfig; use deno_config::deno_json::NodeModulesDirMode; use deno_config::deno_json::TestConfig; @@ -1654,6 +1655,17 @@ impl ConfigData { self.member_dir.maybe_pkg_json() } + pub fn maybe_jsx_import_source_config( + &self, + ) -> Option { + self + .member_dir + .workspace + .to_maybe_jsx_import_source_config() + .ok() + .flatten() + } + pub fn scope_contains_specifier(&self, specifier: &ModuleSpecifier) -> bool { specifier.as_str().starts_with(self.scope.as_str()) || self diff --git a/cli/lsp/diagnostics.rs b/cli/lsp/diagnostics.rs index 83c00d27edadea..e4fb82e58d5122 100644 --- a/cli/lsp/diagnostics.rs +++ b/cli/lsp/diagnostics.rs @@ -1707,6 +1707,7 @@ mod tests { documents: Arc::new(documents), assets: Default::default(), config: Arc::new(config), + is_cjs_resolver: Default::default(), resolver, }, ) diff --git a/cli/lsp/documents.rs b/cli/lsp/documents.rs index e00b0166aed548..b62fa85533856a 100644 --- a/cli/lsp/documents.rs +++ b/cli/lsp/documents.rs @@ -3,7 +3,9 @@ use super::cache::calculate_fs_version; use super::cache::LspCache; use super::config::Config; +use super::resolver::LspIsCjsResolver; use super::resolver::LspResolver; +use super::resolver::SingleReferrerGraphResolver; use super::testing::TestCollector; use super::testing::TestModule; use super::text::LineIndex; @@ -26,7 +28,6 @@ use deno_core::parking_lot::Mutex; use deno_core::ModuleSpecifier; use deno_graph::source::ResolutionMode; use deno_graph::Resolution; -use deno_path_util::url_from_directory_path; use deno_path_util::url_to_file_path; use deno_runtime::deno_node; use deno_semver::jsr::JsrPackageReqReference; @@ -34,7 +35,6 @@ use deno_semver::npm::NpmPackageReqReference; use deno_semver::package::PackageReq; use indexmap::IndexMap; use indexmap::IndexSet; -use node_resolver::InNpmPackageChecker; use node_resolver::NodeModuleKind; use std::borrow::Cow; use std::collections::BTreeMap; @@ -296,6 +296,8 @@ pub struct Document { /// Contains the last-known-good set of dependencies from parsing the module. config: Arc, dependencies: Arc>, + /// If this is maybe a CJS script and maybe not an ES module. + is_script: Option, // TODO(nayeemrmn): This is unused, use it for scope attribution for remote // modules. file_referrer: Option, @@ -326,6 +328,7 @@ impl Document { maybe_lsp_version: Option, maybe_language_id: Option, maybe_headers: Option>, + is_cjs_resolver: &LspIsCjsResolver, resolver: Arc, config: Arc, cache: &Arc, @@ -345,6 +348,7 @@ impl Document { maybe_headers.as_ref(), media_type, file_referrer.as_ref(), + is_cjs_resolver, &resolver, ) } else { @@ -370,6 +374,7 @@ impl Document { file_referrer.as_ref(), ), file_referrer, + is_script: maybe_module.as_ref().map(|m| m.is_script), maybe_types_dependency, line_index, maybe_language_id, @@ -391,6 +396,7 @@ impl Document { fn with_new_config( &self, + is_cjs_resolver: &LspIsCjsResolver, resolver: Arc, config: Arc, ) -> Arc { @@ -402,6 +408,7 @@ impl Document { let dependencies; let maybe_types_dependency; let maybe_parsed_source; + let is_script; let maybe_test_module_fut; if media_type != self.media_type { let parsed_source_result = @@ -411,6 +418,7 @@ impl Document { &parsed_source_result, self.maybe_headers.as_ref(), self.file_referrer.as_ref(), + is_cjs_resolver, &resolver, ) .ok(); @@ -418,6 +426,7 @@ impl Document { .as_ref() .map(|m| Arc::new(m.dependencies.clone())) .unwrap_or_default(); + is_script = maybe_module.as_ref().map(|m| m.is_script); maybe_types_dependency = maybe_module .as_ref() .and_then(|m| Some(Arc::new(m.maybe_types_dependency.clone()?))); @@ -428,6 +437,16 @@ impl Document { let cli_resolver = resolver.as_cli_resolver(self.file_referrer.as_ref()); let npm_resolver = resolver.create_graph_npm_resolver(self.file_referrer.as_ref()); + let config_data = resolver.as_config_data(self.file_referrer.as_ref()); + let jsx_import_source_config = + config_data.and_then(|d| d.maybe_jsx_import_source_config()); + let resolver = SingleReferrerGraphResolver { + valid_referrer: &self.specifier, + referrer_kind: is_cjs_resolver + .get_lsp_referrer_kind(&self.specifier, self.is_script), + cli_resolver, + jsx_import_source_config: jsx_import_source_config.as_ref(), + }; dependencies = Arc::new( self .dependencies @@ -438,7 +457,7 @@ impl Document { d.with_new_resolver( s, &CliJsrUrlProvider, - Some(cli_resolver), + Some(&resolver), Some(&npm_resolver), ), ) @@ -448,10 +467,11 @@ impl Document { maybe_types_dependency = self.maybe_types_dependency.as_ref().map(|d| { Arc::new(d.with_new_resolver( &CliJsrUrlProvider, - Some(cli_resolver), + Some(&resolver), Some(&npm_resolver), )) }); + is_script = self.is_script; maybe_parsed_source = self.maybe_parsed_source().cloned(); maybe_test_module_fut = self .maybe_test_module_fut @@ -463,6 +483,7 @@ impl Document { // updated properties dependencies, file_referrer: self.file_referrer.clone(), + is_script, maybe_types_dependency, maybe_navigation_tree: Mutex::new(None), // maintain - this should all be copies/clones @@ -487,6 +508,7 @@ impl Document { fn with_change( &self, + is_cjs_resolver: &LspIsCjsResolver, version: i32, changes: Vec, ) -> Result, AnyError> { @@ -520,6 +542,7 @@ impl Document { self.maybe_headers.as_ref(), media_type, self.file_referrer.as_ref(), + is_cjs_resolver, self.resolver.as_ref(), ) } else { @@ -543,6 +566,7 @@ impl Document { get_maybe_test_module_fut(maybe_parsed_source.as_ref(), &self.config); Ok(Arc::new(Self { config: self.config.clone(), + is_script: maybe_module.as_ref().map(|m| m.is_script), specifier: self.specifier.clone(), file_referrer: self.file_referrer.clone(), maybe_fs_version: self.maybe_fs_version.clone(), @@ -577,6 +601,7 @@ impl Document { ), maybe_language_id: self.maybe_language_id, dependencies: self.dependencies.clone(), + is_script: self.is_script, maybe_types_dependency: self.maybe_types_dependency.clone(), text: self.text.clone(), text_info_cell: once_cell::sync::OnceCell::new(), @@ -604,6 +629,7 @@ impl Document { ), maybe_language_id: self.maybe_language_id, dependencies: self.dependencies.clone(), + is_script: self.is_script, maybe_types_dependency: self.maybe_types_dependency.clone(), text: self.text.clone(), text_info_cell: once_cell::sync::OnceCell::new(), @@ -652,6 +678,13 @@ impl Document { }) } + /// If this is maybe a CJS script and maybe not an ES module. + /// + /// Use `LspIsCjsResolver` to determine for sure. + pub fn is_script(&self) -> Option { + self.is_script + } + pub fn line_index(&self) -> Arc { self.line_index.clone() } @@ -799,6 +832,7 @@ impl FileSystemDocuments { pub fn get( &self, specifier: &ModuleSpecifier, + is_cjs_resolver: &LspIsCjsResolver, resolver: &Arc, config: &Arc, cache: &Arc, @@ -822,7 +856,14 @@ impl FileSystemDocuments { }; if dirty { // attempt to update the file on the file system - self.refresh_document(specifier, resolver, config, cache, file_referrer) + self.refresh_document( + specifier, + is_cjs_resolver, + resolver, + config, + cache, + file_referrer, + ) } else { old_doc } @@ -833,6 +874,7 @@ impl FileSystemDocuments { fn refresh_document( &self, specifier: &ModuleSpecifier, + is_cjs_resolver: &LspIsCjsResolver, resolver: &Arc, config: &Arc, cache: &Arc, @@ -849,6 +891,7 @@ impl FileSystemDocuments { None, None, None, + is_cjs_resolver, resolver.clone(), config.clone(), cache, @@ -865,6 +908,7 @@ impl FileSystemDocuments { None, None, None, + is_cjs_resolver, resolver.clone(), config.clone(), cache, @@ -892,6 +936,7 @@ impl FileSystemDocuments { None, None, maybe_headers, + is_cjs_resolver, resolver.clone(), config.clone(), cache, @@ -932,6 +977,11 @@ pub struct Documents { /// The DENO_DIR that the documents looks for non-file based modules. cache: Arc, config: Arc, + /// Resolver for detecting if a document is CJS or ESM. + is_cjs_resolver: Arc, + /// A resolver that takes into account currently loaded import map and JSX + /// settings. + resolver: Arc, /// A flag that indicates that stated data is potentially invalid and needs to /// be recalculated before being considered valid. dirty: bool, @@ -939,9 +989,6 @@ pub struct Documents { open_docs: HashMap>, /// Documents stored on the file system. file_system_docs: Arc, - /// A resolver that takes into account currently loaded import map and JSX - /// settings. - resolver: Arc, /// The npm package requirements found in npm specifiers. npm_reqs_by_scope: Arc, BTreeSet>>, @@ -972,6 +1019,7 @@ impl Documents { // the cache for remote modules here in order to get the // x-typescript-types? None, + &self.is_cjs_resolver, self.resolver.clone(), self.config.clone(), &self.cache, @@ -1006,7 +1054,7 @@ impl Documents { )) })?; self.dirty = true; - let doc = doc.with_change(version, changes)?; + let doc = doc.with_change(&self.is_cjs_resolver, version, changes)?; self.open_docs.insert(doc.specifier().clone(), doc.clone()); Ok(doc) } @@ -1135,6 +1183,7 @@ impl Documents { if let Some(old_doc) = old_doc { self.file_system_docs.get( specifier, + &self.is_cjs_resolver, &self.resolver, &self.config, &self.cache, @@ -1159,6 +1208,7 @@ impl Documents { } else { self.file_system_docs.get( &specifier, + &self.is_cjs_resolver, &self.resolver, &self.config, &self.cache, @@ -1217,12 +1267,15 @@ impl Documents { referrer: &ModuleSpecifier, file_referrer: Option<&ModuleSpecifier>, ) -> Vec> { - let document = self.get(referrer); - let file_referrer = document + let referrer_doc = self.get(referrer); + let file_referrer = referrer_doc .as_ref() .and_then(|d| d.file_referrer()) .or(file_referrer); - let dependencies = document.as_ref().map(|d| d.dependencies()); + let dependencies = referrer_doc.as_ref().map(|d| d.dependencies()); + let referrer_kind = self + .is_cjs_resolver + .get_maybe_doc_module_kind(referrer, referrer_doc.as_deref()); let mut results = Vec::new(); for raw_specifier in raw_specifiers { if raw_specifier.starts_with("asset:") { @@ -1239,12 +1292,14 @@ impl Documents { results.push(self.resolve_dependency( specifier, referrer, + referrer_kind, file_referrer, )); } else if let Some(specifier) = dep.maybe_code.maybe_specifier() { results.push(self.resolve_dependency( specifier, referrer, + referrer_kind, file_referrer, )); } else { @@ -1258,12 +1313,14 @@ impl Documents { start: deno_graph::Position::zeroed(), end: deno_graph::Position::zeroed(), }, + referrer_kind, ResolutionMode::Types, ) { results.push(self.resolve_dependency( &specifier, referrer, + referrer_kind, file_referrer, )); } else { @@ -1282,6 +1339,7 @@ impl Documents { ) { self.config = Arc::new(config.clone()); self.cache = Arc::new(cache.clone()); + self.is_cjs_resolver = Arc::new(LspIsCjsResolver::new(cache)); self.resolver = resolver.clone(); node_resolver::PackageJsonThreadLocalCache::clear(); @@ -1305,14 +1363,21 @@ impl Documents { if !config.specifier_enabled(doc.specifier()) { continue; } - *doc = doc.with_new_config(self.resolver.clone(), self.config.clone()); + *doc = doc.with_new_config( + &self.is_cjs_resolver, + self.resolver.clone(), + self.config.clone(), + ); } for mut doc in self.file_system_docs.docs.iter_mut() { if !config.specifier_enabled(doc.specifier()) { continue; } - *doc.value_mut() = - doc.with_new_config(self.resolver.clone(), self.config.clone()); + *doc.value_mut() = doc.with_new_config( + &self.is_cjs_resolver, + self.resolver.clone(), + self.config.clone(), + ); } self.open_docs = open_docs; let mut preload_count = 0; @@ -1329,6 +1394,7 @@ impl Documents { { fs_docs.refresh_document( specifier, + &self.is_cjs_resolver, &self.resolver, &self.config, &self.cache, @@ -1443,7 +1509,8 @@ impl Documents { return Some((specifier, media_type)); }; if let Some(types) = doc.maybe_types_dependency().maybe_specifier() { - self.resolve_dependency(types, &specifier, file_referrer) + let specifier_kind = self.is_cjs_resolver.get_doc_module_kind(&doc); + self.resolve_dependency(types, &specifier, specifier_kind, file_referrer) } else { Some((doc.specifier().clone(), doc.media_type())) } @@ -1511,6 +1578,7 @@ fn parse_and_analyze_module( maybe_headers: Option<&HashMap>, media_type: MediaType, file_referrer: Option<&ModuleSpecifier>, + is_cjs_resolver: &LspIsCjsResolver, resolver: &LspResolver, ) -> (Option, Option) { let parsed_source_result = parse_source(specifier.clone(), text, media_type); @@ -1519,6 +1587,7 @@ fn parse_and_analyze_module( &parsed_source_result, maybe_headers, file_referrer, + is_cjs_resolver, resolver, ); (Some(parsed_source_result), Some(module_result)) @@ -1544,11 +1613,26 @@ fn analyze_module( parsed_source_result: &ParsedSourceResult, maybe_headers: Option<&HashMap>, file_referrer: Option<&ModuleSpecifier>, + is_cjs_resolver: &LspIsCjsResolver, resolver: &LspResolver, ) -> ModuleResult { match parsed_source_result { Ok(parsed_source) => { let npm_resolver = resolver.create_graph_npm_resolver(file_referrer); + let cli_resolver = resolver.as_cli_resolver(file_referrer); + let config_data = resolver.as_config_data(file_referrer); + let valid_referrer = specifier.clone(); + let jsx_import_source_config = + config_data.and_then(|d| d.maybe_jsx_import_source_config()); + let resolver = SingleReferrerGraphResolver { + valid_referrer: &valid_referrer, + referrer_kind: is_cjs_resolver.get_lsp_referrer_kind( + &specifier, + Some(parsed_source.compute_is_script()), + ), + cli_resolver, + jsx_import_source_config: jsx_import_source_config.as_ref(), + }; Ok(deno_graph::parse_module_from_ast( deno_graph::ParseModuleFromAstOptions { graph_kind: deno_graph::GraphKind::TypesOnly, @@ -1559,7 +1643,7 @@ fn analyze_module( // dynamic imports like import(`./dir/${something}`) in the LSP file_system: &deno_graph::source::NullFileSystem, jsr_url_provider: &CliJsrUrlProvider, - maybe_resolver: Some(resolver.as_cli_resolver(file_referrer)), + maybe_resolver: Some(&resolver), maybe_npm_resolver: Some(&npm_resolver), }, )) @@ -1570,37 +1654,6 @@ fn analyze_module( } } -#[derive(Debug)] -struct LspInNpmPackageChecker { - global_cache_dir: ModuleSpecifier, -} - -impl LspInNpmPackageChecker { - pub fn new(cache: &LspCache) -> Self { - Self { - global_cache_dir: url_from_directory_path( - &cache.deno_dir().npm_folder_path(), - ) - .unwrap_or_else(|_| ModuleSpecifier::parse("file:///invalid/").unwrap()), - } - } -} - -impl InNpmPackageChecker for LspInNpmPackageChecker { - fn in_npm_package(&self, specifier: &ModuleSpecifier) -> bool { - if specifier.scheme() != "file" { - return false; - } - if specifier - .as_str() - .starts_with(self.global_cache_dir.as_str()) - { - return true; - } - specifier.as_str().contains("/node_modules/") - } -} - #[cfg(test)] mod tests { use super::*; diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index 1176cd41e3ad09..b2bd72416ac8c7 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -78,7 +78,7 @@ use super::parent_process_checker; use super::performance::Performance; use super::refactor; use super::registries::ModuleRegistry; -use super::resolver::LspCjsTracker; +use super::resolver::LspIsCjsResolver; use super::resolver::LspResolver; use super::testing; use super::text; @@ -146,6 +146,7 @@ pub struct StateSnapshot { pub project_version: usize, pub assets: AssetsSnapshot, pub config: Arc, + pub is_cjs_resolver: Arc, pub documents: Arc, pub resolver: Arc, } @@ -196,7 +197,6 @@ pub struct Inner { cache: LspCache, /// The LSP client that this LSP server is connected to. pub client: Client, - cjs_tracker: Arc, /// Configuration information. pub config: Config, diagnostics_state: Arc, @@ -206,6 +206,7 @@ pub struct Inner { pub documents: Documents, http_client_provider: Arc, initial_cwd: PathBuf, + pub is_cjs_resolver: Arc, jsr_search_api: CliJsrSearchApi, /// Handles module registries, which allow discovery of modules module_registry: ModuleRegistry, @@ -465,7 +466,6 @@ impl Inner { cache.deno_dir().registries_folder_path(), http_client_provider.clone(), ); - let cjs_tracker = Arc::new(LspCjsTracker::new(&cache)); let jsr_search_api = CliJsrSearchApi::new(module_registry.file_fetcher.clone()); let npm_search_api = @@ -484,11 +484,11 @@ impl Inner { let initial_cwd = std::env::current_dir().unwrap_or_else(|_| { panic!("Could not resolve current working directory") }); + let is_cjs_resolver = Arc::new(LspIsCjsResolver::new(&cache)); Self { assets, cache, - cjs_tracker, client, config, diagnostics_state, @@ -496,6 +496,7 @@ impl Inner { documents, http_client_provider, initial_cwd: initial_cwd.clone(), + is_cjs_resolver, jsr_search_api, project_version: 0, task_queue: Default::default(), @@ -606,6 +607,7 @@ impl Inner { project_version: self.project_version, assets: self.assets.snapshot(), config: Arc::new(self.config.clone()), + is_cjs_resolver: self.is_cjs_resolver.clone(), documents: Arc::new(self.documents.clone()), resolver: self.resolver.snapshot(), }) @@ -627,6 +629,7 @@ impl Inner { } }); self.cache = LspCache::new(global_cache_url); + self.is_cjs_resolver = Arc::new(LspIsCjsResolver::new(&self.cache)); let deno_dir = self.cache.deno_dir(); let workspace_settings = self.config.workspace_settings(); let maybe_root_path = self @@ -1628,6 +1631,10 @@ impl Inner { let file_diagnostics = self .diagnostics_server .get_ts_diagnostics(&specifier, asset_or_doc.document_lsp_version()); + let specifier_kind = asset_or_doc + .document() + .map(|d| self.is_cjs_resolver.get_doc_module_kind(d)) + .unwrap_or(NodeModuleKind::Esm); let mut includes_no_cache = false; for diagnostic in &fixable_diagnostics { match diagnostic.source.as_deref() { @@ -1666,7 +1673,13 @@ impl Inner { .await; for action in actions { code_actions - .add_ts_fix_action(&specifier, &action, diagnostic, self) + .add_ts_fix_action( + &specifier, + specifier_kind, + &action, + diagnostic, + self, + ) .map_err(|err| { error!("Unable to convert fix: {:#}", err); LspError::internal_error() @@ -1812,10 +1825,9 @@ impl Inner { error!("Unable to decode code action data: {:#}", err); LspError::invalid_params("The CodeAction's data is invalid.") })?; - let scope = self - .get_asset_or_document(&code_action_data.specifier) - .ok() - .and_then(|d| d.scope().cloned()); + let maybe_asset_or_doc = + self.get_asset_or_document(&code_action_data.specifier).ok(); + let scope = maybe_asset_or_doc.as_ref().and_then(|d| d.scope().cloned()); let combined_code_actions = self .ts_server .get_combined_code_fix( @@ -1842,6 +1854,11 @@ impl Inner { let changes = if code_action_data.fix_id == "fixMissingImport" { fix_ts_import_changes( &code_action_data.specifier, + maybe_asset_or_doc + .as_ref() + .and_then(|d| d.document()) + .map(|d| self.is_cjs_resolver.get_doc_module_kind(d)) + .unwrap_or(NodeModuleKind::Esm), &combined_code_actions.changes, self, ) @@ -1895,6 +1912,10 @@ impl Inner { if kind_suffix == ".rewrite.function.returnType" { refactor_edit_info.edits = fix_ts_import_changes( &action_data.specifier, + asset_or_doc + .document() + .map(|d| self.is_cjs_resolver.get_doc_module_kind(d)) + .unwrap_or(NodeModuleKind::Esm), &refactor_edit_info.edits, self, ) @@ -2244,6 +2265,7 @@ impl Inner { &self.jsr_search_api, &self.npm_search_api, &self.documents, + &self.is_cjs_resolver, self.resolver.as_ref(), self .config diff --git a/cli/lsp/resolver.rs b/cli/lsp/resolver.rs index 8f8e20f3af48a7..10fe7c66c8124f 100644 --- a/cli/lsp/resolver.rs +++ b/cli/lsp/resolver.rs @@ -1,17 +1,17 @@ // Copyright 2018-2024 the Deno authors. All rights reserved. MIT license. use dashmap::DashMap; -use deno_ast::view::Module; use deno_ast::MediaType; -use deno_ast::ParsedSource; use deno_cache_dir::npm::NpmCacheDir; use deno_cache_dir::HttpCache; +use deno_config::deno_json::JsxImportSourceConfig; use deno_config::workspace::PackageJsonDepResolution; use deno_config::workspace::WorkspaceResolver; use deno_core::url::Url; -use deno_graph::source::Resolver; +use deno_graph::source::ResolutionMode; use deno_graph::GraphImport; use deno_graph::ModuleSpecifier; +use deno_graph::Range; use deno_npm::NpmSystemInfo; use deno_path_util::url_from_directory_path; use deno_path_util::url_to_file_path; @@ -36,6 +36,7 @@ use std::collections::HashSet; use std::sync::Arc; use super::cache::LspCache; +use super::documents::Document; use super::jsr::JsrCacheResolver; use crate::args::create_default_npmrc; use crate::args::CacheSetting; @@ -56,12 +57,11 @@ use crate::npm::CliNpmResolverCreateOptions; use crate::npm::CliNpmResolverManagedSnapshotOption; use crate::npm::CreateInNpmPkgCheckerOptions; use crate::npm::ManagedCliNpmResolver; -use crate::resolver::CjsTracker; -use crate::resolver::CjsTrackerOptions; use crate::resolver::CliDenoResolverFs; use crate::resolver::CliNodeResolver; use crate::resolver::CliResolver; use crate::resolver::CliResolverOptions; +use crate::resolver::IsCjsResolver; use crate::resolver::WorkerCliNpmGraphResolver; use crate::tsc::into_specifier_and_media_type; use crate::util::progress_bar::ProgressBar; @@ -104,7 +104,6 @@ impl LspScopeResolver { ) -> Self { let mut npm_resolver = None; let mut node_resolver = None; - let mut lsp_cjs_tracker = None; let fs = Arc::new(deno_fs::RealFs); let pkg_json_resolver = Arc::new(PackageJsonResolver::new( deno_runtime::deno_node::DenoFsNodeResolverEnv::new(fs.clone()), @@ -127,7 +126,7 @@ impl LspScopeResolver { )); } } - let graph_resolver = create_cli_resolver( + let cli_resolver = create_cli_resolver( config_data.map(|d| d.as_ref()), npm_resolver.as_ref(), node_resolver.as_ref(), @@ -140,7 +139,9 @@ impl LspScopeResolver { cache.for_specifier(config_data.map(|d| d.scope.as_ref())), config_data.and_then(|d| d.lockfile.clone()), ))); - let npm_graph_resolver = graph_resolver.create_graph_npm_resolver(); + let npm_graph_resolver = cli_resolver.create_graph_npm_resolver(); + let maybe_jsx_import_source_config = + config_data.and_then(|d| d.maybe_jsx_import_source_config()); let graph_imports = config_data .and_then(|d| d.member_dir.workspace.to_compiler_option_types().ok()) .map(|imports| { @@ -148,11 +149,18 @@ impl LspScopeResolver { imports .into_iter() .map(|(referrer, imports)| { + let resolver = SingleReferrerGraphResolver { + valid_referrer: &referrer, + referrer_kind: NodeModuleKind::Esm, + cli_resolver: &cli_resolver, + jsx_import_source_config: maybe_jsx_import_source_config + .as_ref(), + }; let graph_import = GraphImport::new( &referrer, imports, &CliJsrUrlProvider, - Some(graph_resolver.as_ref()), + Some(&resolver), Some(&npm_graph_resolver), ); (referrer, graph_import) @@ -191,7 +199,7 @@ impl LspScopeResolver { let package_json_deps_by_resolution = Arc::new(package_json_deps_by_resolution.unwrap_or_default()); Self { - resolver: graph_resolver, + resolver: cli_resolver, jsr_resolver, npm_resolver, node_resolver, @@ -337,6 +345,14 @@ impl LspResolver { resolver.resolver.create_graph_npm_resolver() } + pub fn as_config_data( + &self, + file_referrer: Option<&ModuleSpecifier>, + ) -> Option<&Arc> { + let resolver = self.get_scope_resolver(file_referrer); + resolver.config_data.as_ref() + } + pub fn maybe_node_resolver( &self, file_referrer: Option<&ModuleSpecifier>, @@ -645,7 +661,6 @@ fn create_cli_resolver( npm_resolver: Option<&Arc>, node_resolver: Option<&Arc>, ) -> Arc { - let workspace = config_data.map(|d| &d.member_dir.workspace); Arc::new(CliResolver::new(CliResolverOptions { node_resolver: node_resolver.cloned(), npm_resolver: npm_resolver.cloned(), @@ -661,9 +676,6 @@ fn create_cli_resolver( )) }, ), - maybe_jsx_import_source_config: workspace.and_then(|workspace| { - workspace.to_maybe_jsx_import_source_config().ok().flatten() - }), maybe_vendor_dir: config_data.and_then(|d| d.vendor_dir.as_ref()), bare_node_builtins_enabled: config_data .is_some_and(|d| d.unstable.contains("bare-node-builtins")), @@ -696,6 +708,136 @@ impl std::fmt::Debug for RedirectResolver { } } +#[derive(Debug)] +pub struct LspIsCjsResolver { + inner: IsCjsResolver, +} + +impl Default for LspIsCjsResolver { + fn default() -> Self { + LspIsCjsResolver::new(&Default::default()) + } +} + +impl LspIsCjsResolver { + pub fn new(cache: &LspCache) -> Self { + #[derive(Debug)] + struct LspInNpmPackageChecker { + global_cache_dir: ModuleSpecifier, + } + + impl LspInNpmPackageChecker { + pub fn new(cache: &LspCache) -> Self { + Self { + global_cache_dir: url_from_directory_path( + &cache.deno_dir().npm_folder_path(), + ) + .unwrap_or_else(|_| { + ModuleSpecifier::parse("file:///invalid/").unwrap() + }), + } + } + } + + impl InNpmPackageChecker for LspInNpmPackageChecker { + fn in_npm_package(&self, specifier: &ModuleSpecifier) -> bool { + if specifier.scheme() != "file" { + return false; + } + if specifier + .as_str() + .starts_with(self.global_cache_dir.as_str()) + { + return true; + } + specifier.as_str().contains("/node_modules/") + } + } + + let fs = Arc::new(deno_fs::RealFs); + let pkg_json_resolver = Arc::new(PackageJsonResolver::new( + deno_runtime::deno_node::DenoFsNodeResolverEnv::new(fs.clone()), + )); + + LspIsCjsResolver { + inner: IsCjsResolver::new( + Arc::new(LspInNpmPackageChecker::new(cache)), + pkg_json_resolver, + crate::resolver::IsCjsResolverOptions { detect_cjs: true }, + ), + } + } + + pub fn get_maybe_doc_module_kind( + &self, + specifier: &ModuleSpecifier, + maybe_document: Option<&Document>, + ) -> NodeModuleKind { + self.get_lsp_referrer_kind( + specifier, + maybe_document.and_then(|d| d.is_script()), + ) + } + + pub fn get_doc_module_kind(&self, document: &Document) -> NodeModuleKind { + self.get_lsp_referrer_kind(document.specifier(), document.is_script()) + } + + pub fn get_lsp_referrer_kind( + &self, + specifier: &ModuleSpecifier, + is_script: Option, + ) -> NodeModuleKind { + self.inner.get_lsp_referrer_kind(specifier, is_script) + } +} + +#[derive(Debug)] +pub struct SingleReferrerGraphResolver<'a> { + pub valid_referrer: &'a ModuleSpecifier, + pub referrer_kind: NodeModuleKind, + pub cli_resolver: &'a CliResolver, + pub jsx_import_source_config: Option<&'a JsxImportSourceConfig>, +} + +impl<'a> deno_graph::source::Resolver for SingleReferrerGraphResolver<'a> { + fn default_jsx_import_source(&self) -> Option { + self + .jsx_import_source_config + .and_then(|c| c.default_specifier.clone()) + } + + fn default_jsx_import_source_types(&self) -> Option { + self + .jsx_import_source_config + .and_then(|c| c.default_types_specifier.clone()) + } + + fn jsx_import_source_module(&self) -> &str { + self + .jsx_import_source_config + .map(|c| c.module.as_str()) + .unwrap_or(deno_graph::source::DEFAULT_JSX_IMPORT_SOURCE_MODULE) + } + + fn resolve( + &self, + specifier_text: &str, + referrer_range: &Range, + mode: ResolutionMode, + ) -> Result { + // this resolver assumes it will only be used with a single referrer + // with the provided referrer kind + debug_assert_eq!(referrer_range.specifier, *self.valid_referrer); + self.cli_resolver.resolve( + specifier_text, + referrer_range, + self.referrer_kind, + mode, + ) + } +} + impl RedirectResolver { fn new( cache: Arc, diff --git a/cli/lsp/tsc.rs b/cli/lsp/tsc.rs index 6f63ced5be8a0a..c9b24176ad2e0a 100644 --- a/cli/lsp/tsc.rs +++ b/cli/lsp/tsc.rs @@ -69,6 +69,7 @@ use indexmap::IndexMap; use indexmap::IndexSet; use lazy_regex::lazy_regex; use log::error; +use node_resolver::NodeModuleKind; use once_cell::sync::Lazy; use regex::Captures; use regex::Regex; @@ -4401,25 +4402,15 @@ fn op_load<'s>( None } else { let asset_or_document = state.get_asset_or_document(&specifier); - asset_or_document.map(|doc| { - let maybe_cjs_tracker = state - .state_snapshot - .resolver - .maybe_cjs_tracker(Some(&specifier)); - LoadResponse { - data: doc.text(), - script_kind: crate::tsc::as_ts_script_kind(doc.media_type()), - version: state.script_version(&specifier), - is_cjs: maybe_cjs_tracker - .map(|t| { - t.is_cjs( - &specifier, - doc.media_type(), - doc.maybe_parsed_source().and_then(|p| p.as_ref().ok()), - ) - }) - .unwrap_or(false), - } + asset_or_document.map(|doc| LoadResponse { + data: doc.text(), + script_kind: crate::tsc::as_ts_script_kind(doc.media_type()), + version: state.script_version(&specifier), + is_cjs: doc + .document() + .map(|d| state.state_snapshot.is_cjs_resolver.get_doc_module_kind(d)) + .unwrap_or(NodeModuleKind::Esm) + == NodeModuleKind::Cjs, }) }; @@ -4662,6 +4653,10 @@ fn op_script_names(state: &mut OpState) -> ScriptNames { let (types, _) = documents.resolve_dependency( types, specifier, + state + .state_snapshot + .is_cjs_resolver + .get_doc_module_kind(doc), doc.file_referrer(), )?; let types_doc = documents.get_or_load(&types, doc.file_referrer())?; @@ -5544,6 +5539,7 @@ mod tests { documents: Arc::new(documents), assets: Default::default(), config: Arc::new(config), + is_cjs_resolver: Default::default(), resolver, }); let performance = Arc::new(Performance::default()); diff --git a/cli/module_loader.rs b/cli/module_loader.rs index 7da070b00b7025..f94a5973a69e17 100644 --- a/cli/module_loader.rs +++ b/cli/module_loader.rs @@ -60,7 +60,6 @@ use deno_core::RequestedModuleType; use deno_core::ResolutionKind; use deno_core::SourceCodeCacheInfo; use deno_graph::source::ResolutionMode; -use deno_graph::source::Resolver; use deno_graph::GraphKind; use deno_graph::JsModule; use deno_graph::JsonModule; @@ -74,7 +73,6 @@ use deno_runtime::deno_node::NodeRequireLoader; use deno_runtime::deno_permissions::PermissionsContainer; use deno_semver::npm::NpmPackageReqReference; use node_resolver::InNpmPackageChecker; -use node_resolver::NodeModuleKind; use node_resolver::NodeResolutionMode; pub struct ModuleLoadPreparer { diff --git a/cli/resolver.rs b/cli/resolver.rs index d11bf92131d5e5..14dc346e2b5a2e 100644 --- a/cli/resolver.rs +++ b/cli/resolver.rs @@ -16,7 +16,6 @@ use deno_core::ModuleSourceCode; use deno_core::ModuleSpecifier; use deno_graph::source::ResolutionMode; use deno_graph::source::ResolveError; -use deno_graph::source::Resolver; use deno_graph::source::UnknownBuiltInNodeModuleError; use deno_graph::NpmLoadError; use deno_graph::NpmResolvePkgReqsResult; @@ -50,7 +49,6 @@ use std::path::PathBuf; use std::sync::Arc; use thiserror::Error; -use crate::args::JsxImportSourceConfig; use crate::args::DENO_DISABLE_PEDANTIC_NODE_WARNINGS; use crate::node::CliNodeCodeTranslator; use crate::npm::CliNpmResolver; @@ -411,10 +409,6 @@ impl NpmModuleLoader { } } -pub struct CjsTrackerOptions { - pub detect_cjs: bool, -} - /// Keeps track of what module specifiers were resolved as CJS. /// /// Modules that are `.js` or `.ts` are only known to be CJS or @@ -422,9 +416,7 @@ pub struct CjsTrackerOptions { /// will be "maybe CJS" until they're loaded. #[derive(Debug)] pub struct CjsTracker { - in_npm_pkg_checker: Arc, - pkg_json_resolver: Arc, - detect_cjs: bool, + is_cjs_resolver: IsCjsResolver, known: DashMap, } @@ -432,12 +424,14 @@ impl CjsTracker { pub fn new( in_npm_pkg_checker: Arc, pkg_json_resolver: Arc, - options: CjsTrackerOptions, + options: IsCjsResolverOptions, ) -> Self { Self { - in_npm_pkg_checker, - pkg_json_resolver, - detect_cjs: options.detect_cjs, + is_cjs_resolver: IsCjsResolver::new( + in_npm_pkg_checker, + pkg_json_resolver, + options, + ), known: Default::default(), } } @@ -477,7 +471,7 @@ impl CjsTracker { .get_known_kind_with_is_script(specifier, media_type, is_script) { Some(kind) => kind, - None => self.check_based_on_pkg_json(specifier)?, + None => self.is_cjs_resolver.check_based_on_pkg_json(specifier)?, }; Ok(kind == NodeModuleKind::Cjs) } @@ -514,6 +508,64 @@ impl CjsTracker { specifier: &ModuleSpecifier, media_type: MediaType, is_script: Option, + ) -> Option { + self.is_cjs_resolver.get_known_kind_with_is_script( + specifier, + media_type, + is_script, + Some(&self.known), + ) + } +} + +pub struct IsCjsResolverOptions { + pub detect_cjs: bool, +} + +#[derive(Debug)] +pub struct IsCjsResolver { + in_npm_pkg_checker: Arc, + pkg_json_resolver: Arc, + detect_cjs: bool, +} + +impl IsCjsResolver { + pub fn new( + in_npm_pkg_checker: Arc, + pkg_json_resolver: Arc, + options: IsCjsResolverOptions, + ) -> Self { + Self { + in_npm_pkg_checker, + pkg_json_resolver, + detect_cjs: options.detect_cjs, + } + } + + pub fn get_lsp_referrer_kind( + &self, + specifier: &ModuleSpecifier, + is_script: Option, + ) -> NodeModuleKind { + if specifier.scheme() != "file" { + return NodeModuleKind::Esm; + } + self + .get_known_kind_with_is_script( + specifier, + MediaType::from_specifier(specifier), + is_script, + None, + ) + .unwrap_or(NodeModuleKind::Esm) + } + + fn get_known_kind_with_is_script( + &self, + specifier: &ModuleSpecifier, + media_type: MediaType, + is_script: Option, + known_cache: Option<&DashMap>, ) -> Option { if specifier.scheme() != "file" { return Some(NodeModuleKind::Esm); @@ -525,12 +577,14 @@ impl CjsTracker { MediaType::Dts => { // dts files are always determined based on the package.json because // they contain imports/exports even when considered CJS - if let Some(value) = self.known.get(specifier).map(|v| *v) { + if let Some(value) = known_cache.and_then(|c| c.get(specifier)).map(|v| *v) { Some(value) } else { let value = self.check_based_on_pkg_json(specifier).ok(); if let Some(value) = value { - self.known.insert(specifier.clone(), value); + if let Some(known_cache) = known_cache { + known_cache.insert(specifier.clone(), value); + } } Some(value.unwrap_or(NodeModuleKind::Esm)) } @@ -545,17 +599,21 @@ impl CjsTracker { | MediaType::Css | MediaType::SourceMap | MediaType::Unknown => { - if let Some(value) = self.known.get(specifier).map(|v| *v) { + if let Some(value) = known_cache.and_then(|c| c.get(specifier)).map(|v| *v) { if value == NodeModuleKind::Cjs && is_script == Some(false) { // we now know this is actually esm - self.known.insert(specifier.clone(), NodeModuleKind::Esm); + if let Some(known_cache) = known_cache { + known_cache.insert(specifier.clone(), NodeModuleKind::Esm); + } Some(NodeModuleKind::Esm) } else { Some(value) } } else if is_script == Some(false) { // we know this is esm - self.known.insert(specifier.clone(), NodeModuleKind::Esm); + if let Some(known_cache) = known_cache { + known_cache.insert(specifier.clone(), NodeModuleKind::Esm); + } Some(NodeModuleKind::Esm) } else { None @@ -623,7 +681,6 @@ pub struct CliResolverOptions<'a> { pub sloppy_imports_resolver: Option>, pub workspace_resolver: Arc, pub bare_node_builtins_enabled: bool, - pub maybe_jsx_import_source_config: Option, pub maybe_vendor_dir: Option<&'a PathBuf>, } diff --git a/cli/standalone/mod.rs b/cli/standalone/mod.rs index ca65871dd874c5..562b8984b03d31 100644 --- a/cli/standalone/mod.rs +++ b/cli/standalone/mod.rs @@ -77,9 +77,9 @@ use crate::npm::CliNpmResolverCreateOptions; use crate::npm::CliNpmResolverManagedSnapshotOption; use crate::npm::CreateInNpmPkgCheckerOptions; use crate::resolver::CjsTracker; -use crate::resolver::CjsTrackerOptions; use crate::resolver::CliDenoResolverFs; use crate::resolver::CliNodeResolver; +use crate::resolver::IsCjsResolverOptions; use crate::resolver::NpmModuleLoader; use crate::util::progress_bar::ProgressBar; use crate::util::progress_bar::ProgressBarStyle; @@ -648,7 +648,7 @@ pub async fn run(data: StandaloneData) -> Result { let cjs_tracker = Arc::new(CjsTracker::new( in_npm_pkg_checker.clone(), pkg_json_resolver.clone(), - CjsTrackerOptions { + IsCjsResolverOptions { detect_cjs: !metadata.workspace_resolver.package_jsons.is_empty(), }, )); diff --git a/cli/tools/repl/session.rs b/cli/tools/repl/session.rs index 90284e97a5d46b..e35af4aa3e8f26 100644 --- a/cli/tools/repl/session.rs +++ b/cli/tools/repl/session.rs @@ -44,12 +44,12 @@ use deno_core::url::Url; use deno_core::LocalInspectorSession; use deno_core::PollEventLoopOptions; use deno_graph::source::ResolutionMode; -use deno_graph::source::Resolver; use deno_graph::Position; use deno_graph::PositionRange; use deno_graph::SpecifierWithRange; use deno_runtime::worker::MainWorker; use deno_semver::npm::NpmPackageReqReference; +use node_resolver::NodeModuleKind; use once_cell::sync::Lazy; use regex::Match; use regex::Regex; @@ -712,7 +712,12 @@ impl ReplSession { .flat_map(|i| { self .resolver - .resolve(i, &referrer_range, ResolutionMode::Execution) + .resolve( + i, + &referrer_range, + NodeModuleKind::Esm, + ResolutionMode::Execution, + ) .ok() .or_else(|| ModuleSpecifier::parse(i).ok()) }) From e3134c0a2d85faf5d9fd0de394d1666f244180aa Mon Sep 17 00:00:00 2001 From: David Sherret Date: Mon, 11 Nov 2024 16:59:03 -0500 Subject: [PATCH 15/22] fix tests --- cli/resolver.rs | 9 +-------- cli/tools/installer.rs | 1 + 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/cli/resolver.rs b/cli/resolver.rs index 14dc346e2b5a2e..e3f41238dc43d1 100644 --- a/cli/resolver.rs +++ b/cli/resolver.rs @@ -493,14 +493,7 @@ impl CjsTracker { } self .get_known_kind(specifier, MediaType::from_specifier(specifier)) - .unwrap_or_else(|| { - debug_assert!( - false, - "referrer '{}' should always have a known kind", - specifier - ); - NodeModuleKind::Esm - }) + .unwrap_or(NodeModuleKind::Esm) } fn get_known_kind_with_is_script( diff --git a/cli/tools/installer.rs b/cli/tools/installer.rs index ed86e86c79bf0f..1655f0a322eaff 100644 --- a/cli/tools/installer.rs +++ b/cli/tools/installer.rs @@ -1396,6 +1396,7 @@ mod tests { .env_clear() // use the deno binary in the target directory .env("PATH", test_util::target_dir()) + .env("RUST_BACKTRACE", "1") .spawn() .unwrap() .wait() From c8f5141fdf04aa54954569a609f7551c34e1927d Mon Sep 17 00:00:00 2001 From: David Sherret Date: Mon, 11 Nov 2024 17:33:26 -0500 Subject: [PATCH 16/22] fix issue with type checking not resolving based on package.json --- cli/tsc/mod.rs | 19 +++++++++++++++++-- tests/specs/npm/dual_cjs_esm/__test__.jsonc | 4 ---- .../dual_cjs_esm/cjs_referrer/__test__.jsonc | 14 ++++++++++++++ .../npm/dual_cjs_esm/cjs_referrer/check.out | 8 ++++++++ .../npm/dual_cjs_esm/cjs_referrer/main.cts | 4 ++++ .../npm/dual_cjs_esm/cjs_referrer/main.out | 4 ++++ .../dual_cjs_esm/cjs_referrer/package.json | 5 +++++ .../dual_cjs_esm/esm_referrer/__test__.jsonc | 4 ++++ .../{dual_cjs_esm => esm_referrer}/main.out | 0 .../{dual_cjs_esm => esm_referrer}/main.ts | 0 .../ts_referrer_type_cjs/__test__.jsonc | 14 ++++++++++++++ .../ts_referrer_type_cjs/check.out | 8 ++++++++ .../ts_referrer_type_cjs/main.out | 4 ++++ .../dual_cjs_esm/ts_referrer_type_cjs/main.ts | 4 ++++ .../ts_referrer_type_cjs/package.json | 6 ++++++ 15 files changed, 92 insertions(+), 6 deletions(-) delete mode 100644 tests/specs/npm/dual_cjs_esm/__test__.jsonc create mode 100644 tests/specs/npm/dual_cjs_esm/cjs_referrer/__test__.jsonc create mode 100644 tests/specs/npm/dual_cjs_esm/cjs_referrer/check.out create mode 100644 tests/specs/npm/dual_cjs_esm/cjs_referrer/main.cts create mode 100644 tests/specs/npm/dual_cjs_esm/cjs_referrer/main.out create mode 100644 tests/specs/npm/dual_cjs_esm/cjs_referrer/package.json create mode 100644 tests/specs/npm/dual_cjs_esm/esm_referrer/__test__.jsonc rename tests/specs/npm/dual_cjs_esm/{dual_cjs_esm => esm_referrer}/main.out (100%) rename tests/specs/npm/dual_cjs_esm/{dual_cjs_esm => esm_referrer}/main.ts (100%) create mode 100644 tests/specs/npm/dual_cjs_esm/ts_referrer_type_cjs/__test__.jsonc create mode 100644 tests/specs/npm/dual_cjs_esm/ts_referrer_type_cjs/check.out create mode 100644 tests/specs/npm/dual_cjs_esm/ts_referrer_type_cjs/main.out create mode 100644 tests/specs/npm/dual_cjs_esm/ts_referrer_type_cjs/main.ts create mode 100644 tests/specs/npm/dual_cjs_esm/ts_referrer_type_cjs/package.json diff --git a/cli/tsc/mod.rs b/cli/tsc/mod.rs index cd9b86cd71cd84..a569061625a7a5 100644 --- a/cli/tsc/mod.rs +++ b/cli/tsc/mod.rs @@ -363,6 +363,17 @@ impl TypeCheckingCjsTracker { .unwrap_or(false) }) } + + pub fn is_cjs_with_known_is_script( + &self, + specifier: &ModuleSpecifier, + media_type: MediaType, + is_script: bool, + ) -> Result { + self + .cjs_tracker + .is_cjs_with_known_is_script(specifier, media_type, is_script) + } } #[derive(Debug)] @@ -621,8 +632,12 @@ fn op_load_inner( match module { Module::Js(module) => { media_type = module.media_type; - if matches!(media_type, MediaType::Cjs | MediaType::Cts) { - is_cjs = true; + if let Some(npm_state) = &state.maybe_npm { + is_cjs = npm_state.cjs_tracker.is_cjs_with_known_is_script( + specifier, + module.media_type, + module.is_script, + )?; } let source = module .fast_check_module() diff --git a/tests/specs/npm/dual_cjs_esm/__test__.jsonc b/tests/specs/npm/dual_cjs_esm/__test__.jsonc deleted file mode 100644 index f2b0d694e3b559..00000000000000 --- a/tests/specs/npm/dual_cjs_esm/__test__.jsonc +++ /dev/null @@ -1,4 +0,0 @@ -{ - "args": "run -A --quiet dual_cjs_esm/main.ts", - "output": "dual_cjs_esm/main.out" -} diff --git a/tests/specs/npm/dual_cjs_esm/cjs_referrer/__test__.jsonc b/tests/specs/npm/dual_cjs_esm/cjs_referrer/__test__.jsonc new file mode 100644 index 00000000000000..62777cdaa13073 --- /dev/null +++ b/tests/specs/npm/dual_cjs_esm/cjs_referrer/__test__.jsonc @@ -0,0 +1,14 @@ +{ + "tempDir": true, + "tests": { + "check": { + "args": "check --node-modules-dir=auto main.cts", + "output": "check.out", + "exitCode": 1 + }, + "run": { + "args": "run --node-modules-dir=auto main.cts", + "output": "main.out" + } + } +} diff --git a/tests/specs/npm/dual_cjs_esm/cjs_referrer/check.out b/tests/specs/npm/dual_cjs_esm/cjs_referrer/check.out new file mode 100644 index 00000000000000..267d31fb7564a4 --- /dev/null +++ b/tests/specs/npm/dual_cjs_esm/cjs_referrer/check.out @@ -0,0 +1,8 @@ +Download http://localhost:4260/@denotest%2fdual-cjs-esm +Download http://localhost:4260/@denotest/dual-cjs-esm/1.0.0.tgz +Initialize @denotest/dual-cjs-esm@1.0.0 +Check file:///[WILDLINE]/main.cts +error: TS2322 [ERROR]: Type '"cjs"' is not assignable to type '"other"'. +const kind: "other" = mod.getKind(); + ~~~~ + at file:///[WILDLINE]/main.cts:3:7 diff --git a/tests/specs/npm/dual_cjs_esm/cjs_referrer/main.cts b/tests/specs/npm/dual_cjs_esm/cjs_referrer/main.cts new file mode 100644 index 00000000000000..b8dd343f8ba4ac --- /dev/null +++ b/tests/specs/npm/dual_cjs_esm/cjs_referrer/main.cts @@ -0,0 +1,4 @@ +import mod = require("@denotest/dual-cjs-esm"); + +const kind: "other" = mod.getKind(); +console.log(kind); diff --git a/tests/specs/npm/dual_cjs_esm/cjs_referrer/main.out b/tests/specs/npm/dual_cjs_esm/cjs_referrer/main.out new file mode 100644 index 00000000000000..62ddbf47931db6 --- /dev/null +++ b/tests/specs/npm/dual_cjs_esm/cjs_referrer/main.out @@ -0,0 +1,4 @@ +Download http://localhost:4260/@denotest%2fdual-cjs-esm +Download http://localhost:4260/@denotest/dual-cjs-esm/1.0.0.tgz +Initialize @denotest/dual-cjs-esm@1.0.0 +cjs diff --git a/tests/specs/npm/dual_cjs_esm/cjs_referrer/package.json b/tests/specs/npm/dual_cjs_esm/cjs_referrer/package.json new file mode 100644 index 00000000000000..e1b1e1a5f8f1cf --- /dev/null +++ b/tests/specs/npm/dual_cjs_esm/cjs_referrer/package.json @@ -0,0 +1,5 @@ +{ + "dependencies": { + "@denotest/dual-cjs-esm": "*" + } +} diff --git a/tests/specs/npm/dual_cjs_esm/esm_referrer/__test__.jsonc b/tests/specs/npm/dual_cjs_esm/esm_referrer/__test__.jsonc new file mode 100644 index 00000000000000..0ef147253652df --- /dev/null +++ b/tests/specs/npm/dual_cjs_esm/esm_referrer/__test__.jsonc @@ -0,0 +1,4 @@ +{ + "args": "run -A --quiet main.ts", + "output": "main.out" +} diff --git a/tests/specs/npm/dual_cjs_esm/dual_cjs_esm/main.out b/tests/specs/npm/dual_cjs_esm/esm_referrer/main.out similarity index 100% rename from tests/specs/npm/dual_cjs_esm/dual_cjs_esm/main.out rename to tests/specs/npm/dual_cjs_esm/esm_referrer/main.out diff --git a/tests/specs/npm/dual_cjs_esm/dual_cjs_esm/main.ts b/tests/specs/npm/dual_cjs_esm/esm_referrer/main.ts similarity index 100% rename from tests/specs/npm/dual_cjs_esm/dual_cjs_esm/main.ts rename to tests/specs/npm/dual_cjs_esm/esm_referrer/main.ts diff --git a/tests/specs/npm/dual_cjs_esm/ts_referrer_type_cjs/__test__.jsonc b/tests/specs/npm/dual_cjs_esm/ts_referrer_type_cjs/__test__.jsonc new file mode 100644 index 00000000000000..27136d4d0e9242 --- /dev/null +++ b/tests/specs/npm/dual_cjs_esm/ts_referrer_type_cjs/__test__.jsonc @@ -0,0 +1,14 @@ +{ + "tempDir": true, + "tests": { + "check": { + "args": "check --node-modules-dir=auto main.ts", + "output": "check.out", + "exitCode": 1 + }, + "run": { + "args": "run --node-modules-dir=auto main.ts", + "output": "main.out" + } + } +} diff --git a/tests/specs/npm/dual_cjs_esm/ts_referrer_type_cjs/check.out b/tests/specs/npm/dual_cjs_esm/ts_referrer_type_cjs/check.out new file mode 100644 index 00000000000000..cbd7740a9f6e90 --- /dev/null +++ b/tests/specs/npm/dual_cjs_esm/ts_referrer_type_cjs/check.out @@ -0,0 +1,8 @@ +Download http://localhost:4260/@denotest%2fdual-cjs-esm +Download http://localhost:4260/@denotest/dual-cjs-esm/1.0.0.tgz +Initialize @denotest/dual-cjs-esm@1.0.0 +Check file:///[WILDLINE]/main.ts +error: TS2322 [ERROR]: Type '"cjs"' is not assignable to type '"other"'. +const kind: "other" = mod.getKind(); + ~~~~ + at file:///[WILDLINE]/main.ts:3:7 diff --git a/tests/specs/npm/dual_cjs_esm/ts_referrer_type_cjs/main.out b/tests/specs/npm/dual_cjs_esm/ts_referrer_type_cjs/main.out new file mode 100644 index 00000000000000..62ddbf47931db6 --- /dev/null +++ b/tests/specs/npm/dual_cjs_esm/ts_referrer_type_cjs/main.out @@ -0,0 +1,4 @@ +Download http://localhost:4260/@denotest%2fdual-cjs-esm +Download http://localhost:4260/@denotest/dual-cjs-esm/1.0.0.tgz +Initialize @denotest/dual-cjs-esm@1.0.0 +cjs diff --git a/tests/specs/npm/dual_cjs_esm/ts_referrer_type_cjs/main.ts b/tests/specs/npm/dual_cjs_esm/ts_referrer_type_cjs/main.ts new file mode 100644 index 00000000000000..b8dd343f8ba4ac --- /dev/null +++ b/tests/specs/npm/dual_cjs_esm/ts_referrer_type_cjs/main.ts @@ -0,0 +1,4 @@ +import mod = require("@denotest/dual-cjs-esm"); + +const kind: "other" = mod.getKind(); +console.log(kind); diff --git a/tests/specs/npm/dual_cjs_esm/ts_referrer_type_cjs/package.json b/tests/specs/npm/dual_cjs_esm/ts_referrer_type_cjs/package.json new file mode 100644 index 00000000000000..419d3d9f11ee25 --- /dev/null +++ b/tests/specs/npm/dual_cjs_esm/ts_referrer_type_cjs/package.json @@ -0,0 +1,6 @@ +{ + "type": "commonjs", + "dependencies": { + "@denotest/dual-cjs-esm": "*" + } +} From 63f62552f5bde3be176856698f22b2f2927f3fd1 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Mon, 11 Nov 2024 17:59:19 -0500 Subject: [PATCH 17/22] switch eval to mts --- cli/args/mod.rs | 6 +++--- cli/lsp/repl.rs | 2 +- cli/module_loader.rs | 2 +- cli/tools/coverage/mod.rs | 2 +- cli/tools/jupyter/mod.rs | 2 +- cli/tools/lint/mod.rs | 2 +- cli/tools/repl/session.rs | 2 +- ext/node/polyfills/process.ts | 2 +- tests/specs/eval/pkg_json_type_cjs/__test__.jsonc | 4 ++++ tests/specs/eval/pkg_json_type_cjs/package.json | 3 +++ tests/specs/run/stdin_type_cjs/__test__.jsonc | 5 +++++ tests/specs/run/stdin_type_cjs/package.json | 3 +++ tests/specs/run/stdin_type_cjs/stdin_read_all.ts | 1 + 13 files changed, 26 insertions(+), 10 deletions(-) create mode 100644 tests/specs/eval/pkg_json_type_cjs/__test__.jsonc create mode 100644 tests/specs/eval/pkg_json_type_cjs/package.json create mode 100644 tests/specs/run/stdin_type_cjs/__test__.jsonc create mode 100644 tests/specs/run/stdin_type_cjs/package.json create mode 100644 tests/specs/run/stdin_type_cjs/stdin_read_all.ts diff --git a/cli/args/mod.rs b/cli/args/mod.rs index 0fdfe2c242dfc3..57f7d4f8866fc6 100644 --- a/cli/args/mod.rs +++ b/cli/args/mod.rs @@ -1141,14 +1141,14 @@ impl CliOptions { resolve_url_or_path(&compile_flags.source_file, self.initial_cwd())? } DenoSubcommand::Eval(_) => { - resolve_url_or_path("./$deno$eval.ts", self.initial_cwd())? + resolve_url_or_path("./$deno$eval.mts", self.initial_cwd())? } DenoSubcommand::Repl(_) => { - resolve_url_or_path("./$deno$repl.ts", self.initial_cwd())? + resolve_url_or_path("./$deno$repl.mts", self.initial_cwd())? } DenoSubcommand::Run(run_flags) => { if run_flags.is_stdin() { - resolve_url_or_path("./$deno$stdin.ts", self.initial_cwd())? + resolve_url_or_path("./$deno$stdin.mts", self.initial_cwd())? } else { resolve_url_or_path(&run_flags.script, self.initial_cwd())? } diff --git a/cli/lsp/repl.rs b/cli/lsp/repl.rs index fa5809045ed954..b4aaa8cd0d0594 100644 --- a/cli/lsp/repl.rs +++ b/cli/lsp/repl.rs @@ -263,7 +263,7 @@ impl ReplLanguageServer { } fn get_document_uri(&self) -> Uri { - uri_parse_unencoded(self.cwd_uri.join("$deno$repl.ts").unwrap().as_str()) + uri_parse_unencoded(self.cwd_uri.join("$deno$repl.mts").unwrap().as_str()) .unwrap() } } diff --git a/cli/module_loader.rs b/cli/module_loader.rs index f94a5973a69e17..9f47ff974a9f4e 100644 --- a/cli/module_loader.rs +++ b/cli/module_loader.rs @@ -445,7 +445,7 @@ impl let referrer = if referrer.is_empty() && self.shared.is_repl { // FIXME(bartlomieju): this is a hacky way to provide compatibility with REPL // and `Deno.core.evalContext` API. Ideally we should always have a referrer filled - "./$deno$repl.ts" + "./$deno$repl.mts" } else { referrer }; diff --git a/cli/tools/coverage/mod.rs b/cli/tools/coverage/mod.rs index f59333247510fc..2a554c13359ad2 100644 --- a/cli/tools/coverage/mod.rs +++ b/cli/tools/coverage/mod.rs @@ -480,7 +480,7 @@ fn filter_coverages( .filter(|e| { let is_internal = e.url.starts_with("ext:") || e.url.ends_with("__anonymous__") - || e.url.ends_with("$deno$test.js") + || e.url.ends_with("$deno$test.mjs") || e.url.ends_with(".snap") || is_supported_test_path(Path::new(e.url.as_str())) || doc_test_re.is_match(e.url.as_str()) diff --git a/cli/tools/jupyter/mod.rs b/cli/tools/jupyter/mod.rs index 0ffd0da1ee1187..732f95c49f25f7 100644 --- a/cli/tools/jupyter/mod.rs +++ b/cli/tools/jupyter/mod.rs @@ -61,7 +61,7 @@ pub async fn kernel( let factory = CliFactory::from_flags(flags); let cli_options = factory.cli_options()?; let main_module = - resolve_url_or_path("./$deno$jupyter.ts", cli_options.initial_cwd()) + resolve_url_or_path("./$deno$jupyter.mts", cli_options.initial_cwd()) .unwrap(); // TODO(bartlomieju): should we run with all permissions? let permissions = diff --git a/cli/tools/lint/mod.rs b/cli/tools/lint/mod.rs index e096b486ef2b2f..d8edf240481cf5 100644 --- a/cli/tools/lint/mod.rs +++ b/cli/tools/lint/mod.rs @@ -63,7 +63,7 @@ pub use rules::LintRuleProvider; const JSON_SCHEMA_VERSION: u8 = 1; -static STDIN_FILE_NAME: &str = "$deno$stdin.ts"; +static STDIN_FILE_NAME: &str = "$deno$stdin.mts"; pub async fn lint( flags: Arc, diff --git a/cli/tools/repl/session.rs b/cli/tools/repl/session.rs index e35af4aa3e8f26..8e05c4abbcff89 100644 --- a/cli/tools/repl/session.rs +++ b/cli/tools/repl/session.rs @@ -245,7 +245,7 @@ impl ReplSession { assert_ne!(context_id, 0); let referrer = - deno_core::resolve_path("./$deno$repl.ts", cli_options.initial_cwd()) + deno_core::resolve_path("./$deno$repl.mts", cli_options.initial_cwd()) .unwrap(); let cwd_url = diff --git a/ext/node/polyfills/process.ts b/ext/node/polyfills/process.ts index 2605fa6d1ad743..487b850787ef59 100644 --- a/ext/node/polyfills/process.ts +++ b/ext/node/polyfills/process.ts @@ -909,7 +909,7 @@ Object.defineProperty(argv, "1", { if (Deno.mainModule?.startsWith("file:")) { return pathFromURL(new URL(Deno.mainModule)); } else { - return join(Deno.cwd(), "$deno$node.js"); + return join(Deno.cwd(), "$deno$node.mjs"); } }, }); diff --git a/tests/specs/eval/pkg_json_type_cjs/__test__.jsonc b/tests/specs/eval/pkg_json_type_cjs/__test__.jsonc new file mode 100644 index 00000000000000..cd3804d773a32c --- /dev/null +++ b/tests/specs/eval/pkg_json_type_cjs/__test__.jsonc @@ -0,0 +1,4 @@ +{ + "args": "eval console.log(1)", + "output": "1\n" +} diff --git a/tests/specs/eval/pkg_json_type_cjs/package.json b/tests/specs/eval/pkg_json_type_cjs/package.json new file mode 100644 index 00000000000000..5bbefffbabee39 --- /dev/null +++ b/tests/specs/eval/pkg_json_type_cjs/package.json @@ -0,0 +1,3 @@ +{ + "type": "commonjs" +} diff --git a/tests/specs/run/stdin_type_cjs/__test__.jsonc b/tests/specs/run/stdin_type_cjs/__test__.jsonc new file mode 100644 index 00000000000000..e60af4a80335c5 --- /dev/null +++ b/tests/specs/run/stdin_type_cjs/__test__.jsonc @@ -0,0 +1,5 @@ +{ + "args": "run --quiet -", + "output": "1\n", + "input": "console.log(1)" +} diff --git a/tests/specs/run/stdin_type_cjs/package.json b/tests/specs/run/stdin_type_cjs/package.json new file mode 100644 index 00000000000000..5bbefffbabee39 --- /dev/null +++ b/tests/specs/run/stdin_type_cjs/package.json @@ -0,0 +1,3 @@ +{ + "type": "commonjs" +} diff --git a/tests/specs/run/stdin_type_cjs/stdin_read_all.ts b/tests/specs/run/stdin_type_cjs/stdin_read_all.ts new file mode 100644 index 00000000000000..2ecae40b716c3e --- /dev/null +++ b/tests/specs/run/stdin_type_cjs/stdin_read_all.ts @@ -0,0 +1 @@ +Deno.stdin.readable.pipeTo(Deno.stdout.writable); From 1ff35733459b1eeb98d38a12aef171fe924bdadc Mon Sep 17 00:00:00 2001 From: David Sherret Date: Tue, 12 Nov 2024 11:45:44 -0500 Subject: [PATCH 18/22] fix mac --- cli/lsp/resolver.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/cli/lsp/resolver.rs b/cli/lsp/resolver.rs index 10fe7c66c8124f..b0f40a06d434c2 100644 --- a/cli/lsp/resolver.rs +++ b/cli/lsp/resolver.rs @@ -64,6 +64,7 @@ use crate::resolver::CliResolverOptions; use crate::resolver::IsCjsResolver; use crate::resolver::WorkerCliNpmGraphResolver; use crate::tsc::into_specifier_and_media_type; +use crate::util::fs::canonicalize_path_maybe_not_exists; use crate::util::progress_bar::ProgressBar; use crate::util::progress_bar::ProgressBarStyle; @@ -728,9 +729,11 @@ impl LspIsCjsResolver { impl LspInNpmPackageChecker { pub fn new(cache: &LspCache) -> Self { + let npm_folder_path = cache.deno_dir().npm_folder_path(); Self { global_cache_dir: url_from_directory_path( - &cache.deno_dir().npm_folder_path(), + &canonicalize_path_maybe_not_exists(&npm_folder_path) + .unwrap_or(npm_folder_path), ) .unwrap_or_else(|_| { ModuleSpecifier::parse("file:///invalid/").unwrap() From fa3bd0e0494b6e1eda3cd57ec6b074e451384c33 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Tue, 12 Nov 2024 13:03:05 -0500 Subject: [PATCH 19/22] add final lsp test --- cli/resolver.rs | 6 +++ tests/integration/lsp_tests.rs | 49 +++++++++++++++++++ tests/specs/mod.rs | 20 ++++++++ .../dual_cjs_esm/cjs_referrer/__test__.jsonc | 2 +- .../ts_referrer_type_cjs/__test__.jsonc | 2 +- tests/specs/schema.json | 3 ++ 6 files changed, 80 insertions(+), 2 deletions(-) diff --git a/cli/resolver.rs b/cli/resolver.rs index e3f41238dc43d1..bbbb0662f65eb3 100644 --- a/cli/resolver.rs +++ b/cli/resolver.rs @@ -608,6 +608,12 @@ impl IsCjsResolver { known_cache.insert(specifier.clone(), NodeModuleKind::Esm); } Some(NodeModuleKind::Esm) + } else if is_script == Some(true) { + // we know this is cjs + if let Some(known_cache) = known_cache { + known_cache.insert(specifier.clone(), NodeModuleKind::Cjs); + } + Some(NodeModuleKind::Cjs) } else { None } diff --git a/tests/integration/lsp_tests.rs b/tests/integration/lsp_tests.rs index af5f9de23eb5e1..dcef6960848c95 100644 --- a/tests/integration/lsp_tests.rs +++ b/tests/integration/lsp_tests.rs @@ -16139,6 +16139,55 @@ fn lsp_cjs_import_dual() { ); } +#[test] +fn lsp_type_commonjs() { + let context = TestContextBuilder::new() + .use_http_server() + .use_temp_cwd() + .add_npm_env_vars() + .build(); + let temp_dir = context.temp_dir(); + temp_dir.write("deno.json", r#"{}"#); + temp_dir.write( + "package.json", + r#"{ + "type": "commonjs", + "dependencies": { + "@denotest/dual-cjs-esm": "1" + } +}"#, + ); + context.run_npm("install"); + + let mut client = context.new_lsp_command().build(); + client.initialize_default(); + let main_url = temp_dir.path().join("main.ts").url_file(); + let diagnostics = client.did_open( + json!({ + "textDocument": { + "uri": main_url, + "languageId": "typescript", + "version": 1, + // getKind() should resolve as "cjs" and cause a type checker error + "text": "import mod = require('@denotest/dual-cjs-esm');\nconst kind: 'other' = mod.getKind(); console.log(kind);", + } + }), + ); + assert_eq!( + json!(diagnostics.all()), + json!([{ + "range": { + "start": { "line": 1, "character": 6, }, + "end": { "line": 1, "character": 10, }, + }, + "severity": 1, + "code": 2322, + "source": "deno-ts", + "message": "Type '\"cjs\"' is not assignable to type '\"other\"'.", + }]) + ); +} + #[test] fn lsp_ts_code_fix_any_param() { let context = TestContextBuilder::new().use_temp_cwd().build(); diff --git a/tests/specs/mod.rs b/tests/specs/mod.rs index 34221dd9da63d1..b4c8781d312caf 100644 --- a/tests/specs/mod.rs +++ b/tests/specs/mod.rs @@ -119,6 +119,9 @@ struct MultiStepMetaData { /// steps. #[serde(default)] pub temp_dir: bool, + /// Whether the temporary directory should be symlinked to another path. + #[serde(default)] + pub symlinked_temp_dir: bool, /// The base environment to use for the test. #[serde(default)] pub base: Option, @@ -142,6 +145,8 @@ struct SingleTestMetaData { #[serde(default)] pub temp_dir: bool, #[serde(default)] + pub symlinked_temp_dir: bool, + #[serde(default)] pub repeat: Option, #[serde(flatten)] pub step: StepMetaData, @@ -155,6 +160,7 @@ impl SingleTestMetaData { base: self.base, cwd: None, temp_dir: self.temp_dir, + symlinked_temp_dir: self.symlinked_temp_dir, repeat: self.repeat, envs: Default::default(), steps: vec![self.step], @@ -330,6 +336,20 @@ fn test_context_from_metadata( builder = builder.cwd(cwd.to_string_lossy()); } + if metadata.symlinked_temp_dir { + // not actually deprecated, we just want to discourage its use + // because it's mostly used for testing purposes locally + #[allow(deprecated)] + { + builder = builder.use_symlinked_temp_dir(); + } + if cfg!(not(debug_assertions)) { + // panic to prevent using this on the CI as CI already uses + // a symlinked temp directory for every test + panic!("Cannot use symlinkedTempDir in release mode"); + } + } + match &metadata.base { // todo(dsherret): add bases in the future as needed Some(base) => panic!("Unknown test base: {}", base), diff --git a/tests/specs/npm/dual_cjs_esm/cjs_referrer/__test__.jsonc b/tests/specs/npm/dual_cjs_esm/cjs_referrer/__test__.jsonc index 62777cdaa13073..de2c1a0bc5934a 100644 --- a/tests/specs/npm/dual_cjs_esm/cjs_referrer/__test__.jsonc +++ b/tests/specs/npm/dual_cjs_esm/cjs_referrer/__test__.jsonc @@ -7,7 +7,7 @@ "exitCode": 1 }, "run": { - "args": "run --node-modules-dir=auto main.cts", + "args": "run --node-modules-dir=auto --allow-read main.cts", "output": "main.out" } } diff --git a/tests/specs/npm/dual_cjs_esm/ts_referrer_type_cjs/__test__.jsonc b/tests/specs/npm/dual_cjs_esm/ts_referrer_type_cjs/__test__.jsonc index 27136d4d0e9242..cf19217d18f200 100644 --- a/tests/specs/npm/dual_cjs_esm/ts_referrer_type_cjs/__test__.jsonc +++ b/tests/specs/npm/dual_cjs_esm/ts_referrer_type_cjs/__test__.jsonc @@ -7,7 +7,7 @@ "exitCode": 1 }, "run": { - "args": "run --node-modules-dir=auto main.ts", + "args": "run --node-modules-dir=auto --allow-read main.ts", "output": "main.out" } } diff --git a/tests/specs/schema.json b/tests/specs/schema.json index 8f3953ee44477e..2b35d9bd7dba4b 100644 --- a/tests/specs/schema.json +++ b/tests/specs/schema.json @@ -36,6 +36,9 @@ "flaky": { "type": "boolean" }, + "symlinkedTempDir": { + "type": "boolean" + }, "if": { "type": "string", "examples": [ From c84900ece4dbc5b549cf650b080345445a4a95db Mon Sep 17 00:00:00 2001 From: David Sherret Date: Tue, 12 Nov 2024 13:44:19 -0500 Subject: [PATCH 20/22] fix --- cli/resolver.rs | 56 ++++++++++++++++++++++++++----------------------- 1 file changed, 30 insertions(+), 26 deletions(-) diff --git a/cli/resolver.rs b/cli/resolver.rs index bbbb0662f65eb3..fe752a64f39b76 100644 --- a/cli/resolver.rs +++ b/cli/resolver.rs @@ -506,7 +506,7 @@ impl CjsTracker { specifier, media_type, is_script, - Some(&self.known), + &self.known, ) } } @@ -543,14 +543,30 @@ impl IsCjsResolver { if specifier.scheme() != "file" { return NodeModuleKind::Esm; } - self - .get_known_kind_with_is_script( - specifier, - MediaType::from_specifier(specifier), - is_script, - None, - ) - .unwrap_or(NodeModuleKind::Esm) + match MediaType::from_specifier(specifier) { + MediaType::Mts | MediaType::Mjs | MediaType::Dmts => NodeModuleKind::Esm, + MediaType::Cjs | MediaType::Cts | MediaType::Dcts => NodeModuleKind::Cjs, + MediaType::Dts => { + // dts files are always determined based on the package.json because + // they contain imports/exports even when considered CJS + self.check_based_on_pkg_json(specifier).unwrap_or(NodeModuleKind::Esm) + } + MediaType::Wasm | + MediaType::Json => NodeModuleKind::Esm, + MediaType::JavaScript + | MediaType::Jsx + | MediaType::TypeScript + | MediaType::Tsx + // treat these as unknown + | MediaType::Css + | MediaType::SourceMap + | MediaType::Unknown => { + match is_script { + Some(true) => self.check_based_on_pkg_json(specifier).unwrap_or(NodeModuleKind::Esm), + Some(false) | None => NodeModuleKind::Esm, + } + } + } } fn get_known_kind_with_is_script( @@ -558,7 +574,7 @@ impl IsCjsResolver { specifier: &ModuleSpecifier, media_type: MediaType, is_script: Option, - known_cache: Option<&DashMap>, + known_cache: &DashMap, ) -> Option { if specifier.scheme() != "file" { return Some(NodeModuleKind::Esm); @@ -570,14 +586,12 @@ impl IsCjsResolver { MediaType::Dts => { // dts files are always determined based on the package.json because // they contain imports/exports even when considered CJS - if let Some(value) = known_cache.and_then(|c| c.get(specifier)).map(|v| *v) { + if let Some(value) = known_cache.get(specifier).map(|v| *v) { Some(value) } else { let value = self.check_based_on_pkg_json(specifier).ok(); if let Some(value) = value { - if let Some(known_cache) = known_cache { - known_cache.insert(specifier.clone(), value); - } + known_cache.insert(specifier.clone(), value); } Some(value.unwrap_or(NodeModuleKind::Esm)) } @@ -592,28 +606,18 @@ impl IsCjsResolver { | MediaType::Css | MediaType::SourceMap | MediaType::Unknown => { - if let Some(value) = known_cache.and_then(|c| c.get(specifier)).map(|v| *v) { + if let Some(value) = known_cache.get(specifier).map(|v| *v) { if value == NodeModuleKind::Cjs && is_script == Some(false) { // we now know this is actually esm - if let Some(known_cache) = known_cache { - known_cache.insert(specifier.clone(), NodeModuleKind::Esm); - } + known_cache.insert(specifier.clone(), NodeModuleKind::Esm); Some(NodeModuleKind::Esm) } else { Some(value) } } else if is_script == Some(false) { // we know this is esm - if let Some(known_cache) = known_cache { known_cache.insert(specifier.clone(), NodeModuleKind::Esm); - } Some(NodeModuleKind::Esm) - } else if is_script == Some(true) { - // we know this is cjs - if let Some(known_cache) = known_cache { - known_cache.insert(specifier.clone(), NodeModuleKind::Cjs); - } - Some(NodeModuleKind::Cjs) } else { None } From fe03ea70838f1e6e024948b161776a4313a934ad Mon Sep 17 00:00:00 2001 From: David Sherret Date: Tue, 12 Nov 2024 18:51:39 -0500 Subject: [PATCH 21/22] Align require node module kind detection, handle resolving `node` binary main entrypoints, and various small improvements/tests --- cli/args/mod.rs | 62 +++++++++++++++++-- cli/factory.rs | 1 + cli/lsp/resolver.rs | 5 +- cli/module_loader.rs | 43 +++++-------- cli/resolver.rs | 13 ++-- cli/standalone/mod.rs | 10 +++ ext/node/lib.rs | 7 ++- ext/node/ops/require.rs | 40 +++--------- ext/node/polyfills/01_require.js | 4 +- resolvers/node/resolution.rs | 19 +++--- .../1.0.0/install.js | 12 ++++ .../1.0.0/package.json | 7 +++ .../install-no-ext/1.0.0/install/check.js | 1 + .../install-no-ext/1.0.0/install/index.js | 1 + .../install-no-ext/1.0.0/install/output.js | 1 + .../install-no-ext/1.0.0/package.json | 7 +++ .../__test__.jsonc | 5 ++ .../output.out | 4 ++ .../package.json | 5 ++ .../scripts_install_no_ext/__test__.jsonc | 5 ++ .../install/scripts_install_no_ext/output.out | 4 ++ .../scripts_install_no_ext/package.json | 5 ++ 22 files changed, 183 insertions(+), 78 deletions(-) create mode 100644 tests/registry/npm/@denotest/install-launch-cjs-temp-dir/1.0.0/install.js create mode 100644 tests/registry/npm/@denotest/install-launch-cjs-temp-dir/1.0.0/package.json create mode 100644 tests/registry/npm/@denotest/install-no-ext/1.0.0/install/check.js create mode 100644 tests/registry/npm/@denotest/install-no-ext/1.0.0/install/index.js create mode 100644 tests/registry/npm/@denotest/install-no-ext/1.0.0/install/output.js create mode 100644 tests/registry/npm/@denotest/install-no-ext/1.0.0/package.json create mode 100644 tests/specs/install/scripts_install_launch_cjs_temp_dir/__test__.jsonc create mode 100644 tests/specs/install/scripts_install_launch_cjs_temp_dir/output.out create mode 100644 tests/specs/install/scripts_install_launch_cjs_temp_dir/package.json create mode 100644 tests/specs/install/scripts_install_no_ext/__test__.jsonc create mode 100644 tests/specs/install/scripts_install_no_ext/output.out create mode 100644 tests/specs/install/scripts_install_no_ext/package.json diff --git a/cli/args/mod.rs b/cli/args/mod.rs index 57f7d4f8866fc6..25d4372030ed5c 100644 --- a/cli/args/mod.rs +++ b/cli/args/mod.rs @@ -7,6 +7,7 @@ mod import_map; mod lockfile; mod package_json; +use deno_ast::MediaType; use deno_ast::SourceMapOption; use deno_config::deno_json::NodeModulesDirMode; use deno_config::workspace::CreateResolverOptions; @@ -1136,7 +1137,7 @@ impl CliOptions { self .main_module_cell .get_or_init(|| { - let main_module = match &self.flags.subcommand { + Ok(match &self.flags.subcommand { DenoSubcommand::Compile(compile_flags) => { resolve_url_or_path(&compile_flags.source_file, self.initial_cwd())? } @@ -1150,7 +1151,20 @@ impl CliOptions { if run_flags.is_stdin() { resolve_url_or_path("./$deno$stdin.mts", self.initial_cwd())? } else { - resolve_url_or_path(&run_flags.script, self.initial_cwd())? + let url = + resolve_url_or_path(&run_flags.script, self.initial_cwd())?; + if self.is_node_main() + && url.scheme() == "file" + && MediaType::from_specifier(&url) == MediaType::Unknown + { + try_resolve_node_binary_main_entrypoint( + &run_flags.script, + self.initial_cwd(), + )? + .unwrap_or(url) + } else { + url + } } } DenoSubcommand::Serve(run_flags) => { @@ -1159,9 +1173,7 @@ impl CliOptions { _ => { bail!("No main module.") } - }; - - Ok(main_module) + }) }) .as_ref() .map_err(|err| deno_core::anyhow::anyhow!("{}", err)) @@ -1206,6 +1218,14 @@ impl CliOptions { } } + // If the main module should be treated as being in an npm package. + // This is triggered via a secret environment variable which is used + // for functionality like child_process.fork. Users should NOT depend + // on this functionality. + pub fn is_node_main(&self) -> bool { + NPM_PROCESS_STATE.is_some() + } + pub fn has_node_modules_dir(&self) -> bool { self.maybe_node_modules_folder.is_some() } @@ -1581,7 +1601,7 @@ impl CliOptions { } pub fn detect_cjs(&self) -> bool { - self.workspace().package_jsons().next().is_some() + self.workspace().package_jsons().next().is_some() || self.is_node_main() } fn byonm_enabled(&self) -> bool { @@ -1782,6 +1802,36 @@ fn resolve_node_modules_folder( Ok(Some(canonicalize_path_maybe_not_exists(&path)?)) } +fn try_resolve_node_binary_main_entrypoint( + specifier: &str, + initial_cwd: &Path, +) -> Result, AnyError> { + // node allows running files at paths without a `.js` extension + // or at directories with an index.js file + let path = deno_core::normalize_path(initial_cwd.join(specifier)); + if path.is_dir() { + let index_file = path.join("index.js"); + Ok(if index_file.is_file() { + Some(deno_path_util::url_from_file_path(&index_file)?) + } else { + None + }) + } else { + let path = path.with_extension( + path + .extension() + .and_then(|s| s.to_str()) + .map(|s| format!("{}.js", s)) + .unwrap_or("js".to_string()), + ); + if path.is_file() { + Ok(Some(deno_path_util::url_from_file_path(&path)?)) + } else { + Ok(None) + } + } +} + fn resolve_import_map_specifier( maybe_import_map_path: Option<&str>, maybe_config_file: Option<&ConfigFile>, diff --git a/cli/factory.rs b/cli/factory.rs index 1446997f26b4e5..e6316701b7fb4e 100644 --- a/cli/factory.rs +++ b/cli/factory.rs @@ -793,6 +793,7 @@ impl CliFactory { self.pkg_json_resolver().clone(), IsCjsResolverOptions { detect_cjs: options.detect_cjs(), + is_node_main: options.is_node_main(), }, ))) }) diff --git a/cli/lsp/resolver.rs b/cli/lsp/resolver.rs index b0f40a06d434c2..37f63b912cbef7 100644 --- a/cli/lsp/resolver.rs +++ b/cli/lsp/resolver.rs @@ -766,7 +766,10 @@ impl LspIsCjsResolver { inner: IsCjsResolver::new( Arc::new(LspInNpmPackageChecker::new(cache)), pkg_json_resolver, - crate::resolver::IsCjsResolverOptions { detect_cjs: true }, + crate::resolver::IsCjsResolverOptions { + detect_cjs: true, + is_node_main: false, + }, ), } } diff --git a/cli/module_loader.rs b/cli/module_loader.rs index 9f47ff974a9f4e..f9c974d77ee28e 100644 --- a/cli/module_loader.rs +++ b/cli/module_loader.rs @@ -72,6 +72,7 @@ use deno_runtime::deno_node::create_host_defined_options; use deno_runtime::deno_node::NodeRequireLoader; use deno_runtime::deno_permissions::PermissionsContainer; use deno_semver::npm::NpmPackageReqReference; +use node_resolver::errors::ClosestPkgJsonError; use node_resolver::InNpmPackageChecker; use node_resolver::NodeResolutionMode; @@ -291,13 +292,14 @@ impl CliModuleLoaderFactory { parsed_source_cache: self.shared.parsed_source_cache.clone(), shared: self.shared.clone(), }))); - let node_require_loader = Rc::new(CliNodeRequireLoader::new( - self.shared.emitter.clone(), - self.shared.fs.clone(), + let node_require_loader = Rc::new(CliNodeRequireLoader { + cjs_tracker: self.shared.cjs_tracker.clone(), + emitter: self.shared.emitter.clone(), + fs: self.shared.fs.clone(), graph_container, - self.shared.in_npm_pkg_checker.clone(), - self.shared.npm_resolver.clone(), - )); + in_npm_pkg_checker: self.shared.in_npm_pkg_checker.clone(), + npm_resolver: self.shared.npm_resolver.clone(), + }); CreateModuleLoaderResult { module_loader, node_require_loader, @@ -1031,6 +1033,7 @@ impl ModuleGraphUpdatePermit for WorkerModuleGraphUpdatePermit { #[derive(Debug)] struct CliNodeRequireLoader { + cjs_tracker: Arc, emitter: Arc, fs: Arc, graph_container: TGraphContainer, @@ -1038,26 +1041,6 @@ struct CliNodeRequireLoader { npm_resolver: Arc, } -impl - CliNodeRequireLoader -{ - pub fn new( - emitter: Arc, - fs: Arc, - graph_container: TGraphContainer, - in_npm_pkg_checker: Arc, - npm_resolver: Arc, - ) -> Self { - Self { - emitter, - fs, - graph_container, - in_npm_pkg_checker, - npm_resolver, - } - } -} - impl NodeRequireLoader for CliNodeRequireLoader { @@ -1103,4 +1086,12 @@ impl NodeRequireLoader Ok(text) } } + + fn is_maybe_cjs( + &self, + specifier: &ModuleSpecifier, + ) -> Result { + let media_type = MediaType::from_specifier(specifier); + self.cjs_tracker.is_maybe_cjs(specifier, media_type) + } } diff --git a/cli/resolver.rs b/cli/resolver.rs index fe752a64f39b76..786e5d0dbc810a 100644 --- a/cli/resolver.rs +++ b/cli/resolver.rs @@ -511,15 +511,17 @@ impl CjsTracker { } } +#[derive(Debug)] pub struct IsCjsResolverOptions { pub detect_cjs: bool, + pub is_node_main: bool, } #[derive(Debug)] pub struct IsCjsResolver { in_npm_pkg_checker: Arc, pkg_json_resolver: Arc, - detect_cjs: bool, + options: IsCjsResolverOptions, } impl IsCjsResolver { @@ -531,7 +533,7 @@ impl IsCjsResolver { Self { in_npm_pkg_checker, pkg_json_resolver, - detect_cjs: options.detect_cjs, + options, } } @@ -642,16 +644,19 @@ impl IsCjsResolver { } else { Ok(NodeModuleKind::Cjs) } - } else if self.detect_cjs { + } else if self.options.detect_cjs || self.options.is_node_main { if let Some(pkg_json) = self.pkg_json_resolver.get_closest_package_json(specifier)? { - let is_cjs_type = pkg_json.typ == "commonjs"; + let is_cjs_type = pkg_json.typ == "commonjs" + || self.options.is_node_main && pkg_json.typ == "none"; Ok(if is_cjs_type { NodeModuleKind::Cjs } else { NodeModuleKind::Esm }) + } else if self.options.is_node_main { + Ok(NodeModuleKind::Cjs) } else { Ok(NodeModuleKind::Esm) } diff --git a/cli/standalone/mod.rs b/cli/standalone/mod.rs index 562b8984b03d31..79eff6829fc556 100644 --- a/cli/standalone/mod.rs +++ b/cli/standalone/mod.rs @@ -45,6 +45,7 @@ use deno_runtime::WorkerLogLevel; use deno_semver::npm::NpmPackageReqReference; use import_map::parse_from_json; use node_resolver::analyze::NodeCodeTranslator; +use node_resolver::errors::ClosestPkgJsonError; use node_resolver::NodeModuleKind; use node_resolver::NodeResolutionMode; use serialization::DenoCompileModuleSource; @@ -449,6 +450,14 @@ impl NodeRequireLoader for EmbeddedModuleLoader { ) -> Result { Ok(self.shared.fs.read_text_file_lossy_sync(path, None)?) } + + fn is_maybe_cjs( + &self, + specifier: &ModuleSpecifier, + ) -> Result { + let media_type = MediaType::from_specifier(specifier); + self.shared.cjs_tracker.is_maybe_cjs(specifier, media_type) + } } struct StandaloneModuleLoaderFactory { @@ -650,6 +659,7 @@ pub async fn run(data: StandaloneData) -> Result { pkg_json_resolver.clone(), IsCjsResolverOptions { detect_cjs: !metadata.workspace_resolver.package_jsons.is_empty(), + is_node_main: false, }, )); let cache_db = Caches::new(deno_dir_provider.clone()); diff --git a/ext/node/lib.rs b/ext/node/lib.rs index 92c2c1497cc521..7ddae9e0803f06 100644 --- a/ext/node/lib.rs +++ b/ext/node/lib.rs @@ -14,6 +14,7 @@ use deno_core::url::Url; #[allow(unused_imports)] use deno_core::v8; use deno_core::v8::ExternalReference; +use node_resolver::errors::ClosestPkgJsonError; use node_resolver::NpmResolverRc; use once_cell::sync::Lazy; @@ -157,6 +158,10 @@ pub trait NodeRequireLoader { ) -> Result, AnyError>; fn load_text_file_lossy(&self, path: &Path) -> Result; + + /// Get if the module kind is maybe CJS and loading should determine + /// if its CJS or ESM. + fn is_maybe_cjs(&self, specifier: &Url) -> Result; } pub static NODE_ENV_VAR_ALLOWLIST: Lazy> = Lazy::new(|| { @@ -384,6 +389,7 @@ deno_core::extension!(deno_node, ops::require::op_require_proxy_path, ops::require::op_require_is_deno_dir_package, ops::require::op_require_resolve_deno_dir, + ops::require::op_require_is_maybe_cjs, ops::require::op_require_is_request_relative, ops::require::op_require_resolve_lookup_paths, ops::require::op_require_try_self_parent_path

, @@ -396,7 +402,6 @@ deno_core::extension!(deno_node, ops::require::op_require_path_basename, ops::require::op_require_read_file

, ops::require::op_require_as_file_path, - ops::require::op_require_module_format

, ops::require::op_require_resolve_exports

, ops::require::op_require_read_package_scope

, ops::require::op_require_package_imports_resolve

, diff --git a/ext/node/ops/require.rs b/ext/node/ops/require.rs index 97d1f993fd9d50..b7fa8feb20145c 100644 --- a/ext/node/ops/require.rs +++ b/ext/node/ops/require.rs @@ -7,11 +7,12 @@ use deno_core::v8; use deno_core::JsRuntimeInspector; use deno_core::OpState; use deno_fs::FileSystemRc; +use deno_package_json::NodeModuleKind; use deno_package_json::PackageJsonRc; use deno_path_util::normalize_path; use deno_path_util::url_from_file_path; use deno_path_util::url_to_file_path; -use node_resolver::NodeModuleKind; +use node_resolver::errors::ClosestPkgJsonError; use node_resolver::NodeResolutionMode; use node_resolver::REQUIRE_CONDITIONS; use std::borrow::Cow; @@ -565,40 +566,17 @@ where })) } -#[op2] -#[string] -pub fn op_require_module_format

( +#[op2(fast)] +pub fn op_require_is_maybe_cjs( state: &mut OpState, #[string] filename: String, -) -> Result, node_resolver::errors::ClosestPkgJsonError> -where - P: NodePermissions + 'static, -{ +) -> Result { let filename = PathBuf::from(filename); - let pkg_json_resolver = state.borrow::(); - let pkg_json = - pkg_json_resolver.get_closest_package_json_from_path(&filename)?; - let Some(pkg_json) = pkg_json else { - return Ok(None); + let Ok(url) = url_from_file_path(&filename) else { + return Ok(false); }; - match pkg_json.typ.as_str() { - "commonjs" => Ok(Some("commonjs")), - "module" => Ok(Some("module")), - "none" => { - let resolver = state.borrow::(); - match deno_path_util::url_from_file_path(&filename) { - Ok(specifier) => { - if resolver.in_npm_package(&specifier) { - Ok(None) - } else { - Ok(Some("module")) - } - } - Err(_) => Ok(Some("module")), - } - } - _ => Ok(None), - } + let loader = state.borrow::(); + loader.is_maybe_cjs(&url) } #[op2] diff --git a/ext/node/polyfills/01_require.js b/ext/node/polyfills/01_require.js index bacd88090d0a25..682de7e453e070 100644 --- a/ext/node/polyfills/01_require.js +++ b/ext/node/polyfills/01_require.js @@ -11,8 +11,8 @@ import { op_require_can_parse_as_esm, op_require_init_paths, op_require_is_deno_dir_package, + op_require_is_maybe_cjs, op_require_is_request_relative, - op_require_module_format, op_require_node_module_paths, op_require_package_imports_resolve, op_require_path_basename, @@ -1066,7 +1066,7 @@ Module._extensions[".js"] = Module._extensions[".tsx"] = function (module, filename) { const content = op_require_read_file(filename); - const format = op_require_module_format(filename); + const format = op_require_is_maybe_cjs(filename) ? undefined : "module"; module._compile(content, filename, format); }; diff --git a/resolvers/node/resolution.rs b/resolvers/node/resolution.rs index c1d18646227173..fcff2924250634 100644 --- a/resolvers/node/resolution.rs +++ b/resolvers/node/resolution.rs @@ -50,6 +50,15 @@ pub static DEFAULT_CONDITIONS: &[&str] = &["deno", "node", "import"]; pub static REQUIRE_CONDITIONS: &[&str] = &["require", "node"]; static TYPES_ONLY_CONDITIONS: &[&str] = &["types"]; +fn conditions_from_module_kind( + kind: NodeModuleKind, +) -> &'static [&'static str] { + match kind { + NodeModuleKind::Esm => DEFAULT_CONDITIONS, + NodeModuleKind::Cjs => REQUIRE_CONDITIONS, + } +} + pub type NodeModuleKind = deno_package_json::NodeModuleKind; #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -166,8 +175,7 @@ impl NodeResolver { specifier, referrer, referrer_kind, - // even though the referrer may be CJS, if we're here that means we're doing ESM resolution - DEFAULT_CONDITIONS, + conditions_from_module_kind(referrer_kind), mode, )?; @@ -310,7 +318,7 @@ impl NodeResolver { &package_subpath, maybe_referrer, referrer_kind, - DEFAULT_CONDITIONS, + conditions_from_module_kind(referrer_kind), mode, )?; // TODO(bartlomieju): skipped checking errors for commonJS resolution and @@ -441,10 +449,7 @@ impl NodeResolver { /* sub path */ ".", maybe_referrer, referrer_kind, - match referrer_kind { - NodeModuleKind::Esm => DEFAULT_CONDITIONS, - NodeModuleKind::Cjs => REQUIRE_CONDITIONS, - }, + conditions_from_module_kind(referrer_kind), NodeResolutionMode::Types, ); if let Ok(resolution) = resolution_result { diff --git a/tests/registry/npm/@denotest/install-launch-cjs-temp-dir/1.0.0/install.js b/tests/registry/npm/@denotest/install-launch-cjs-temp-dir/1.0.0/install.js new file mode 100644 index 00000000000000..61aadeb8391390 --- /dev/null +++ b/tests/registry/npm/@denotest/install-launch-cjs-temp-dir/1.0.0/install.js @@ -0,0 +1,12 @@ +const tempDir = Deno.makeTempDirSync(); +try { + // should work requiring these because this was launched via a node binary entrypoint + Deno.writeTextFileSync(`${tempDir}/index.js`, "module.exports = require('./other');"); + Deno.writeTextFileSync(`${tempDir}/other.js`, "module.exports = (a, b) => a + b;"); + const add = require(`${tempDir}/index.js`); + if (add(1, 2) !== 3) { + throw new Error("FAILED"); + } +} finally { + Deno.removeSync(tempDir, { recursive: true }); +} diff --git a/tests/registry/npm/@denotest/install-launch-cjs-temp-dir/1.0.0/package.json b/tests/registry/npm/@denotest/install-launch-cjs-temp-dir/1.0.0/package.json new file mode 100644 index 00000000000000..c3cf8dc4c334e6 --- /dev/null +++ b/tests/registry/npm/@denotest/install-launch-cjs-temp-dir/1.0.0/package.json @@ -0,0 +1,7 @@ +{ + "name": "@denotest/install-launch-cjs-temp-dir", + "version": "1.0.0", + "scripts": { + "install": "node install.js" + } +} \ No newline at end of file diff --git a/tests/registry/npm/@denotest/install-no-ext/1.0.0/install/check.js b/tests/registry/npm/@denotest/install-no-ext/1.0.0/install/check.js new file mode 100644 index 00000000000000..7d55c2481d1142 --- /dev/null +++ b/tests/registry/npm/@denotest/install-no-ext/1.0.0/install/check.js @@ -0,0 +1 @@ +require("./output"); diff --git a/tests/registry/npm/@denotest/install-no-ext/1.0.0/install/index.js b/tests/registry/npm/@denotest/install-no-ext/1.0.0/install/index.js new file mode 100644 index 00000000000000..7d55c2481d1142 --- /dev/null +++ b/tests/registry/npm/@denotest/install-no-ext/1.0.0/install/index.js @@ -0,0 +1 @@ +require("./output"); diff --git a/tests/registry/npm/@denotest/install-no-ext/1.0.0/install/output.js b/tests/registry/npm/@denotest/install-no-ext/1.0.0/install/output.js new file mode 100644 index 00000000000000..69668cd6251b0d --- /dev/null +++ b/tests/registry/npm/@denotest/install-no-ext/1.0.0/install/output.js @@ -0,0 +1 @@ +console.log("SUCCESS"); diff --git a/tests/registry/npm/@denotest/install-no-ext/1.0.0/package.json b/tests/registry/npm/@denotest/install-no-ext/1.0.0/package.json new file mode 100644 index 00000000000000..b9abed1f6e1efd --- /dev/null +++ b/tests/registry/npm/@denotest/install-no-ext/1.0.0/package.json @@ -0,0 +1,7 @@ +{ + "name": "@denotest/install-no-ext", + "version": "1.0.0", + "scripts": { + "install": "node install/check && node install" + } +} \ No newline at end of file diff --git a/tests/specs/install/scripts_install_launch_cjs_temp_dir/__test__.jsonc b/tests/specs/install/scripts_install_launch_cjs_temp_dir/__test__.jsonc new file mode 100644 index 00000000000000..087d08eff06892 --- /dev/null +++ b/tests/specs/install/scripts_install_launch_cjs_temp_dir/__test__.jsonc @@ -0,0 +1,5 @@ +{ + "tempDir": true, + "args": "install --allow-scripts", + "output": "output.out" +} diff --git a/tests/specs/install/scripts_install_launch_cjs_temp_dir/output.out b/tests/specs/install/scripts_install_launch_cjs_temp_dir/output.out new file mode 100644 index 00000000000000..d5f06cc6eac75a --- /dev/null +++ b/tests/specs/install/scripts_install_launch_cjs_temp_dir/output.out @@ -0,0 +1,4 @@ +Download http://localhost:4260/@denotest%2finstall-launch-cjs-temp-dir +Download http://localhost:4260/@denotest/install-launch-cjs-temp-dir/1.0.0.tgz +Initialize @denotest/install-launch-cjs-temp-dir@1.0.0 +Initialize @denotest/install-launch-cjs-temp-dir@1.0.0: running 'install' script diff --git a/tests/specs/install/scripts_install_launch_cjs_temp_dir/package.json b/tests/specs/install/scripts_install_launch_cjs_temp_dir/package.json new file mode 100644 index 00000000000000..71672f9bc1b353 --- /dev/null +++ b/tests/specs/install/scripts_install_launch_cjs_temp_dir/package.json @@ -0,0 +1,5 @@ +{ + "dependencies": { + "@denotest/install-launch-cjs-temp-dir": "*" + } +} diff --git a/tests/specs/install/scripts_install_no_ext/__test__.jsonc b/tests/specs/install/scripts_install_no_ext/__test__.jsonc new file mode 100644 index 00000000000000..087d08eff06892 --- /dev/null +++ b/tests/specs/install/scripts_install_no_ext/__test__.jsonc @@ -0,0 +1,5 @@ +{ + "tempDir": true, + "args": "install --allow-scripts", + "output": "output.out" +} diff --git a/tests/specs/install/scripts_install_no_ext/output.out b/tests/specs/install/scripts_install_no_ext/output.out new file mode 100644 index 00000000000000..074e9781216645 --- /dev/null +++ b/tests/specs/install/scripts_install_no_ext/output.out @@ -0,0 +1,4 @@ +Download http://localhost:4260/@denotest%2finstall-no-ext +Download http://localhost:4260/@denotest/install-no-ext/1.0.0.tgz +Initialize @denotest/install-no-ext@1.0.0 +Initialize @denotest/install-no-ext@1.0.0: running 'install' script diff --git a/tests/specs/install/scripts_install_no_ext/package.json b/tests/specs/install/scripts_install_no_ext/package.json new file mode 100644 index 00000000000000..7ac9ca6b29fb6d --- /dev/null +++ b/tests/specs/install/scripts_install_no_ext/package.json @@ -0,0 +1,5 @@ +{ + "dependencies": { + "@denotest/install-no-ext": "*" + } +} From c4256e9ece37a85de2f5f279e7bab0dea4af696f Mon Sep 17 00:00:00 2001 From: David Sherret Date: Wed, 13 Nov 2024 09:31:00 -0500 Subject: [PATCH 22/22] add comment --- cli/args/mod.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/cli/args/mod.rs b/cli/args/mod.rs index 25d4372030ed5c..93bdc30ae0c4df 100644 --- a/cli/args/mod.rs +++ b/cli/args/mod.rs @@ -1601,6 +1601,9 @@ impl CliOptions { } pub fn detect_cjs(&self) -> bool { + // only enabled when there's a package.json in order to not have a + // perf penalty for non-npm Deno projects of searching for the closest + // package.json beside each module self.workspace().package_jsons().next().is_some() || self.is_node_main() }