fix(npm): support bare specifiers in package.json having a path (#17903)

For example `import * as test from "package/path.js"`

cli/args/mod.rs

@@ -49,7 +49,7 @@ use deno_runtime::deno_tls::webpki_roots;
 use deno_runtime::inspector_server::InspectorServer;
 use deno_runtime::permissions::PermissionsOptions;
 use once_cell::sync::Lazy;
-use std::collections::HashMap;
+use std::collections::BTreeMap;
 use std::env;
 use std::io::BufReader;
 use std::io::Cursor;
@@ -799,7 +799,7 @@ impl CliOptions {
 
   pub fn maybe_package_json_deps(
     &self,
-  ) -> Result<Option<HashMap<String, NpmPackageReq>>, AnyError> {
+  ) -> Result<Option<BTreeMap<String, NpmPackageReq>>, AnyError> {
     if matches!(
       self.flags.subcommand,
       DenoSubcommand::Task(TaskFlags { task: None, .. })

cli/args/package_json.rs

@@ -1,5 +1,6 @@
 // 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;
@@ -36,10 +37,10 @@ pub fn parse_dep_entry_name_and_raw_version<'a>(
 /// entries to npm specifiers which can then be used in the resolver.
 pub fn get_local_package_json_version_reqs(
   package_json: &PackageJson,
-) -> Result<HashMap<String, NpmPackageReq>, AnyError> {
+) -> Result<BTreeMap<String, NpmPackageReq>, AnyError> {
   fn insert_deps(
     deps: Option<&HashMap<String, String>>,
-    result: &mut HashMap<String, NpmPackageReq>,
+    result: &mut BTreeMap<String, NpmPackageReq>,
   ) -> Result<(), AnyError> {
     if let Some(deps) = deps {
       for (key, value) in deps {
@@ -73,9 +74,7 @@ pub fn get_local_package_json_version_reqs(
 
   let deps = package_json.dependencies.as_ref();
   let dev_deps = package_json.dev_dependencies.as_ref();
-  let mut result = HashMap::with_capacity(
-    deps.map(|d| d.len()).unwrap_or(0) + dev_deps.map(|d| d.len()).unwrap_or(0),
-  );
+  let mut result = BTreeMap::new();
 
   // insert the dev dependencies first so the dependencies will
   // take priority and overwrite any collisions
@@ -166,7 +165,7 @@ mod test {
     let result = get_local_package_json_version_reqs(&package_json).unwrap();
     assert_eq!(
       result,
-      HashMap::from([
+      BTreeMap::from([
         (
           "test".to_string(),
           NpmPackageReq::from_str("test@^1.2").unwrap()

cli/lsp/documents.rs

@@ -1170,7 +1170,7 @@ impl Documents {
     fn calculate_resolver_config_hash(
       maybe_import_map: Option<&import_map::ImportMap>,
       maybe_jsx_config: Option<&JsxImportSourceConfig>,
-      maybe_package_json_deps: Option<&HashMap<String, NpmPackageReq>>,
+      maybe_package_json_deps: Option<&BTreeMap<String, NpmPackageReq>>,
     ) -> u64 {
       let mut hasher = FastInsecureHasher::default();
       if let Some(import_map) = maybe_import_map {
@@ -1181,7 +1181,6 @@ impl Documents {
         hasher.write_hashable(&jsx_config);
       }
       if let Some(deps) = maybe_package_json_deps {
-        let deps = deps.iter().collect::<BTreeMap<_, _>>();
         hasher.write_hashable(&deps);
       }
       hasher.finish()

cli/npm/resolvers/mod.rs

@@ -19,7 +19,7 @@ use deno_runtime::deno_node::RequireNpmResolver;
 use global::GlobalNpmPackageResolver;
 use serde::Deserialize;
 use serde::Serialize;
-use std::collections::HashMap;
+use std::collections::BTreeMap;
 use std::path::Path;
 use std::path::PathBuf;
 use std::sync::Arc;
@@ -227,7 +227,7 @@ impl NpmPackageResolver {
   /// Adds the package reqs from a package.json if they exist.
   pub async fn add_package_json_deps(
     &self,
-    maybe_package_json_deps: Option<&HashMap<String, NpmPackageReq>>,
+    maybe_package_json_deps: Option<&BTreeMap<String, NpmPackageReq>>,
   ) -> Result<(), AnyError> {
     if let Some(deps) = maybe_package_json_deps {
       let mut package_reqs = deps.values().cloned().collect::<Vec<_>>();

cli/resolver.rs

@@ -14,7 +14,7 @@ use deno_graph::source::UnknownBuiltInNodeModuleError;
 use deno_graph::source::DEFAULT_JSX_IMPORT_SOURCE_MODULE;
 use deno_runtime::deno_node::is_builtin_node_module;
 use import_map::ImportMap;
-use std::collections::HashMap;
+use std::collections::BTreeMap;
 use std::sync::Arc;
 
 use crate::args::JsxImportSourceConfig;
@@ -26,7 +26,7 @@ use crate::npm::NpmResolution;
 #[derive(Debug, Clone)]
 pub struct CliGraphResolver {
   maybe_import_map: Option<Arc<ImportMap>>,
-  maybe_package_json_deps: Option<HashMap<String, NpmPackageReq>>,
+  maybe_package_json_deps: Option<BTreeMap<String, NpmPackageReq>>,
   maybe_default_jsx_import_source: Option<String>,
   maybe_jsx_import_source_module: Option<String>,
   no_npm: bool,
@@ -62,7 +62,7 @@ impl CliGraphResolver {
     no_npm: bool,
     npm_registry_api: NpmRegistryApi,
     npm_resolution: NpmResolution,
-    maybe_package_json_deps: Option<HashMap<String, NpmPackageReq>>,
+    maybe_package_json_deps: Option<BTreeMap<String, NpmPackageReq>>,
   ) -> Self {
     Self {
       maybe_import_map,
@@ -120,6 +120,9 @@ impl Resolver for CliGraphResolver {
     }
 
     if let Some(deps) = self.maybe_package_json_deps.as_ref() {
+      if let Some(specifier) = resolve_package_json_dep(specifier, deps)? {
+        return Ok(specifier);
+      }
       if let Some(req) = deps.get(specifier) {
         return Ok(ModuleSpecifier::parse(&format!("npm:{req}")).unwrap());
       }
@@ -129,6 +132,25 @@ impl Resolver for CliGraphResolver {
   }
 }
 
+fn resolve_package_json_dep(
+  specifier: &str,
+  deps: &BTreeMap<String, NpmPackageReq>,
+) -> Result<Option<ModuleSpecifier>, deno_core::url::ParseError> {
+  for (bare_specifier, req) in deps {
+    if specifier.starts_with(bare_specifier) {
+      if specifier.len() == bare_specifier.len() {
+        return ModuleSpecifier::parse(&format!("npm:{req}")).map(Some);
+      }
+      let path = &specifier[bare_specifier.len()..];
+      if path.starts_with('/') {
+        return ModuleSpecifier::parse(&format!("npm:/{req}{path}")).map(Some);
+      }
+    }
+  }
+
+  Ok(None)
+}
+
 impl NpmResolver for CliGraphResolver {
   fn resolve_builtin_node_module(
     &self,
@@ -184,3 +206,64 @@ impl NpmResolver for CliGraphResolver {
       .resolve_package_req_for_deno_graph(package_req)
   }
 }
+
+#[cfg(test)]
+mod test {
+  use super::*;
+
+  #[test]
+  fn test_resolve_package_json_dep() {
+    fn resolve(
+      specifier: &str,
+      deps: &BTreeMap<String, NpmPackageReq>,
+    ) -> Result<Option<String>, String> {
+      resolve_package_json_dep(specifier, deps)
+        .map(|s| s.map(|s| s.to_string()))
+        .map_err(|err| err.to_string())
+    }
+
+    let deps = BTreeMap::from([
+      (
+        "package".to_string(),
+        NpmPackageReq::from_str("package@1.0").unwrap(),
+      ),
+      (
+        "package-alias".to_string(),
+        NpmPackageReq::from_str("package@^1.2").unwrap(),
+      ),
+      (
+        "@deno/test".to_string(),
+        NpmPackageReq::from_str("@deno/test@~0.2").unwrap(),
+      ),
+    ]);
+
+    assert_eq!(
+      resolve("package", &deps).unwrap(),
+      Some("npm:package@1.0".to_string()),
+    );
+    assert_eq!(
+      resolve("package/some_path.ts", &deps).unwrap(),
+      Some("npm:/package@1.0/some_path.ts".to_string()),
+    );
+
+    assert_eq!(
+      resolve("@deno/test", &deps).unwrap(),
+      Some("npm:@deno/test@~0.2".to_string()),
+    );
+    assert_eq!(
+      resolve("@deno/test/some_path.ts", &deps).unwrap(),
+      Some("npm:/@deno/test@~0.2/some_path.ts".to_string()),
+    );
+    // matches the start, but doesn't have the same length or a path
+    assert_eq!(resolve("@deno/testing", &deps).unwrap(), None,);
+
+    // alias
+    assert_eq!(
+      resolve("package-alias", &deps).unwrap(),
+      Some("npm:package@^1.2".to_string()),
+    );
+
+    // non-existent bare specifier
+    assert_eq!(resolve("non-existent", &deps).unwrap(), None);
+  }
+}

cli/tests/testdata/npm/node_modules_import/main.out

@@ -1,2 +1,3 @@
 2
 2
+2

cli/tests/testdata/npm/node_modules_import/main.ts

@@ -1,13 +1,16 @@
 import * as myImport1 from "@denotest/esm-basic";
 import * as myImport2 from "./node_modules/@denotest/esm-basic/main.mjs";
+import * as myImport3 from "@denotest/esm-basic/main.mjs";
 
 myImport1.setValue(5);
 myImport2.setValue(2);
 
-// these should both give type errors
+// these should all give type errors
 const value1: string = myImport1.getValue();
 const value2: string = myImport2.getValue();
+const value3: string = myImport3.getValue();
 
-// these should both be equal because it should be mutating the same module
+// these should all be equal because it should be mutating the same module
 console.log(value1);
 console.log(value2);
+console.log(value3);

cli/tests/testdata/npm/node_modules_import/main_check.out

@@ -1,11 +1,16 @@
 error: TS2322 [ERROR]: Type 'number' is not assignable to type 'string'.
 const value1: string = myImport1.getValue();
       ~~~~~~
-    at file:///[WILDCARD]/npm/node_modules_import/main.ts:8:7
+    at file:///[WILDCARD]/npm/node_modules_import/main.ts:9:7
 
 TS2322 [ERROR]: Type 'number' is not assignable to type 'string'.
 const value2: string = myImport2.getValue();
       ~~~~~~
-    at file:///[WILDCARD]/npm/node_modules_import/main.ts:9:7
+    at file:///[WILDCARD]/npm/node_modules_import/main.ts:10:7
 
-Found 2 errors.
+TS2322 [ERROR]: Type 'number' is not assignable to type 'string'.
+const value3: string = myImport3.getValue();
+      ~~~~~~
+    at file:///[WILDCARD]/npm/node_modules_import/main.ts:11:7
+
+Found 3 errors.