fix(check): CJS types importing dual ESM/CJS package should prefer CJS types (#24492)

Closes #16370

9 files changed, 138 insertions, 68 deletions

diff --git a/cli/factory.rs b/cli/factory.rs
index e3147e49f..df7b2c868 100644
--- a/cli/factory.rs
+++ b/cli/factory.rs
@@ -409,13 +409,13 @@ impl CliFactory {
         create_cli_npm_resolver(if self.options.use_byonm() && !matches!(self.options.sub_command(), DenoSubcommand::Install(_)) {
           CliNpmResolverCreateOptions::Byonm(CliNpmResolverByonmCreateOptions {
             fs: fs.clone(),
-            root_node_modules_dir: match self.options.node_modules_dir_path() {
+            root_node_modules_dir: Some(match self.options.node_modules_dir_path() {
               Some(node_modules_path) => node_modules_path.to_path_buf(),
               // path needs to be canonicalized for node resolution
               // (node_modules_dir_path above is already canonicalized)
               None => canonicalize_path_maybe_not_exists(self.options.initial_cwd())?
                 .join("node_modules"),
-            },
+            }),
           })
         } else {
           CliNpmResolverCreateOptions::Managed(CliNpmResolverManagedCreateOptions {
@@ -732,7 +732,7 @@ impl CliFactory {
       .cli_node_resolver
       .get_or_try_init_async(async {
         Ok(Arc::new(CliNodeResolver::new(
-          Some(self.cjs_resolutions().clone()),
+          self.cjs_resolutions().clone(),
           self.fs().clone(),
           self.node_resolver().await?.clone(),
           self.npm_resolver().await?.clone(),
diff --git a/cli/lsp/resolver.rs b/cli/lsp/resolver.rs
index f160622ab..21b46255c 100644
--- a/cli/lsp/resolver.rs
+++ b/cli/lsp/resolver.rs
@@ -4,6 +4,7 @@ use crate::args::create_default_npmrc;
 use crate::args::CacheSetting;
 use crate::args::CliLockfile;
 use crate::args::PackageJsonInstallDepsProvider;
+use crate::args::DENO_FUTURE;
 use crate::graph_util::CliJsrUrlProvider;
 use crate::http_util::HttpClientProvider;
 use crate::lsp::config::Config;
@@ -16,6 +17,7 @@ use crate::npm::CliNpmResolverCreateOptions;
 use crate::npm::CliNpmResolverManagedCreateOptions;
 use crate::npm::CliNpmResolverManagedSnapshotOption;
 use crate::npm::ManagedCliNpmResolver;
+use crate::resolver::CjsResolutionStore;
 use crate::resolver::CliGraphResolver;
 use crate::resolver::CliGraphResolverOptions;
 use crate::resolver::CliNodeResolver;
@@ -38,6 +40,7 @@ use deno_runtime::deno_node::errors::ClosestPkgJsonError;
 use deno_runtime::deno_node::NodeResolution;
 use deno_runtime::deno_node::NodeResolutionMode;
 use deno_runtime::deno_node::NodeResolver;
+use deno_runtime::deno_node::NpmResolver;
 use deno_runtime::deno_node::PackageJson;
 use deno_runtime::fs_util::specifier_to_file_path;
 use deno_semver::jsr::JsrPackageReqReference;
@@ -343,11 +346,29 @@ impl LspResolver {
   }
 
   pub fn in_node_modules(&self, specifier: &ModuleSpecifier) -> bool {
-    let resolver = self.get_scope_resolver(Some(specifier));
-    if let Some(npm_resolver) = &resolver.npm_resolver {
-      return npm_resolver.in_npm_package(specifier);
+    fn has_node_modules_dir(specifier: &ModuleSpecifier) -> bool {
+      // consider any /node_modules/ directory as being in the node_modules
+      // folder for the LSP because it's pretty complicated to deal with multiple scopes
+      specifier.scheme() == "file"
+        && specifier
+          .path()
+          .to_ascii_lowercase()
+          .contains("/node_modules/")
     }
-    false
+
+    let global_npm_resolver = self
+      .get_scope_resolver(Some(specifier))
+      .npm_resolver
+      .as_ref()
+      .and_then(|npm_resolver| npm_resolver.as_managed())
+      .filter(|r| r.root_node_modules_path().is_none());
+    if let Some(npm_resolver) = &global_npm_resolver {
+      if npm_resolver.in_npm_package(specifier) {
+        return true;
+      }
+    }
+
+    has_node_modules_dir(specifier)
   }
 
   pub fn node_media_type(
@@ -422,20 +443,17 @@ async fn create_npm_resolver(
   cache: &LspCache,
   http_client_provider: &Arc<HttpClientProvider>,
 ) -> Option<Arc<dyn CliNpmResolver>> {
-  let mut byonm_dir = None;
-  if let Some(config_data) = config_data {
-    if config_data.byonm {
-      byonm_dir = Some(config_data.node_modules_dir.clone().or_else(|| {
-        specifier_to_file_path(&config_data.scope)
-          .ok()
-          .map(|p| p.join("node_modules/"))
-      })?)
-    }
-  }
-  let options = if let Some(byonm_dir) = byonm_dir {
+  let enable_byonm = config_data.map(|d| d.byonm).unwrap_or(*DENO_FUTURE);
+  let options = if enable_byonm {
     CliNpmResolverCreateOptions::Byonm(CliNpmResolverByonmCreateOptions {
       fs: Arc::new(deno_fs::RealFs),
-      root_node_modules_dir: byonm_dir,
+      root_node_modules_dir: config_data.and_then(|config_data| {
+        config_data.node_modules_dir.clone().or_else(|| {
+          specifier_to_file_path(&config_data.scope)
+            .ok()
+            .map(|p| p.join("node_modules/"))
+        })
+      }),
     })
   } else {
     CliNpmResolverCreateOptions::Managed(CliNpmResolverManagedCreateOptions {
@@ -478,6 +496,13 @@ async fn create_npm_resolver(
 fn create_node_resolver(
   npm_resolver: Option<&Arc<dyn CliNpmResolver>>,
 ) -> Option<Arc<CliNodeResolver>> {
+  use once_cell::sync::Lazy;
+
+  // it's not ideal to share this across all scopes and to
+  // never clear it, but it's fine for the time being
+  static CJS_RESOLUTIONS: Lazy<Arc<CjsResolutionStore>> =
+    Lazy::new(Default::default);
+
   let npm_resolver = npm_resolver?;
   let fs = Arc::new(deno_fs::RealFs);
   let node_resolver_inner = Arc::new(NodeResolver::new(
@@ -485,7 +510,7 @@ fn create_node_resolver(
     npm_resolver.clone().into_npm_resolver(),
   ));
   Some(Arc::new(CliNodeResolver::new(
-    None,
+    CJS_RESOLUTIONS.clone(),
     fs,
     node_resolver_inner,
     npm_resolver.clone(),
diff --git a/cli/lsp/tsc.rs b/cli/lsp/tsc.rs
index cc88a0811..6759f75f4 100644
--- a/cli/lsp/tsc.rs
+++ b/cli/lsp/tsc.rs
@@ -4347,9 +4347,17 @@ fn op_release(
 fn op_resolve(
   state: &mut OpState,
   #[string] base: String,
+  is_base_cjs: bool,
   #[serde] specifiers: Vec<String>,
 ) -> Result<Vec<Option<(String, String)>>, AnyError> {
-  op_resolve_inner(state, ResolveArgs { base, specifiers })
+  op_resolve_inner(
+    state,
+    ResolveArgs {
+      base,
+      is_base_cjs,
+      specifiers,
+    },
+  )
 }
 
 struct TscRequestArray {
@@ -6287,6 +6295,7 @@ mod tests {
       &mut state,
       ResolveArgs {
         base: "file:///a.ts".to_string(),
+        is_base_cjs: false,
         specifiers: vec!["./b.ts".to_string()],
       },
     )
diff --git a/cli/npm/byonm.rs b/cli/npm/byonm.rs
index 4abb6b3b2..6f45648a8 100644
--- a/cli/npm/byonm.rs
+++ b/cli/npm/byonm.rs
@@ -29,7 +29,8 @@ use super::InnerCliNpmResolverRef;
 
 pub struct CliNpmResolverByonmCreateOptions {
   pub fs: Arc<dyn FileSystem>,
-  pub root_node_modules_dir: PathBuf,
+  // todo(dsherret): investigate removing this
+  pub root_node_modules_dir: Option<PathBuf>,
 }
 
 pub fn create_byonm_npm_resolver(
@@ -44,7 +45,7 @@ pub fn create_byonm_npm_resolver(
 #[derive(Debug)]
 pub struct ByonmCliNpmResolver {
   fs: Arc<dyn FileSystem>,
-  root_node_modules_dir: PathBuf,
+  root_node_modules_dir: Option<PathBuf>,
 }
 
 impl ByonmCliNpmResolver {
@@ -131,16 +132,16 @@ impl ByonmCliNpmResolver {
     }
 
     // otherwise, fall fallback to the project's package.json
-    let root_pkg_json_path = self
-      .root_node_modules_dir
-      .parent()
-      .unwrap()
-      .join("package.json");
-    if let Some(pkg_json) =
-      load_pkg_json(self.fs.as_ref(), &root_pkg_json_path)?
-    {
-      if let Some(alias) = resolve_alias_from_pkg_json(req, pkg_json.as_ref()) {
-        return Ok((pkg_json, alias));
+    if let Some(root_node_modules_dir) = &self.root_node_modules_dir {
+      let root_pkg_json_path =
+        root_node_modules_dir.parent().unwrap().join("package.json");
+      if let Some(pkg_json) =
+        load_pkg_json(self.fs.as_ref(), &root_pkg_json_path)?
+      {
+        if let Some(alias) = resolve_alias_from_pkg_json(req, pkg_json.as_ref())
+        {
+          return Ok((pkg_json, alias));
+        }
       }
     }
 
@@ -159,9 +160,10 @@ impl NpmResolver for ByonmCliNpmResolver {
   fn get_npm_process_state(&self) -> String {
     serde_json::to_string(&NpmProcessState {
       kind: NpmProcessStateKind::Byonm,
-      local_node_modules_path: Some(
-        self.root_node_modules_dir.to_string_lossy().to_string(),
-      ),
+      local_node_modules_path: self
+        .root_node_modules_dir
+        .as_ref()
+        .map(|p| p.to_string_lossy().to_string()),
     })
     .unwrap()
   }
@@ -256,7 +258,7 @@ impl CliNpmResolver for ByonmCliNpmResolver {
   }
 
   fn root_node_modules_path(&self) -> Option<&PathBuf> {
-    Some(&self.root_node_modules_dir)
+    self.root_node_modules_dir.as_ref()
   }
 
   fn resolve_pkg_folder_from_deno_module_req(
diff --git a/cli/resolver.rs b/cli/resolver.rs
index e035a313c..7b8766ee9 100644
--- a/cli/resolver.rs
+++ b/cli/resolver.rs
@@ -27,6 +27,7 @@ use deno_runtime::deno_node::errors::ClosestPkgJsonError;
 use deno_runtime::deno_node::errors::UrlToNodeResolutionError;
 use deno_runtime::deno_node::is_builtin_node_module;
 use deno_runtime::deno_node::parse_npm_pkg_name;
+use deno_runtime::deno_node::NodeModuleKind;
 use deno_runtime::deno_node::NodeResolution;
 use deno_runtime::deno_node::NodeResolutionMode;
 use deno_runtime::deno_node::NodeResolver;
@@ -66,8 +67,7 @@ pub struct ModuleCodeStringSource {
 
 #[derive(Debug)]
 pub struct CliNodeResolver {
-  // not used in the LSP
-  cjs_resolutions: Option<Arc<CjsResolutionStore>>,
+  cjs_resolutions: Arc<CjsResolutionStore>,
   fs: Arc<dyn deno_fs::FileSystem>,
   node_resolver: Arc<NodeResolver>,
   // todo(dsherret): remove this pub(crate)
@@ -76,7 +76,7 @@ pub struct CliNodeResolver {
 
 impl CliNodeResolver {
   pub fn new(
-    cjs_resolutions: Option<Arc<CjsResolutionStore>>,
+    cjs_resolutions: Arc<CjsResolutionStore>,
     fs: Arc<dyn deno_fs::FileSystem>,
     node_resolver: Arc<NodeResolver>,
     npm_resolver: Arc<dyn CliNpmResolver>,
@@ -120,10 +120,16 @@ impl CliNodeResolver {
     referrer: &ModuleSpecifier,
     mode: NodeResolutionMode,
   ) -> Result<Option<NodeResolution>, AnyError> {
+    let referrer_kind = if self.cjs_resolutions.contains(referrer) {
+      NodeModuleKind::Cjs
+    } else {
+      NodeModuleKind::Esm
+    };
+
     self.handle_node_resolve_result(
       self
         .node_resolver
-        .resolve(specifier, referrer, mode)
+        .resolve(specifier, referrer, referrer_kind, mode)
         .map_err(AnyError::from),
     )
   }
@@ -241,16 +247,12 @@ impl CliNodeResolver {
       let specifier =
         crate::node::resolve_specifier_into_node_modules(&specifier);
       if self.in_npm_package(&specifier) {
-        if let Some(cjs_resolutions) = &self.cjs_resolutions {
-          let resolution =
-            self.node_resolver.url_to_node_resolution(specifier)?;
-          if let NodeResolution::CommonJs(specifier) = &resolution {
-            cjs_resolutions.insert(specifier.clone());
-          }
-          return Ok(resolution.into_url());
-        } else {
-          return Ok(specifier);
+        let resolution =
+          self.node_resolver.url_to_node_resolution(specifier)?;
+        if let NodeResolution::CommonJs(specifier) = &resolution {
+          self.cjs_resolutions.insert(specifier.clone());
         }
+        return Ok(resolution.into_url());
       }
     }
 
@@ -272,9 +274,7 @@ impl CliNodeResolver {
       Some(response) => {
         if let NodeResolution::CommonJs(specifier) = &response {
           // remember that this was a common js resolution
-          if let Some(cjs_resolutions) = &self.cjs_resolutions {
-            cjs_resolutions.insert(specifier.clone());
-          }
+          self.cjs_resolutions.insert(specifier.clone());
         }
         Ok(Some(response))
       }
diff --git a/cli/standalone/binary.rs b/cli/standalone/binary.rs
index 4d6b32b7d..10a762093 100644
--- a/cli/standalone/binary.rs
+++ b/cli/standalone/binary.rs
@@ -69,7 +69,7 @@ pub enum NodeModules {
     node_modules_dir: Option<String>,
   },
   Byonm {
-    root_node_modules_dir: String,
+    root_node_modules_dir: Option<String>,
   },
 }
 
@@ -572,15 +572,16 @@ impl<'a> DenoCompileBinaryWriter<'a> {
           Some(root_dir),
           files,
           Some(NodeModules::Byonm {
-            root_node_modules_dir: root_dir_url
-              .specifier_key(
-                &ModuleSpecifier::from_directory_path(
-                  // will always be set for byonm
-                  resolver.root_node_modules_path().unwrap(),
-                )
-                .unwrap(),
-              )
-              .into_owned(),
+            root_node_modules_dir: resolver.root_node_modules_path().map(
+              |node_modules_dir| {
+                root_dir_url
+                  .specifier_key(
+                    &ModuleSpecifier::from_directory_path(node_modules_dir)
+                      .unwrap(),
+                  )
+                  .into_owned()
+              },
+            ),
           }),
         )
       }
diff --git a/cli/standalone/mod.rs b/cli/standalone/mod.rs
index 796551729..1df3895ef 100644
--- a/cli/standalone/mod.rs
+++ b/cli/standalone/mod.rs
@@ -490,7 +490,8 @@ pub async fn run(
       let vfs_root_dir_path = root_path.clone();
       let vfs = load_npm_vfs(vfs_root_dir_path.clone())
         .context("Failed to load vfs.")?;
-      let root_node_modules_dir = vfs.root().join(root_node_modules_dir);
+      let root_node_modules_dir =
+        root_node_modules_dir.map(|p| vfs.root().join(p));
       let fs = Arc::new(DenoCompileFileSystem::new(vfs))
         as Arc<dyn deno_fs::FileSystem>;
       let npm_resolver = create_cli_npm_resolver(
@@ -584,7 +585,7 @@ pub async fn run(
     )
   };
   let cli_node_resolver = Arc::new(CliNodeResolver::new(
-    Some(cjs_resolutions.clone()),
+    cjs_resolutions.clone(),
     fs.clone(),
     node_resolver.clone(),
     npm_resolver.clone(),
diff --git a/cli/tsc/99_main_compiler.js b/cli/tsc/99_main_compiler.js
index 3e37070a9..4fba5449f 100644
--- a/cli/tsc/99_main_compiler.js
+++ b/cli/tsc/99_main_compiler.js
@@ -731,6 +731,7 @@ delete Object.prototype.__proto__;
           /** @type {[string, ts.Extension] | undefined} */
           const resolved = ops.op_resolve(
             containingFilePath,
+            isCjsCache.has(containingFilePath),
             [fileReference.fileName],
           )?.[0];
           if (resolved) {
@@ -764,6 +765,7 @@ delete Object.prototype.__proto__;
       /** @type {Array<[string, ts.Extension] | undefined>} */
       const resolved = ops.op_resolve(
         base,
+        isCjsCache.has(base),
         specifiers,
       );
       if (resolved) {
diff --git a/cli/tsc/mod.rs b/cli/tsc/mod.rs
index 6306c9975..f6440266e 100644
--- a/cli/tsc/mod.rs
+++ b/cli/tsc/mod.rs
@@ -30,6 +30,7 @@ use deno_graph::GraphKind;
 use deno_graph::Module;
 use deno_graph::ModuleGraph;
 use deno_graph::ResolutionResolved;
+use deno_runtime::deno_node::NodeModuleKind;
 use deno_runtime::deno_node::NodeResolution;
 use deno_runtime::deno_node::NodeResolutionMode;
 use deno_runtime::deno_node::NodeResolver;
@@ -585,6 +586,8 @@ pub struct ResolveArgs {
   /// The base specifier that the supplied specifier strings should be resolved
   /// relative to.
   pub base: String,
+  /// If the base is cjs.
+  pub is_base_cjs: bool,
   /// A list of specifiers that should be resolved.
   pub specifiers: Vec<String>,
 }
@@ -594,9 +597,17 @@ pub struct ResolveArgs {
 fn op_resolve(
   state: &mut OpState,
   #[string] base: String,
+  is_base_cjs: bool,
   #[serde] specifiers: Vec<String>,
 ) -> Result<Vec<(String, String)>, AnyError> {
-  op_resolve_inner(state, ResolveArgs { base, specifiers })
+  op_resolve_inner(
+    state,
+    ResolveArgs {
+      base,
+      is_base_cjs,
+      specifiers,
+    },
+  )
 }
 
 #[inline]
@@ -607,6 +618,11 @@ fn op_resolve_inner(
   let state = state.borrow_mut::<State>();
   let mut resolved: Vec<(String, String)> =
     Vec::with_capacity(args.specifiers.len());
+  let referrer_kind = if args.is_base_cjs {
+    NodeModuleKind::Cjs
+  } else {
+    NodeModuleKind::Esm
+  };
   let referrer = if let Some(remapped_specifier) =
     state.remapped_specifiers.get(&args.base)
   {
@@ -646,7 +662,12 @@ fn op_resolve_inner(
       Some(ResolutionResolved { specifier, .. }) => {
         resolve_graph_specifier_types(specifier, &referrer, state)?
       }
-      _ => resolve_non_graph_specifier_types(&specifier, &referrer, state)?,
+      _ => resolve_non_graph_specifier_types(
+        &specifier,
+        &referrer,
+        referrer_kind,
+        state,
+      )?,
     };
     let result = match maybe_result {
       Some((specifier, media_type)) => {
@@ -766,6 +787,7 @@ fn resolve_graph_specifier_types(
 fn resolve_non_graph_specifier_types(
   specifier: &str,
   referrer: &ModuleSpecifier,
+  referrer_kind: NodeModuleKind,
   state: &State,
 ) -> Result<Option<(ModuleSpecifier, MediaType)>, AnyError> {
   let npm = match state.maybe_npm.as_ref() {
@@ -777,11 +799,17 @@ fn resolve_non_graph_specifier_types(
     // we're in an npm package, so use node resolution
     Ok(Some(NodeResolution::into_specifier_and_media_type(
       node_resolver
-        .resolve(specifier, referrer, NodeResolutionMode::Types)
+        .resolve(
+          specifier,
+          referrer,
+          referrer_kind,
+          NodeResolutionMode::Types,
+        )
         .ok()
         .flatten(),
     )))
   } else if let Ok(npm_req_ref) = NpmPackageReqReference::from_str(specifier) {
+    debug_assert_eq!(referrer_kind, NodeModuleKind::Esm);
     // todo(dsherret): add support for injecting this in the graph so
     // we don't need this special code here.
     // This could occur when resolving npm:@types/node when it is
@@ -1184,6 +1212,7 @@ mod tests {
       &mut state,
       ResolveArgs {
         base: "https://deno.land/x/a.ts".to_string(),
+        is_base_cjs: false,
         specifiers: vec!["./b.ts".to_string()],
       },
     )
@@ -1206,6 +1235,7 @@ mod tests {
       &mut state,
       ResolveArgs {
         base: "https://deno.land/x/a.ts".to_string(),
+        is_base_cjs: false,
         specifiers: vec!["./bad.ts".to_string()],
       },
     )