fix: improve auto-imports for npm packages (#27224)

Improves auto-imports when using `"nodeModulesDir": "auto"`

cli/lsp/analysis.rs

@@ -353,7 +353,12 @@ impl<'a> TsResponseImportMapper<'a> {
           let pkg_reqs = npm_resolver.resolve_pkg_reqs_from_pkg_id(&pkg_id);
           // check if any pkg reqs match what is found in an import map
           if !pkg_reqs.is_empty() {
-            let sub_path = self.resolve_package_path(specifier);
+            let sub_path = npm_resolver
+              .resolve_pkg_folder_from_pkg_id(&pkg_id)
+              .ok()
+              .and_then(|pkg_folder| {
+                self.resolve_package_path(specifier, &pkg_folder)
+              });
             if let Some(import_map) = self.maybe_import_map {
               let pkg_reqs = pkg_reqs.iter().collect::<HashSet<_>>();
               let mut matches = Vec::new();
@@ -368,8 +373,13 @@ impl<'a> TsResponseImportMapper<'a> {
                       if let Some(key_sub_path) =
                         sub_path.strip_prefix(value_sub_path)
                       {
-                        matches
+                        // keys that don't end in a slash can't be mapped to a subpath
-                          .push(format!("{}{}", entry.raw_key, key_sub_path));
+                        if entry.raw_key.ends_with('/')
+                          || key_sub_path.is_empty()
+                        {
+                          matches
+                            .push(format!("{}{}", entry.raw_key, key_sub_path));
+                        }
                       }
                     }
                   }
@@ -413,10 +423,16 @@ impl<'a> TsResponseImportMapper<'a> {
   fn resolve_package_path(
     &self,
     specifier: &ModuleSpecifier,
+    package_root_folder: &Path,
   ) -> Option<String> {
     let package_json = self
       .resolver
-      .get_closest_package_json(specifier)
+      .pkg_json_resolver(specifier)
+      // the specifier might have a closer package.json, but we
+      // want the root of the package's package.json
+      .get_closest_package_json_from_file_path(
+        &package_root_folder.join("package.json"),
+      )
       .ok()
       .flatten()?;
     let root_folder = package_json.path.parent()?;

cli/lsp/resolver.rs

@@ -20,14 +20,12 @@ use deno_resolver::DenoResolverOptions;
 use deno_resolver::NodeAndNpmReqResolver;
 use deno_runtime::deno_fs;
 use deno_runtime::deno_node::NodeResolver;
-use deno_runtime::deno_node::PackageJson;
 use deno_runtime::deno_node::PackageJsonResolver;
 use deno_semver::jsr::JsrPackageReqReference;
 use deno_semver::npm::NpmPackageReqReference;
 use deno_semver::package::PackageNv;
 use deno_semver::package::PackageReq;
 use indexmap::IndexMap;
-use node_resolver::errors::ClosestPkgJsonError;
 use node_resolver::InNpmPackageChecker;
 use node_resolver::NodeResolutionKind;
 use node_resolver::ResolutionMode;
@@ -380,6 +378,14 @@ impl LspResolver {
     resolver.npm_resolver.as_ref().and_then(|r| r.as_managed())
   }
 
+  pub fn pkg_json_resolver(
+    &self,
+    referrer: &ModuleSpecifier,
+  ) -> &Arc<PackageJsonResolver> {
+    let resolver = self.get_scope_resolver(Some(referrer));
+    &resolver.pkg_json_resolver
+  }
+
   pub fn graph_imports_by_referrer(
     &self,
     file_referrer: &ModuleSpecifier,
@@ -522,16 +528,6 @@ impl LspResolver {
       .is_some()
   }
 
-  pub fn get_closest_package_json(
-    &self,
-    referrer: &ModuleSpecifier,
-  ) -> Result<Option<Arc<PackageJson>>, ClosestPkgJsonError> {
-    let resolver = self.get_scope_resolver(Some(referrer));
-    resolver
-      .pkg_json_resolver
-      .get_closest_package_json(referrer)
-  }
-
   pub fn resolve_redirects(
     &self,
     specifier: &ModuleSpecifier,

cli/npm/managed/resolvers/local.rs

@@ -236,8 +236,21 @@ impl NpmPackageFsResolver for LocalNpmPackageResolver {
     else {
       return Ok(None);
     };
-    let folder_name = folder_path.parent().unwrap().to_string_lossy();
+    // ex. project/node_modules/.deno/preact@10.24.3/node_modules/preact/
-    Ok(get_package_folder_id_from_folder_name(&folder_name))
+    let Some(node_modules_ancestor) = folder_path
+      .ancestors()
+      .find(|ancestor| ancestor.ends_with("node_modules"))
+    else {
+      return Ok(None);
+    };
+    let Some(folder_name) =
+      node_modules_ancestor.parent().and_then(|p| p.file_name())
+    else {
+      return Ok(None);
+    };
+    Ok(get_package_folder_id_from_folder_name(
+      &folder_name.to_string_lossy(),
+    ))
   }
 
   async fn cache_packages(&self) -> Result<(), AnyError> {

ext/node/ops/require.rs

@@ -429,7 +429,9 @@ where
 
   let pkg_json_resolver = state.borrow::<PackageJsonResolverRc>();
   let pkg = pkg_json_resolver
-    .get_closest_package_json_from_path(&PathBuf::from(parent_path.unwrap()))
+    .get_closest_package_json_from_file_path(&PathBuf::from(
+      parent_path.unwrap(),
+    ))
     .ok()
     .flatten();
   if pkg.is_none() {
@@ -620,8 +622,8 @@ where
   let referrer_path = ensure_read_permission::<P>(state, &referrer_path)
     .map_err(RequireErrorKind::Permission)?;
   let pkg_json_resolver = state.borrow::<PackageJsonResolverRc>();
-  let Some(pkg) =
+  let Some(pkg) = pkg_json_resolver
-    pkg_json_resolver.get_closest_package_json_from_path(&referrer_path)?
+    .get_closest_package_json_from_file_path(&referrer_path)?
   else {
     return Ok(None);
   };

resolvers/node/package_json.rs

@@ -60,10 +60,10 @@ impl<TEnv: NodeResolverEnv> PackageJsonResolver<TEnv> {
     let Ok(file_path) = deno_path_util::url_to_file_path(url) else {
       return Ok(None);
     };
-    self.get_closest_package_json_from_path(&file_path)
+    self.get_closest_package_json_from_file_path(&file_path)
   }
 
-  pub fn get_closest_package_json_from_path(
+  pub fn get_closest_package_json_from_file_path(
     &self,
     file_path: &Path,
   ) -> Result<Option<PackageJsonRc>, ClosestPkgJsonError> {

tests/integration/lsp_tests.rs

@@ -9610,6 +9610,69 @@ fn lsp_completions_npm() {
   client.shutdown();
 }
 
+#[test]
+fn lsp_auto_imports_npm_auto() {
+  let context = TestContextBuilder::for_npm().use_temp_cwd().build();
+  let temp_dir_path = context.temp_dir().path();
+  temp_dir_path.join("deno.json").write_json(&json!({
+    "nodeModulesDir": "auto",
+    "imports": {
+      "preact": "npm:preact@^10.19.6",
+    },
+  }));
+  // add a file that uses the import so that typescript knows about it
+  temp_dir_path
+    .join("mod.ts")
+    .write("import { useEffect } from 'preact/hooks'; console.log(useEffect);");
+  context.run_deno("cache mod.ts");
+  let mut client = context.new_lsp_command().build();
+  client.initialize_default();
+  let file_uri = temp_dir_path.join("file.ts").url_file();
+  let mut diagnostics = client
+    .did_open(json!({
+      "textDocument": {
+        "uri": file_uri,
+        "languageId": "typescript",
+        "version": 1,
+        "text": "useEffect",
+      }
+    }))
+    .all();
+  assert_eq!(diagnostics.len(), 1);
+  let diagnostic = diagnostics.remove(0);
+  let res = client.write_request(
+    "textDocument/codeAction",
+    json!({
+      "textDocument": {
+        "uri": file_uri,
+      },
+      "range": {
+        "start": { "line": 0, "character": 0 },
+        "end": { "line": 0, "character": 9 }
+      },
+      "context": {
+        "diagnostics": [diagnostic],
+        "only": ["quickfix"]
+      }
+    }),
+  );
+  assert_eq!(
+    res
+      .as_array()
+      .unwrap()
+      .first()
+      .unwrap()
+      .as_object()
+      .unwrap()
+      .get("title")
+      .unwrap()
+      .as_str()
+      .unwrap(),
+    "Add import from \"preact/hooks\""
+  );
+  client.shutdown();
+}
+
 #[test]
 fn lsp_npm_specifier_unopened_file() {
   let context = TestContextBuilder::new()

tests/util/server/src/builders.rs

@@ -325,6 +325,15 @@ impl TestContext {
     builder
   }
 
+  pub fn run_deno(&self, args: impl AsRef<str>) {
+    self
+      .new_command()
+      .name("deno")
+      .args(args)
+      .run()
+      .skip_output_check();
+  }
+
   pub fn run_npm(&self, args: impl AsRef<str>) {
     self
       .new_command()