From 121769844d4456b296e42d60794813da1b1472eb Mon Sep 17 00:00:00 2001
From: David Sherret <dsherret@users.noreply.github.com>
Date: Fri, 3 May 2024 16:44:41 -0400
Subject: [PATCH] fix(lsp): always cache all npm packages (#23679)

Closes #23659
---
 cli/lsp/language_server.rs     | 22 +++++++++--
 tests/integration/lsp_tests.rs | 71 ++++++++++++++++++++++++++++++++--
 tools/lint.js                  |  2 +-
 3 files changed, 87 insertions(+), 8 deletions(-)

diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs
index ff0fa296e3..01c9ac02f6 100644
--- a/cli/lsp/language_server.rs
+++ b/cli/lsp/language_server.rs
@@ -279,9 +279,9 @@ impl LanguageServer {
       Ok(())
     }
 
-    // do as much as possible in a read, then do a write outside
+    // prepare the cache inside the lock
     let maybe_prepare_cache_result = {
-      let inner = self.0.read().await; // ensure dropped
+      let mut inner = self.0.write().await; // ensure dropped
       match inner.prepare_cache(specifiers, referrer, force_global_cache) {
         Ok(maybe_cache_result) => maybe_cache_result,
         Err(err) => {
@@ -297,6 +297,7 @@ impl LanguageServer {
       }
     };
     if let Some(result) = maybe_prepare_cache_result {
+      // cache outside the lock
       let cli_options = result.cli_options;
       let roots = result.roots;
       let open_docs = result.open_docs;
@@ -312,6 +313,8 @@ impl LanguageServer {
           .client
           .show_message(MessageType::WARNING, err);
       }
+
+      // now get the lock back to update with the new information
       let mut inner = self.0.write().await;
       let lockfile = inner.config.tree.root_lockfile().cloned();
       inner.documents.refresh_lockfile(lockfile);
@@ -3280,7 +3283,7 @@ struct PrepareCacheResult {
 // These are implementations of custom commands supported by the LSP
 impl Inner {
   fn prepare_cache(
-    &self,
+    &mut self,
     specifiers: Vec<ModuleSpecifier>,
     referrer: ModuleSpecifier,
     force_global_cache: bool,
@@ -3289,11 +3292,22 @@ impl Inner {
       .performance
       .mark_with_args("lsp.cache", (&specifiers, &referrer));
     let config_data = self.config.tree.root_data();
-    let roots = if !specifiers.is_empty() {
+    let mut roots = if !specifiers.is_empty() {
       specifiers
     } else {
       vec![referrer.clone()]
     };
+
+    // always include the npm packages since resolution of one npm package
+    // might affect the resolution of other npm packages
+    roots.extend(
+      self
+        .documents
+        .npm_package_reqs()
+        .iter()
+        .map(|req| ModuleSpecifier::parse(&format!("npm:{}", req)).unwrap()),
+    );
+
     let workspace_settings = self.config.workspace_settings();
     let cli_options = CliOptions::new(
       Flags {
diff --git a/tests/integration/lsp_tests.rs b/tests/integration/lsp_tests.rs
index faec27e1c5..7bac0192da 100644
--- a/tests/integration/lsp_tests.rs
+++ b/tests/integration/lsp_tests.rs
@@ -5391,7 +5391,8 @@ fn lsp_code_actions_deno_cache_all() {
 
   let res =
     client
-    .write_request(      "textDocument/codeAction",
+    .write_request(
+      "textDocument/codeAction",
       json!({
         "textDocument": {
           "uri": "file:///a/file.ts",
@@ -5417,8 +5418,7 @@ fn lsp_code_actions_deno_cache_all() {
           "only": ["quickfix"],
         }
       }),
-    )
-    ;
+    );
   assert_eq!(
     res,
     json!([
@@ -7078,6 +7078,71 @@ fn lsp_npm_completions_auto_import_and_quick_fix_no_import_map() {
   client.shutdown();
 }
 
+#[test]
+fn lsp_npm_always_caches() {
+  // npm specifiers should always be cached even when not specified
+  // because they affect the resolution of each other
+  let context = TestContextBuilder::new()
+    .use_http_server()
+    .use_temp_cwd()
+    .build();
+  let temp_dir_path = context.temp_dir().path();
+
+  // this file should be auto-discovered by the lsp
+  let not_opened_file = temp_dir_path.join("not_opened.ts");
+  not_opened_file.write("import chalk from 'npm:chalk@5.0';\n");
+
+  // create the lsp and cache a different npm specifier
+  let mut client = context.new_lsp_command().build();
+  client.initialize_default();
+  let opened_file_uri = temp_dir_path.join("file.ts").uri_file();
+  client.did_open(
+    json!({
+      "textDocument": {
+        "uri": opened_file_uri,
+        "languageId": "typescript",
+        "version": 1,
+        "text": "import {getClient} from 'npm:@denotest/types-exports-subpaths@1/client';\n",
+      }
+    }),
+  );
+  client.write_request(
+    "workspace/executeCommand",
+    json!({
+      "command": "deno.cache",
+      "arguments": [
+        ["npm:@denotest/types-exports-subpaths@1/client"],
+        opened_file_uri,
+      ],
+    }),
+  );
+
+  // now open a new file and chalk should be working
+  let new_file_uri = temp_dir_path.join("new_file.ts").uri_file();
+  client.did_open(json!({
+    "textDocument": {
+      "uri": new_file_uri,
+      "languageId": "typescript",
+      "version": 1,
+      "text": "import chalk from 'npm:chalk@5.0';\nchalk.",
+    }
+  }));
+
+  let list = client.get_completion_list(
+    new_file_uri,
+    (1, 6),
+    json!({
+      "triggerKind": 2,
+      "triggerCharacter": "."
+    }),
+  );
+  assert!(!list.is_incomplete);
+  assert!(list.items.iter().any(|i| i.label == "green"));
+  assert!(list.items.iter().any(|i| i.label == "red"));
+
+  client.shutdown();
+}
+
 #[test]
 fn lsp_semantic_tokens_for_disabled_module() {
   let context = TestContextBuilder::new()
diff --git a/tools/lint.js b/tools/lint.js
index 38077273cf..36e79b45b0 100755
--- a/tools/lint.js
+++ b/tools/lint.js
@@ -51,7 +51,7 @@ async function dlint() {
     ":!:cli/bench/testdata/react-dom.js",
     ":!:cli/compilers/wasm_wrap.js",
     ":!:cli/tsc/dts/**",
-    ":!:target/**",
+    ":!:target/",
     ":!:tests/specs/**",
     ":!:tests/testdata/encoding/**",
     ":!:tests/testdata/error_syntax.js",