fix(repl): improve package.json support

cli/args/flags.rs

@@ -527,7 +527,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,
     }
   }
@@ -2245,7 +2245,7 @@ fn lock_arg() -> Arg {
   Arg::new("lock")
     .long("lock")
     .value_name("FILE")
-    .help("Check the specified lock file. 
+    .help("Check the specified lock file.
 
 If value is not provided, defaults to \"deno.lock\" in the current working directory.")
     .num_args(0..=1)

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)+)
+    } 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)+)
+      $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,18 +312,35 @@ 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;
     }
 
     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 {