fix(repl): improve package.json support (#18497)

1. Fixes a cosmetic issue in the repl where it would display lsp warning
messages.
2. Lazily loads dependencies from the package.json on use.
3. Supports using bare specifiers from package.json in the REPL.

Closes #17929
Closes #18494

cli/args/flags.rs

@@ -526,7 +526,7 @@ impl Flags {
           .ok()
       }
       Task(_) | Check(_) | Coverage(_) | Cache(_) | Info(_) | Eval(_)
-      | Test(_) | Bench(_) => std::env::current_dir().ok(),
+      | Test(_) | Bench(_) | Repl(_) => std::env::current_dir().ok(),
       _ => None,
     }
   }

cli/lsp/documents.rs

@@ -834,8 +834,6 @@ pub struct Documents {
   /// A resolver that takes into account currently loaded import map and JSX
   /// settings.
   resolver: CliGraphResolver,
-  /// The npm package requirements found in a package.json file.
-  npm_package_json_reqs: Arc<Vec<NpmPackageReq>>,
   /// The npm package requirements found in npm specifiers.
   npm_specifier_reqs: Arc<Vec<NpmPackageReq>>,
   /// Gets if any document had a node: specifier such that a @types/node package
@@ -856,7 +854,6 @@ impl Documents {
       resolver_config_hash: 0,
       imports: Default::default(),
       resolver: CliGraphResolver::default(),
-      npm_package_json_reqs: Default::default(),
       npm_specifier_reqs: Default::default(),
       has_injected_types_node_package: false,
       specifier_resolver: Arc::new(SpecifierResolver::new(location)),
@@ -994,15 +991,9 @@ impl Documents {
   }
 
   /// Returns a collection of npm package requirements.
-  pub fn npm_package_reqs(&mut self) -> Vec<NpmPackageReq> {
+  pub fn npm_package_reqs(&mut self) -> Arc<Vec<NpmPackageReq>> {
     self.calculate_dependents_if_dirty();
-    let mut reqs = Vec::with_capacity(
-      self.npm_package_json_reqs.len() + self.npm_specifier_reqs.len(),
-    );
-    // resolve the package.json reqs first, then the npm specifiers
-    reqs.extend(self.npm_package_json_reqs.iter().cloned());
-    reqs.extend(self.npm_specifier_reqs.iter().cloned());
-    reqs
+    self.npm_specifier_reqs.clone()
   }
 
   /// Returns if a @types/node package was injected into the npm
@@ -1206,20 +1197,6 @@ impl Documents {
       maybe_jsx_config.as_ref(),
       maybe_package_json_deps.as_ref(),
     );
-    self.npm_package_json_reqs = Arc::new({
-      match &maybe_package_json_deps {
-        Some(deps) => {
-          let mut reqs = deps
-            .values()
-            .filter_map(|r| r.as_ref().ok())
-            .cloned()
-            .collect::<Vec<_>>();
-          reqs.sort();
-          reqs
-        }
-        None => Vec::new(),
-      }
-    });
     let deps_installer = PackageJsonDepsInstaller::new(
       npm_registry_api.clone(),
       npm_resolution.clone(),

cli/lsp/language_server.rs

@@ -13,7 +13,6 @@ use deno_runtime::deno_node::PackageJson;
 use deno_runtime::deno_web::BlobStore;
 use import_map::ImportMap;
 use log::error;
-use log::warn;
 use serde_json::from_value;
 use std::collections::HashMap;
 use std::collections::HashSet;
@@ -47,6 +46,7 @@ use super::documents::Documents;
 use super::documents::DocumentsFilter;
 use super::documents::LanguageId;
 use super::logging::lsp_log;
+use super::logging::lsp_warn;
 use super::lsp_custom;
 use super::parent_process_checker;
 use super::performance::Performance;
@@ -675,7 +675,7 @@ impl Inner {
       if let Some(ignored_options) = maybe_ignored_options {
         // TODO(@kitsonk) turn these into diagnostics that can be sent to the
         // client
-        warn!("{}", ignored_options);
+        lsp_warn!("{}", ignored_options);
       }
     }
 
@@ -1166,9 +1166,10 @@ impl Inner {
           LanguageId::Unknown
         });
     if language_id == LanguageId::Unknown {
-      warn!(
+      lsp_warn!(
         "Unsupported language id \"{}\" received for document \"{}\".",
-        params.text_document.language_id, params.text_document.uri
+        params.text_document.language_id,
+        params.text_document.uri
       );
     }
     let document = self.documents.open(
@@ -1209,8 +1210,12 @@ impl Inner {
 
   async fn refresh_npm_specifiers(&mut self) {
     let package_reqs = self.documents.npm_package_reqs();
-    if let Err(err) = self.npm_resolver.set_package_reqs(package_reqs).await {
-      warn!("Could not set npm package requirements. {:#}", err);
+    if let Err(err) = self
+      .npm_resolver
+      .set_package_reqs((*package_reqs).clone())
+      .await
+    {
+      lsp_warn!("Could not set npm package requirements. {:#}", err);
     }
   }
 
@@ -1463,7 +1468,7 @@ impl Inner {
       Ok(None) => Some(Vec::new()),
       Err(err) => {
         // TODO(lucacasonato): handle error properly
-        warn!("Format error: {:#}", err);
+        lsp_warn!("Format error: {:#}", err);
         None
       }
     };
@@ -2846,7 +2851,7 @@ impl tower_lsp::LanguageServer for LanguageServer {
         .register_capability(vec![registration])
         .await
       {
-        warn!("Client errored on capabilities.\n{:#}", err);
+        lsp_warn!("Client errored on capabilities.\n{:#}", err);
       }
     }
 

cli/lsp/logging.rs

@@ -6,6 +6,8 @@ use std::sync::atomic::Ordering;
 
 static LSP_DEBUG_FLAG: AtomicBool = AtomicBool::new(false);
 static LSP_LOG_LEVEL: AtomicUsize = AtomicUsize::new(log::Level::Info as usize);
+static LSP_WARN_LEVEL: AtomicUsize =
+  AtomicUsize::new(log::Level::Warn as usize);
 
 pub fn set_lsp_debug_flag(value: bool) {
   LSP_DEBUG_FLAG.store(value, Ordering::SeqCst)
@@ -15,6 +17,7 @@ pub fn lsp_debug_enabled() -> bool {
   LSP_DEBUG_FLAG.load(Ordering::SeqCst)
 }
 
+/// Change the lsp to log at the provided level.
 pub fn set_lsp_log_level(level: log::Level) {
   LSP_LOG_LEVEL.store(level as usize, Ordering::SeqCst)
 }
@@ -28,13 +31,40 @@ pub fn lsp_log_level() -> log::Level {
   }
 }
 
+/// Change the lsp to warn at the provided level.
+pub fn set_lsp_warn_level(level: log::Level) {
+  LSP_WARN_LEVEL.store(level as usize, Ordering::SeqCst)
+}
+
+pub fn lsp_warn_level() -> log::Level {
+  let level = LSP_LOG_LEVEL.load(Ordering::SeqCst);
+  // TODO(bartlomieju):
+  #[allow(clippy::undocumented_unsafe_blocks)]
+  unsafe {
+    std::mem::transmute(level)
+  }
+}
+
 /// Use this macro to do "info" logs in the lsp code. This allows
 /// for downgrading these logs to another log level in the REPL.
 macro_rules! lsp_log {
   ($($arg:tt)+) => (
-    let lsp_log_level = crate::lsp::logging::lsp_log_level();
+    let lsp_log_level = $crate::lsp::logging::lsp_log_level();
     if lsp_log_level == log::Level::Debug {
-      crate::lsp::logging::lsp_debug!($($arg)+)
+      $crate::lsp::logging::lsp_debug!($($arg)+)
+    } else {
+      log::log!(lsp_log_level, $($arg)+)
+    }
+  )
+}
+
+/// Use this macro to do "warn" logs in the lsp code. This allows
+/// for downgrading these logs to another log level in the REPL.
+macro_rules! lsp_warn {
+  ($($arg:tt)+) => (
+    let lsp_log_level = $crate::lsp::logging::lsp_warn_level();
+    if lsp_log_level == log::Level::Debug {
+      $crate::lsp::logging::lsp_debug!($($arg)+)
     } else {
       log::log!(lsp_log_level, $($arg)+)
     }
@@ -51,3 +81,4 @@ macro_rules! lsp_debug {
 
 pub(super) use lsp_debug;
 pub(super) use lsp_log;
+pub(super) use lsp_warn;

cli/lsp/repl.rs

@@ -53,7 +53,10 @@ pub struct ReplLanguageServer {
 
 impl ReplLanguageServer {
   pub async fn new_initialized() -> Result<ReplLanguageServer, AnyError> {
+    // downgrade info and warn lsp logging to debug
     super::logging::set_lsp_log_level(log::Level::Debug);
+    super::logging::set_lsp_warn_level(log::Level::Debug);
+
     let language_server =
       super::language_server::LanguageServer::new(Client::new_for_repl());
 

cli/lsp/tsc.rs

@@ -20,6 +20,7 @@ use super::urls::LspUrlMap;
 use super::urls::INVALID_SPECIFIER;
 
 use crate::args::TsConfig;
+use crate::lsp::logging::lsp_warn;
 use crate::tsc;
 use crate::tsc::ResolveArgs;
 use crate::util::path::relative_specifier;
@@ -43,7 +44,6 @@ use deno_core::ModuleSpecifier;
 use deno_core::OpState;
 use deno_core::RuntimeOptions;
 use deno_runtime::tokio_util::create_basic_runtime;
-use log::warn;
 use once_cell::sync::Lazy;
 use regex::Captures;
 use regex::Regex;
@@ -114,7 +114,7 @@ impl TsServer {
           }
           let value = request(&mut ts_runtime, state_snapshot, req, token);
           if tx.send(value).is_err() {
-            warn!("Unable to send result to client.");
+            lsp_warn!("Unable to send result to client.");
           }
         }
       })

cli/tests/integration/repl_tests.rs

@@ -5,6 +5,7 @@ use test_util::assert_contains;
 use test_util::assert_ends_with;
 use test_util::assert_not_contains;
 use util::TempDir;
+use util::TestContextBuilder;
 
 #[test]
 fn pty_multiline() {
@@ -884,3 +885,39 @@ fn pty_tab_indexable_props() {
     assert_not_contains!(output, "0", "1", "2");
   });
 }
+
+#[test]
+fn package_json_uncached_no_error() {
+  let test_context = TestContextBuilder::for_npm()
+    .use_temp_cwd()
+    .use_http_server()
+    .env("RUST_BACKTRACE", "1")
+    .build();
+  let temp_dir = test_context.temp_dir();
+  temp_dir.write(
+    "package.json",
+    r#"{
+  "dependencies": {
+    "@denotest/esm-basic": "1.0.0"
+  }
+}
+"#,
+  );
+  test_context.new_command().with_pty(|mut console| {
+    console.write_line("console.log(123 + 456);");
+    console.expect("579");
+    assert_not_contains!(
+      console.all_output(),
+      "Could not set npm package requirements",
+    );
+
+    // should support getting the package now though
+    console
+      .write_line("import { getValue, setValue } from '@denotest/esm-basic';");
+    console.expect("undefined");
+    console.write_line("setValue(12 + 30);");
+    console.expect("undefined");
+    console.write_line("getValue()");
+    console.expect("42")
+  });
+}

cli/tests/integration/run_tests.rs

@@ -4248,8 +4248,6 @@ itest!(permission_args_quiet {
 
 // Regression test for https://github.com/denoland/deno/issues/16772
 #[test]
-// todo(dsherret): getting a dns error on windows for some reason
-#[cfg(unix)]
 fn file_fetcher_preserves_permissions() {
   let _guard = util::http_server();
   util::with_pty(&["repl", "--quiet"], |mut console| {

cli/tools/repl/session.rs

@@ -106,7 +106,7 @@ pub fn result_to_evaluation_output(
   match r {
     Ok(value) => value,
     Err(err) => {
-      EvaluationOutput::Error(format!("{} {}", colors::red("error:"), err))
+      EvaluationOutput::Error(format!("{} {:#}", colors::red("error:"), err))
     }
   }
 }

test_util/src/builders.rs

@@ -312,6 +312,14 @@ impl TestCommandBuilder {
     .collect::<Vec<_>>()
   }
 
+  fn build_envs(&self) -> HashMap<String, String> {
+    let mut envs = self.context.envs.clone();
+    for (key, value) in &self.envs {
+      envs.insert(key.to_string(), value.to_string());
+    }
+    envs
+  }
+
   pub fn with_pty(&self, mut action: impl FnMut(Pty)) {
     if !Pty::is_supported() {
       return;
@@ -319,11 +327,20 @@ impl TestCommandBuilder {
 
     let args = self.build_args();
     let args = args.iter().map(|s| s.as_str()).collect::<Vec<_>>();
-    let mut envs = self.envs.clone();
+    let mut envs = self.build_envs();
     if !envs.contains_key("NO_COLOR") {
       // set this by default for pty tests
       envs.insert("NO_COLOR".to_string(), "1".to_string());
     }
+
+    // note(dsherret): for some reason I need to inject the current
+    // environment here for the pty tests or else I get dns errors
+    if !self.env_clear {
+      for (key, value) in std::env::vars() {
+        envs.entry(key).or_insert(value);
+      }
+    }
+
     action(Pty::new(
       &self.build_command_path(),
       &args,
@@ -361,13 +378,7 @@ impl TestCommandBuilder {
       command.env_clear();
     }
     command.env("DENO_DIR", self.context.deno_dir.path());
-    command.envs({
-      let mut envs = self.context.envs.clone();
-      for (key, value) in &self.envs {
-        envs.insert(key.to_string(), value.to_string());
-      }
-      envs
-    });
+    command.envs(self.build_envs());
     command.current_dir(cwd);
     command.stdin(Stdio::piped());
 

test_util/src/lib.rs

@@ -2166,24 +2166,12 @@ pub fn pattern_match(pattern: &str, s: &str, wildcard: &str) -> bool {
   t.1.is_empty()
 }
 
-pub fn with_pty(deno_args: &[&str], mut action: impl FnMut(Pty)) {
-  if !Pty::is_supported() {
-    return;
-  }
-
-  let deno_dir = new_deno_dir();
-  let mut env_vars = std::collections::HashMap::new();
-  env_vars.insert("NO_COLOR".to_string(), "1".to_string());
-  env_vars.insert(
-    "DENO_DIR".to_string(),
-    deno_dir.path().to_string_lossy().to_string(),
-  );
-  action(Pty::new(
-    &deno_exe_path(),
-    deno_args,
-    &testdata_path(),
-    Some(env_vars),
-  ))
+pub fn with_pty(deno_args: &[&str], action: impl FnMut(Pty)) {
+  let context = TestContextBuilder::default().build();
+  context
+    .new_command()
+    .args_vec(deno_args.iter().map(ToString::to_string).collect())
+    .with_pty(action);
 }
 
 pub struct WrkOutput {

test_util/src/pty.rs

@@ -1,5 +1,6 @@
 // Copyright 2018-2023 the Deno authors. All rights reserved. MIT license.
 
+use std::borrow::Cow;
 use std::collections::HashMap;
 use std::collections::HashSet;
 use std::io::Read;
@@ -35,7 +36,7 @@ impl Pty {
       read_bytes: Vec::new(),
       last_index: 0,
     };
-    if args[0] == "repl" && !args.contains(&"--quiet") {
+    if args.is_empty() || args[0] == "repl" && !args.contains(&"--quiet") {
       // wait for the repl to start up before writing to it
       pty.expect("exit using ctrl+d, ctrl+c, or close()");
     }
@@ -151,6 +152,10 @@ impl Pty {
     });
   }
 
+  pub fn all_output(&self) -> Cow<str> {
+    String::from_utf8_lossy(&self.read_bytes)
+  }
+
   #[track_caller]
   fn read_until_with_advancing(
     &mut self,