Repository: denoland/deno
Pull request: perf(lsp): use a stub module in tsc for failed resolutions

Previously when you edited any import statement, the ts server would re-resolve `jsxImportSource` from all modules in the graph. The resolution would return null because it's not a deno_graph-discovered dependency of those modules. Resolutions that return null are frequently retried. Instead make it resolve to a stub module `internal://missing_dependency.d.ts`.

Also resolve `jsxImportSource` up front using the `deno.json` as the referrer.

Changed files:
filename: cli/lsp/config.rs
patch: @@ -1112,19 +1112,29 @@ impl Default for LspTsConfig {
 }
 
 impl LspTsConfig {
-  pub fn new(config_file: Option<&ConfigFile>) -> Self {
+  pub fn new(
+    config_file: Option<&ConfigFile>,
+    import_map: Option<&ImportMap>,
+  ) -> Self {
     let mut ts_config = Self::default();
-    if let Some(config_file) = config_file {
-      match config_file.to_compiler_options() {
-        Ok((value, maybe_ignored_options)) => {
-          ts_config.inner.merge(&value);
-          if let Some(ignored_options) = maybe_ignored_options {
-            lsp_warn!("{}", ignored_options);
-          }
-        }
-        Err(err) => lsp_warn!("{}", err),
-      }
+    match ts_config.inner.merge_tsconfig_from_config_file(config_file) {
+      Ok(Some(ignored_options)) => lsp_warn!("{}", ignored_options),
+      Err(err) => lsp_warn!("{}", err),
+      _ => {}
     }
+    let mut maybe_map_jsx_import_source = || {
+      let import_map = import_map?;
+      let referrer = &config_file?.specifier;
+      let compiler_options = ts_config.inner.0.as_object_mut()?;
+      let jsx_import_source =
+        compiler_options.get("jsxImportSource")?.as_str()?;
+      let jsx_import_source =
+        import_map.resolve(jsx_import_source, referrer).ok()?;
+      compiler_options
+        .insert("jsxImportSource".to_string(), json!(jsx_import_source));
+      Some(())
+    };
+    maybe_map_jsx_import_source();
     ts_config
   }
 }
@@ -1267,7 +1277,6 @@ impl ConfigData {
       .unwrap_or_default();
     let lint_rules =
       get_configured_rules(lint_options.rules.clone(), config_file.as_ref());
-    let ts_config = LspTsConfig::new(config_file.as_ref());
     let vendor_dir = config_file.as_ref().and_then(|c| c.vendor_dir_path());
 
     // Load lockfile
@@ -1436,6 +1445,7 @@ impl ConfigData {
         }
       }
     }
+    let ts_config = LspTsConfig::new(config_file.as_ref(), import_map.as_ref());
 
     ConfigData {
       config_file: config_file.map(Arc::new),

filename: cli/lsp/tsc.rs
patch: @@ -28,6 +28,7 @@ use crate::lsp::documents::Documents;
 use crate::lsp::logging::lsp_warn;
 use crate::tsc;
 use crate::tsc::ResolveArgs;
+use crate::tsc::MISSING_DEPENDENCY_SPECIFIER;
 use crate::util::path::relative_specifier;
 use crate::util::path::specifier_to_file_path;
 use crate::util::path::to_percent_decoded_str;
@@ -4008,7 +4009,7 @@ fn op_load<'s>(
     .mark_with_args("tsc.op.op_load", specifier);
   let specifier = state.specifier_map.normalize(specifier)?;
   let maybe_load_response =
-    if specifier.as_str() == "internal:///missing_dependency.d.ts" {
+    if specifier.as_str() == MISSING_DEPENDENCY_SPECIFIER {
       None
     } else {
       let asset_or_document = state.get_asset_or_document(&specifier);
@@ -4026,11 +4027,11 @@ fn op_load<'s>(
 }
 
 #[op2]
-fn op_resolve<'s>(
-  scope: &'s mut v8::HandleScope,
+#[serde]
+fn op_resolve(
   state: &mut OpState,
   #[serde] args: ResolveArgs,
-) -> Result<v8::Local<'s, v8::Value>, AnyError> {
+) -> Result<Vec<Option<(String, String)>>, AnyError> {
   let state = state.borrow_mut::<State>();
   let mark = state.performance.mark_with_args("tsc.op.op_resolve", &args);
   let referrer = state.specifier_map.normalize(&args.base)?;
@@ -4043,13 +4044,17 @@ fn op_resolve<'s>(
       );
       resolved
         .into_iter()
-        .map(|o| {
-          o.map(|(s, mt)| {
-            (
-              state.specifier_map.denormalize(&s),
-              mt.as_ts_extension().to_string(),
-            )
-          })
+        // Resolved `node:` specifier means the user doesn't have @types/node,
+        // resolve to stub.
+        .map(|o| match o.filter(|(s, _)| s.scheme() != "node") {
+          Some((s, mt)) => Some((
+            state.specifier_map.denormalize(&s),
+            mt.as_ts_extension().to_string(),
+          )),
+          None => Some((
+            MISSING_DEPENDENCY_SPECIFIER.to_string(),
+            MediaType::Dts.as_ts_extension().to_string(),
+          )),
         })
         .collect()
     }
@@ -4062,9 +4067,8 @@ fn op_resolve<'s>(
     }
   };
 
-  let response = serde_v8::to_v8(scope, specifiers)?;
   state.performance.measure(mark);
-  Ok(response)
+  Ok(specifiers)
 }
 
 #[op2]
@@ -4750,6 +4754,14 @@ mod tests {
     (ts_server, snapshot, cache)
   }
 
+  fn setup_op_state(state_snapshot: Arc<StateSnapshot>) -> OpState {
+    let state =
+      State::new(state_snapshot, Default::default(), Default::default());
+    let mut op_state = OpState::new(None);
+    op_state.put(state);
+    op_state
+  }
+
   #[test]
   fn test_replace_links() {
     let actual = replace_links(r"test {@link http://deno.land/x/mod.ts} test");
@@ -5552,4 +5564,36 @@ mod tests {
       Some(false)
     );
   }
+
+  #[tokio::test]
+  async fn resolve_unknown_dependency_to_stub_module() {
+    let temp_dir = TempDir::new();
+    let (_, snapshot, _) = setup(
+      &temp_dir,
+      json!({
+        "target": "esnext",
+        "module": "esnext",
+        "lib": ["deno.ns", "deno.window"],
+        "noEmit": true,
+      }),
+      &[("file:///a.ts", "", 1, LanguageId::TypeScript)],
+    )
+    .await;
+    let mut state = setup_op_state(snapshot);
+    let resolved = op_resolve::call(
+      &mut state,
+      ResolveArgs {
+        base: "file:///a.ts".to_string(),
+        specifiers: vec!["./b.ts".to_string()],
+      },
+    )
+    .unwrap();
+    assert_eq!(
+      resolved,
+      vec![Some((
+        MISSING_DEPENDENCY_SPECIFIER.to_string(),
+        MediaType::Dts.as_ts_extension().to_string()
+      ))]
+    );
+  }
 }

filename: cli/tsc/99_main_compiler.js
patch: @@ -725,10 +725,6 @@ delete Object.prototype.__proto__;
           if (item) {
             isCjsCache.add(item);
             const [resolvedFileName, extension] = item;
-            if (resolvedFileName.startsWith("node:")) {
-              // probably means the user doesn't have @types/node, so resolve to undefined
-              return undefined;
-            }
             return {
               resolvedFileName,
               extension,

filename: cli/tsc/mod.rs
patch: @@ -30,7 +30,6 @@ use deno_graph::GraphKind;
 use deno_graph::Module;
 use deno_graph::ModuleGraph;
 use deno_graph::ResolutionResolved;
-use deno_runtime::deno_node;
 use deno_runtime::deno_node::NodeResolution;
 use deno_runtime::deno_node::NodeResolutionMode;
 use deno_runtime::deno_node::NodeResolver;
@@ -444,6 +443,9 @@ pub fn as_ts_script_kind(media_type: MediaType) -> i32 {
   }
 }
 
+pub const MISSING_DEPENDENCY_SPECIFIER: &str =
+  "internal:///missing_dependency.d.ts";
+
 #[derive(Debug, Serialize)]
 #[serde(rename_all = "camelCase")]
 struct LoadResponse {
@@ -471,7 +473,7 @@ fn op_load(
     state.maybe_tsbuildinfo.as_deref().map(Cow::Borrowed)
   // in certain situations we return a "blank" module to tsc and we need to
   // handle the request for that module here.
-  } else if load_specifier == "internal:///missing_dependency.d.ts" {
+  } else if load_specifier == MISSING_DEPENDENCY_SPECIFIER {
     None
   } else if let Some(name) = load_specifier.strip_prefix("asset:///") {
     let maybe_source = get_lazily_loaded_asset(name);
@@ -575,14 +577,12 @@ fn op_resolve(
     )?
   };
   for specifier in args.specifiers {
-    if let Some(module_name) = specifier.strip_prefix("node:") {
-      if deno_node::is_builtin_node_module(module_name) {
-        // return itself for node: specifiers because during type checking
-        // we resolve to the ambient modules in the @types/node package
-        // rather than deno_std/node
-        resolved.push((specifier, MediaType::Dts.to_string()));
-        continue;
-      }
+    if specifier.starts_with("node:") {
+      resolved.push((
+        MISSING_DEPENDENCY_SPECIFIER.to_string(),
+        MediaType::Dts.to_string(),
+      ));
+      continue;
     }
 
     if specifier.starts_with("asset:///") {
@@ -632,7 +632,7 @@ fn op_resolve(
         (specifier_str, media_type.as_ts_extension().into())
       }
       None => (
-        "internal:///missing_dependency.d.ts".to_string(),
+        MISSING_DEPENDENCY_SPECIFIER.to_string(),
         ".d.ts".to_string(),
       ),
     };
@@ -1159,7 +1159,7 @@ mod tests {
     .expect("should have not errored");
     assert_eq!(
       actual,
-      vec![("internal:///missing_dependency.d.ts".into(), ".d.ts".into())]
+      vec![(MISSING_DEPENDENCY_SPECIFIER.into(), ".d.ts".into())]
     );
   }