refactor(node): combine node resolution code for resolving a package subpath from external code (#20791)

We had two methods that did the same functionality.

cli/args/package_json.rs

@@ -1,7 +1,5 @@
 // Copyright 2018-2023 the Deno authors. All rights reserved. MIT license.
 
-use std::collections::BTreeMap;
-use std::collections::HashMap;
 use std::path::Path;
 use std::path::PathBuf;
 
@@ -13,6 +11,7 @@ use deno_runtime::deno_node::PackageJson;
 use deno_semver::package::PackageReq;
 use deno_semver::VersionReq;
 use deno_semver::VersionReqSpecifierParseError;
+use indexmap::IndexMap;
 use thiserror::Error;
 
 #[derive(Debug, Error, Clone)]
@@ -26,7 +25,7 @@ pub enum PackageJsonDepValueParseError {
 }
 
 pub type PackageJsonDeps =
-  BTreeMap<String, Result<PackageReq, PackageJsonDepValueParseError>>;
+  IndexMap<String, Result<PackageReq, PackageJsonDepValueParseError>>;
 
 #[derive(Debug, Default)]
 pub struct PackageJsonDepsProvider(Option<PackageJsonDeps>);
@@ -92,24 +91,25 @@ pub fn get_local_package_json_version_reqs(
   }
 
   fn insert_deps(
-    deps: Option<&HashMap<String, String>>,
+    deps: Option<&IndexMap<String, String>>,
     result: &mut PackageJsonDeps,
   ) {
     if let Some(deps) = deps {
       for (key, value) in deps {
-        result.insert(key.to_string(), parse_entry(key, value));
+        result
+          .entry(key.to_string())
+          .or_insert_with(|| parse_entry(key, value));
       }
     }
   }
 
   let deps = package_json.dependencies.as_ref();
   let dev_deps = package_json.dev_dependencies.as_ref();
-  let mut result = BTreeMap::new();
+  let mut result = IndexMap::new();
 
-  // insert the dev dependencies first so the dependencies will
-  // take priority and overwrite any collisions
-  insert_deps(dev_deps, &mut result);
+  // favors the deps over dev_deps
   insert_deps(deps, &mut result);
+  insert_deps(dev_deps, &mut result);
 
   result
 }
@@ -182,7 +182,7 @@ mod test {
 
   fn get_local_package_json_version_reqs_for_tests(
     package_json: &PackageJson,
-  ) -> BTreeMap<String, Result<PackageReq, String>> {
+  ) -> IndexMap<String, Result<PackageReq, String>> {
     get_local_package_json_version_reqs(package_json)
       .into_iter()
       .map(|(k, v)| {
@@ -194,17 +194,17 @@ mod test {
           },
         )
       })
-      .collect::<BTreeMap<_, _>>()
+      .collect::<IndexMap<_, _>>()
   }
 
   #[test]
   fn test_get_local_package_json_version_reqs() {
     let mut package_json = PackageJson::empty(PathBuf::from("/package.json"));
-    package_json.dependencies = Some(HashMap::from([
+    package_json.dependencies = Some(IndexMap::from([
       ("test".to_string(), "^1.2".to_string()),
       ("other".to_string(), "npm:package@~1.3".to_string()),
     ]));
-    package_json.dev_dependencies = Some(HashMap::from([
+    package_json.dev_dependencies = Some(IndexMap::from([
       ("package_b".to_string(), "~2.2".to_string()),
       // should be ignored
       ("other".to_string(), "^3.2".to_string()),
@@ -212,7 +212,7 @@ mod test {
     let deps = get_local_package_json_version_reqs_for_tests(&package_json);
     assert_eq!(
       deps,
-      BTreeMap::from([
+      IndexMap::from([
         (
           "test".to_string(),
           Ok(PackageReq::from_str("test@^1.2").unwrap())
@@ -232,14 +232,14 @@ mod test {
   #[test]
   fn test_get_local_package_json_version_reqs_errors_non_npm_specifier() {
     let mut package_json = PackageJson::empty(PathBuf::from("/package.json"));
-    package_json.dependencies = Some(HashMap::from([(
+    package_json.dependencies = Some(IndexMap::from([(
       "test".to_string(),
       "1.x - 1.3".to_string(),
     )]));
     let map = get_local_package_json_version_reqs_for_tests(&package_json);
     assert_eq!(
       map,
-      BTreeMap::from([(
+      IndexMap::from([(
         "test".to_string(),
         Err(
           concat!(
@@ -256,7 +256,7 @@ mod test {
   #[test]
   fn test_get_local_package_json_version_reqs_skips_certain_specifiers() {
     let mut package_json = PackageJson::empty(PathBuf::from("/package.json"));
-    package_json.dependencies = Some(HashMap::from([
+    package_json.dependencies = Some(IndexMap::from([
       ("test".to_string(), "1".to_string()),
       ("work-test".to_string(), "workspace:1.1.1".to_string()),
       ("file-test".to_string(), "file:something".to_string()),
@@ -267,7 +267,7 @@ mod test {
     let result = get_local_package_json_version_reqs_for_tests(&package_json);
     assert_eq!(
       result,
-      BTreeMap::from([
+      IndexMap::from([
         (
           "file-test".to_string(),
           Err("Not implemented scheme 'file'".to_string()),

cli/lsp/documents.rs

@@ -47,7 +47,6 @@ use indexmap::IndexMap;
 use lsp::Url;
 use once_cell::sync::Lazy;
 use package_json::PackageJsonDepsProvider;
-use std::collections::BTreeMap;
 use std::collections::HashMap;
 use std::collections::HashSet;
 use std::collections::VecDeque;
@@ -1267,8 +1266,8 @@ impl Documents {
       if let Some(package_json_deps) = &maybe_package_json_deps {
         // We need to ensure the hashing is deterministic so explicitly type
         // this in order to catch if the type of package_json_deps ever changes
-        // from a sorted/deterministic BTreeMap to something else.
-        let package_json_deps: &BTreeMap<_, _> = *package_json_deps;
+        // from a sorted/deterministic IndexMap to something else.
+        let package_json_deps: &IndexMap<_, _> = *package_json_deps;
         for (key, value) in package_json_deps {
           hasher.write_hashable(key);
           match value {

ext/node/analyze.rs

@@ -216,7 +216,7 @@ impl<TCjsCodeAnalyzer: CjsCodeAnalyzer> NodeCodeTranslator<TCjsCodeAnalyzer> {
       if let Some(exports) = &package_json.exports {
         return self.node_resolver.package_exports_resolve(
           &package_json_path,
-          package_subpath,
+          &package_subpath,
           exports,
           referrer,
           NodeModuleKind::Esm,

ext/node/errors.rs

@@ -53,8 +53,8 @@ pub fn err_module_not_found(path: &str, base: &str, typ: &str) -> AnyError {
 }
 
 pub fn err_invalid_package_target(
-  pkg_path: String,
-  key: String,
+  pkg_path: &str,
+  key: &str,
   target: String,
   is_import: bool,
   maybe_referrer: Option<String>,
@@ -95,7 +95,7 @@ pub fn err_invalid_package_target(
 
 pub fn err_package_path_not_exported(
   mut pkg_path: String,
-  subpath: String,
+  subpath: &str,
   maybe_referrer: Option<String>,
   mode: NodeResolutionMode,
 ) -> AnyError {
@@ -178,7 +178,7 @@ mod test {
     assert_eq!(
       err_package_path_not_exported(
         "test_path".to_string(),
-        "./jsx-runtime".to_string(),
+        "./jsx-runtime",
         None,
         NodeResolutionMode::Types,
       )
@@ -188,7 +188,7 @@ mod test {
     assert_eq!(
       err_package_path_not_exported(
         "test_path".to_string(),
-        ".".to_string(),
+        ".",
         None,
         NodeResolutionMode::Types,
       )

ext/node/ops/require.rs

@@ -423,7 +423,7 @@ where
     node_resolver
       .package_exports_resolve(
         &pkg.path,
-        expansion,
+        &expansion,
         exports,
         &referrer,
         NodeModuleKind::Cjs,
@@ -507,7 +507,7 @@ where
     node_resolver
       .package_exports_resolve(
         &pkg.path,
-        format!(".{expansion}"),
+        &format!(".{expansion}"),
         exports,
         &referrer,
         NodeModuleKind::Cjs,

ext/node/package_json.rs

@@ -36,8 +36,8 @@ pub struct PackageJson {
   pub path: PathBuf,
   pub typ: String,
   pub types: Option<String>,
-  pub dependencies: Option<HashMap<String, String>>,
-  pub dev_dependencies: Option<HashMap<String, String>>,
+  pub dependencies: Option<IndexMap<String, String>>,
+  pub dev_dependencies: Option<IndexMap<String, String>>,
   pub scripts: Option<IndexMap<String, String>>,
 }
 
@@ -146,7 +146,7 @@ impl PackageJson {
 
     let dependencies = package_json.get("dependencies").and_then(|d| {
       if d.is_object() {
-        let deps: HashMap<String, String> =
+        let deps: IndexMap<String, String> =
           serde_json::from_value(d.to_owned()).unwrap();
         Some(deps)
       } else {
@@ -155,7 +155,7 @@ impl PackageJson {
     });
     let dev_dependencies = package_json.get("devDependencies").and_then(|d| {
       if d.is_object() {
-        let deps: HashMap<String, String> =
+        let deps: IndexMap<String, String> =
           serde_json::from_value(d.to_owned()).unwrap();
         Some(deps)
       } else {

ext/node/resolution.rs

@@ -327,18 +327,24 @@ impl NodeResolver {
 
   pub fn resolve_npm_reference(
     &self,
-    package_folder: &Path,
-    sub_path: Option<&str>,
+    package_dir: &Path,
+    package_subpath: Option<&str>,
     mode: NodeResolutionMode,
     permissions: &dyn NodePermissions,
   ) -> Result<Option<NodeResolution>, AnyError> {
+    let package_json_path = package_dir.join("package.json");
+    let referrer = ModuleSpecifier::from_directory_path(package_dir).unwrap();
+    let package_json =
+      self.load_package_json(permissions, package_json_path.clone())?;
     let node_module_kind = NodeModuleKind::Esm;
+    let package_subpath = package_subpath
+      .map(|s| format!("./{s}"))
+      .unwrap_or_else(|| ".".to_string());
     let maybe_resolved_path = self
-      .package_config_resolve(
-        &sub_path
-          .map(|s| format!("./{s}"))
-          .unwrap_or_else(|| ".".to_string()),
-        package_folder,
+      .resolve_package_subpath(
+        &package_json,
+        &package_subpath,
+        &referrer,
         node_module_kind,
         DEFAULT_CONDITIONS,
         mode,
@@ -346,8 +352,9 @@ impl NodeResolver {
       )
       .with_context(|| {
         format!(
-          "Failed resolving package config for '{}'",
-          package_folder.join("package.json").display()
+          "Failed resolving package subpath '{}' for '{}'",
+          package_subpath,
+          package_json.path.display()
         )
       })?;
     let resolved_path = match maybe_resolved_path {
@@ -440,53 +447,6 @@ impl NodeResolver {
     }
   }
 
-  fn package_config_resolve(
-    &self,
-    package_subpath: &str,
-    package_dir: &Path,
-    referrer_kind: NodeModuleKind,
-    conditions: &[&str],
-    mode: NodeResolutionMode,
-    permissions: &dyn NodePermissions,
-  ) -> Result<Option<PathBuf>, AnyError> {
-    let package_json_path = package_dir.join("package.json");
-    let referrer = ModuleSpecifier::from_directory_path(package_dir).unwrap();
-    let package_config =
-      self.load_package_json(permissions, package_json_path.clone())?;
-    if let Some(exports) = &package_config.exports {
-      let result = self.package_exports_resolve(
-        &package_json_path,
-        package_subpath.to_string(),
-        exports,
-        &referrer,
-        referrer_kind,
-        conditions,
-        mode,
-        permissions,
-      );
-      match result {
-        Ok(found) => return Ok(Some(found)),
-        Err(exports_err) => {
-          if mode.is_types() && package_subpath == "." {
-            if let Ok(Some(path)) =
-              self.legacy_main_resolve(&package_config, referrer_kind, mode)
-            {
-              return Ok(Some(path));
-            } else {
-              return Ok(None);
-            }
-          }
-          return Err(exports_err);
-        }
-      }
-    }
-    if package_subpath == "." {
-      return self.legacy_main_resolve(&package_config, referrer_kind, mode);
-    }
-
-    Ok(Some(package_dir.join(package_subpath)))
-  }
-
   /// Checks if the resolved file has a corresponding declaration file.
   pub(super) fn path_to_declaration_path(
     &self,
@@ -592,8 +552,8 @@ impl NodeResolver {
             let maybe_resolved = self.resolve_package_target(
               package_json_path.as_ref().unwrap(),
               imports.get(name).unwrap().to_owned(),
-              "".to_string(),
-              name.to_string(),
+              "",
+              name,
               referrer,
               referrer_kind,
               false,
@@ -635,8 +595,8 @@ impl NodeResolver {
               let maybe_resolved = self.resolve_package_target(
                 package_json_path.as_ref().unwrap(),
                 target,
-                best_match_subpath.unwrap(),
-                best_match.to_string(),
+                &best_match_subpath.unwrap(),
+                best_match,
                 referrer,
                 referrer_kind,
                 true,
@@ -665,8 +625,8 @@ impl NodeResolver {
   fn resolve_package_target_string(
     &self,
     target: String,
-    subpath: String,
-    match_: String,
+    subpath: &str,
+    match_: &str,
     package_json_path: &Path,
     referrer: &ModuleSpecifier,
     referrer_kind: NodeModuleKind,
@@ -746,9 +706,9 @@ impl NodeResolver {
     if subpath.is_empty() {
       return Ok(resolved_path);
     }
-    if invalid_segment_re.is_match(&subpath) {
+    if invalid_segment_re.is_match(subpath) {
       let request = if pattern {
-        match_.replace('*', &subpath)
+        match_.replace('*', subpath)
       } else {
         format!("{match_}{subpath}")
       };
@@ -767,7 +727,7 @@ impl NodeResolver {
         });
       return Ok(PathBuf::from(replaced.to_string()));
     }
-    Ok(resolved_path.join(&subpath).clean())
+    Ok(resolved_path.join(subpath).clean())
   }
 
   #[allow(clippy::too_many_arguments)]
@@ -775,8 +735,8 @@ impl NodeResolver {
     &self,
     package_json_path: &Path,
     target: Value,
-    subpath: String,
-    package_subpath: String,
+    subpath: &str,
+    package_subpath: &str,
     referrer: &ModuleSpecifier,
     referrer_kind: NodeModuleKind,
     pattern: bool,
@@ -901,7 +861,7 @@ impl NodeResolver {
   pub fn package_exports_resolve(
     &self,
     package_json_path: &Path,
-    package_subpath: String,
+    package_subpath: &str,
     package_exports: &Map<String, Value>,
     referrer: &ModuleSpecifier,
     referrer_kind: NodeModuleKind,
@@ -909,16 +869,16 @@ impl NodeResolver {
     mode: NodeResolutionMode,
     permissions: &dyn NodePermissions,
   ) -> Result<PathBuf, AnyError> {
-    if package_exports.contains_key(&package_subpath)
+    if package_exports.contains_key(package_subpath)
       && package_subpath.find('*').is_none()
       && !package_subpath.ends_with('/')
     {
-      let target = package_exports.get(&package_subpath).unwrap().to_owned();
+      let target = package_exports.get(package_subpath).unwrap().to_owned();
       let resolved = self.resolve_package_target(
         package_json_path,
         target,
-        "".to_string(),
-        package_subpath.to_string(),
+        "",
+        package_subpath,
         referrer,
         referrer_kind,
         false,
@@ -977,8 +937,8 @@ impl NodeResolver {
       let maybe_resolved = self.resolve_package_target(
         package_json_path,
         target,
-        best_match_subpath.unwrap(),
-        best_match.to_string(),
+        &best_match_subpath.unwrap(),
+        best_match,
         referrer,
         referrer_kind,
         true,
@@ -1032,7 +992,7 @@ impl NodeResolver {
         return self
           .package_exports_resolve(
             &package_config.path,
-            package_subpath,
+            &package_subpath,
             exports,
             referrer,
             referrer_kind,
@@ -1065,26 +1025,60 @@ impl NodeResolver {
     // Package match.
     let package_json =
       self.load_package_json(permissions, package_json_path)?;
+    self.resolve_package_subpath(
+      &package_json,
+      &package_subpath,
+      referrer,
+      referrer_kind,
+      conditions,
+      mode,
+      permissions,
+    )
+  }
+
+  #[allow(clippy::too_many_arguments)]
+  fn resolve_package_subpath(
+    &self,
+    package_json: &PackageJson,
+    package_subpath: &str,
+    referrer: &ModuleSpecifier,
+    referrer_kind: NodeModuleKind,
+    conditions: &[&str],
+    mode: NodeResolutionMode,
+    permissions: &dyn NodePermissions,
+  ) -> Result<Option<PathBuf>, AnyError> {
     if let Some(exports) = &package_json.exports {
-      return self
-        .package_exports_resolve(
-          &package_json.path,
-          package_subpath,
-          exports,
-          referrer,
-          referrer_kind,
-          conditions,
-          mode,
-          permissions,
-        )
-        .map(Some);
+      let result = self.package_exports_resolve(
+        &package_json.path,
+        package_subpath,
+        exports,
+        referrer,
+        referrer_kind,
+        conditions,
+        mode,
+        permissions,
+      );
+      match result {
+        Ok(found) => return Ok(Some(found)),
+        Err(exports_err) => {
+          if mode.is_types() && package_subpath == "." {
+            if let Ok(Some(path)) =
+              self.legacy_main_resolve(package_json, referrer_kind, mode)
+            {
+              return Ok(Some(path));
+            } else {
+              return Ok(None);
+            }
+          }
+          return Err(exports_err);
+        }
+      }
     }
     if package_subpath == "." {
-      return self.legacy_main_resolve(&package_json, referrer_kind, mode);
+      return self.legacy_main_resolve(package_json, referrer_kind, mode);
     }
 
-    let file_path = package_json.path.parent().unwrap().join(&package_subpath);
-
+    let file_path = package_json.path.parent().unwrap().join(package_subpath);
     if mode.is_types() {
       let maybe_declaration_path =
         self.path_to_declaration_path(file_path, referrer_kind);
@@ -1443,14 +1437,14 @@ fn throw_import_not_defined(
 }
 
 fn throw_invalid_package_target(
-  subpath: String,
+  subpath: &str,
   target: String,
   package_json_path: &Path,
   internal: bool,
   referrer: &ModuleSpecifier,
 ) -> AnyError {
   errors::err_invalid_package_target(
-    package_json_path.parent().unwrap().display().to_string(),
+    &package_json_path.parent().unwrap().display().to_string(),
     subpath,
     target,
     internal,
@@ -1478,7 +1472,7 @@ fn throw_invalid_subpath(
 }
 
 fn throw_exports_not_found(
-  subpath: String,
+  subpath: &str,
   package_json_path: &Path,
   referrer: &ModuleSpecifier,
   mode: NodeResolutionMode,