refactor: Remove PrettyJsError and js_error_create_fn (denoland#14378)

This commit:
- removes "fmt_errors::PrettyJsError" in favor of "format_js_error" fn
- removes "deno_core::JsError::create" and 
"deno_core::RuntimeOptions::js_error_create_fn"
- adds new option to "deno_runtime::ops::worker_host::init"

cli/fmt_errors.rs

@@ -4,11 +4,8 @@ use crate::colors::cyan;
 use crate::colors::italic_bold;
 use crate::colors::red;
 use crate::colors::yellow;
-use deno_core::error::{AnyError, JsError, JsStackFrame};
+use deno_core::error::{JsError, JsStackFrame};
 use deno_core::url::Url;
-use std::error::Error;
-use std::fmt;
-use std::ops::Deref;
 
 const SOURCE_ABBREV_THRESHOLD: usize = 150;
 const DATA_URL_ABBREV_THRESHOLD: usize = 150;
@@ -181,12 +178,12 @@ fn format_maybe_source_line(
  format!("\n{}{}\n{}{}", indent, source_line, indent, color_underline)
 }
 
-fn format_js_error(js_error: &JsError, is_child: bool) -> String {
+fn format_js_error_inner(js_error: &JsError, is_child: bool) -> String {
  let mut s = String::new();
  s.push_str(&js_error.exception_message);
  if let Some(aggregated) = &js_error.aggregated {
  for aggregated_error in aggregated {
- let error_string = format_js_error(aggregated_error, true);
+ let error_string = format_js_error_inner(aggregated_error, true);
  for line in error_string.trim_start_matches("Uncaught ").lines() {
  s.push_str(&format!("\n {}", line));
  }
@@ -209,7 +206,7 @@ fn format_js_error(js_error: &JsError, is_child: bool) -> String {
  s.push_str(&format!("\n at {}", format_frame(frame)));
  }
  if let Some(cause) = &js_error.cause {
- let error_string = format_js_error(cause, true);
+ let error_string = format_js_error_inner(cause, true);
  s.push_str(&format!(
  "\nCaused by: {}",
  error_string.trim_start_matches("Uncaught ")
@@ -218,33 +215,10 @@ fn format_js_error(js_error: &JsError, is_child: bool) -> String {
  s
 }
 
-/// Wrapper around deno_core::JsError which provides colorful
-/// string representation.
-#[derive(Debug)]
-pub struct PrettyJsError(JsError);
-
-impl PrettyJsError {
- pub fn create(js_error: JsError) -> AnyError {
- let pretty_js_error = Self(js_error);
- pretty_js_error.into()
- }
-}
-
-impl Deref for PrettyJsError {
- type Target = JsError;
- fn deref(&self) -> &Self::Target {
- &self.0
- }
+pub fn format_js_error(js_error: &JsError) -> String {
+ format_js_error_inner(js_error, false)
 }
 
-impl fmt::Display for PrettyJsError {
- fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
- write!(f, "{}", &format_js_error(&self.0, false))
- }
-}
-
-impl Error for PrettyJsError {}
-
 #[cfg(test)]
 mod tests {
  use super::*;

cli/main.rs

@@ -63,7 +63,7 @@ use crate::flags::TypeCheckMode;
 use crate::flags::UninstallFlags;
 use crate::flags::UpgradeFlags;
 use crate::flags::VendorFlags;
-use crate::fmt_errors::PrettyJsError;
+use crate::fmt_errors::format_js_error;
 use crate::graph_util::graph_lock_or_exit;
 use crate::graph_util::graph_valid;
 use crate::module_loader::CliModuleLoader;
@@ -73,6 +73,7 @@ use crate::resolver::JsxResolver;
 use deno_ast::MediaType;
 use deno_core::error::generic_error;
 use deno_core::error::AnyError;
+use deno_core::error::JsError;
 use deno_core::futures::future::FutureExt;
 use deno_core::futures::future::LocalFutureObj;
 use deno_core::futures::Future;
@@ -102,7 +103,6 @@ use std::io::Write;
 use std::iter::once;
 use std::path::PathBuf;
 use std::pin::Pin;
-use std::rc::Rc;
 use std::sync::Arc;
 
 fn create_web_worker_preload_module_callback(
@@ -170,8 +170,8 @@ fn create_web_worker_callback(
  module_loader,
  create_web_worker_cb,
  preload_module_cb,
+ format_js_error_fn: Some(Arc::new(format_js_error)),
  source_map_getter: Some(Box::new(ps.clone())),
- js_error_create_fn: Some(Rc::new(PrettyJsError::create)),
  use_deno_namespace: args.use_deno_namespace,
  worker_type: args.worker_type,
  maybe_inspector_server,
@@ -264,7 +264,7 @@ pub fn create_main_worker(
  user_agent: version::get_user_agent(),
  seed: ps.flags.seed,
  source_map_getter: Some(Box::new(ps.clone())),
- js_error_create_fn: Some(Rc::new(PrettyJsError::create)),
+ format_js_error_fn: Some(Arc::new(format_js_error)),
  create_web_worker_cb,
  web_worker_preload_module_cb,
  maybe_inspector_server,
@@ -1481,10 +1481,14 @@ fn unwrap_or_exit<T>(result: Result<T, AnyError>) -> T {
  match result {
  Ok(value) => value,
  Err(error) => {
+ let error_string = match error.downcast_ref::<JsError>() {
+ Some(e) => format_js_error(e),
+ None => format!("{:?}", error),
+ };
  eprintln!(
  "{}: {}",
  colors::red_bold("error"),
- format!("{:?}", error).trim_start_matches("error: ")
+ error_string.trim_start_matches("error: ")
  );
  std::process::exit(1);
  }

cli/standalone.rs

@@ -3,6 +3,7 @@
 use crate::colors;
 use crate::file_fetcher::get_source_from_data_url;
 use crate::flags::Flags;
+use crate::fmt_errors::format_js_error;
 use crate::ops;
 use crate::proc_state::ProcState;
 use crate::version;
@@ -293,7 +294,7 @@ pub async fn run(
  root_cert_store: Some(root_cert_store),
  seed: metadata.seed,
  source_map_getter: None,
- js_error_create_fn: None,
+ format_js_error_fn: Some(Arc::new(format_js_error)),
  create_web_worker_cb,
  web_worker_preload_module_cb,
  maybe_inspector_server: None,

cli/tests/integration/compile_tests.rs

@@ -105,9 +105,12 @@ fn standalone_error() {
  assert!(!output.status.success());
  assert_eq!(output.stdout, b"");
  let stderr = String::from_utf8(output.stderr).unwrap();
+ let stderr = util::strip_ansi_codes(&stderr).to_string();
  // On Windows, we cannot assert the file path (because '\').
  // Instead we just check for relevant output.
- assert!(stderr.contains("error: Error: boom!\n at boom (file:https://"));
+ assert!(stderr.contains("error: Uncaught Error: boom!"));
+ assert!(stderr.contains("throw new Error(\"boom!\");"));
+ assert!(stderr.contains("\n at boom (file:https://"));
  assert!(stderr.contains("standalone_error.ts:2:11"));
  assert!(stderr.contains("at foo (file:https://"));
  assert!(stderr.contains("standalone_error.ts:5:5"));
@@ -148,9 +151,12 @@ fn standalone_error_module_with_imports() {
  println!("{:#?}", &output);
  assert_eq!(output.stdout, b"hello\n");
  let stderr = String::from_utf8(output.stderr).unwrap();
+ let stderr = util::strip_ansi_codes(&stderr).to_string();
  // On Windows, we cannot assert the file path (because '\').
  // Instead we just check for relevant output.
- assert!(stderr.contains("error: Error: boom!\n at file:https://"));
+ assert!(stderr.contains("error: Uncaught Error: boom!"));
+ assert!(stderr.contains("throw new Error(\"boom!\");"));
+ assert!(stderr.contains("\n at file:https://"));
  assert!(stderr.contains("standalone_error_module_with_imports_2.ts:2:7"));
 }
 

cli/tests/integration/worker_tests.rs

@@ -104,3 +104,9 @@ itest!(worker_terminate_tla_crash {
  args: "run --quiet --reload workers/terminate_tla_crash.js",
  output: "workers/terminate_tla_crash.js.out",
 });
+
+itest!(worker_error_event {
+ args: "run --quiet -A workers/error_event.ts",
+ output: "workers/error_event.ts.out",
+ exit_code: 1,
+});

cli/tests/testdata/workers/error_event.ts

@@ -0,0 +1,11 @@
+const worker = new Worker(new URL("error.ts", import.meta.url).href, {
+ type: "module",
+});
+worker.addEventListener("error", (e) => {
+ console.log({
+ "message": e.message,
+ "filename": e.filename?.slice?.(-100),
+ "lineno": e.lineno,
+ "colno": e.colno,
+ });
+});

cli/tests/testdata/workers/error_event.ts.out

@@ -0,0 +1,13 @@
+error: Uncaught (in worker "") Error: foo
+ throw new Error("foo");
+ ^
+ at foo ([WILDCARD]/error.ts:2:9)
+ at [WILDCARD]/error.ts:5:1
+{
+ message: "Uncaught Error: foo",
+ filename: "[WILDCARD]/error.ts",
+ lineno: 2,
+ colno: 9
+}
+error: Uncaught (in promise) Error: Unhandled error in child worker.
+ at [WILDCARD]

cli/tools/test.rs

@@ -13,7 +13,7 @@ use crate::file_watcher::ResolutionResult;
 use crate::flags::Flags;
 use crate::flags::TestFlags;
 use crate::flags::TypeCheckMode;
-use crate::fmt_errors::PrettyJsError;
+use crate::fmt_errors::format_js_error;
 use crate::fs_util::collect_specifiers;
 use crate::fs_util::is_supported_test_ext;
 use crate::fs_util::is_supported_test_path;
@@ -557,8 +557,8 @@ fn abbreviate_test_error(js_error: &JsError) -> JsError {
  js_error
 }
 
-// This function maps JsError to PrettyJsError and applies some changes
-// specifically for test runner purposes:
+// This function prettifies `JsError` and applies some changes specifically for
+// test runner purposes:
 //
 // - filter out stack frames:
 // - if stack trace consists of mixed user and internal code, the frames
@@ -570,7 +570,7 @@ pub fn format_test_error(js_error: &JsError) -> String {
  .exception_message
  .trim_start_matches("Uncaught ")
  .to_string();
- PrettyJsError::create(js_error).to_string()
+ format_js_error(&js_error)
 }
 
 fn create_reporter(

core/error.rs

@@ -172,10 +172,6 @@ pub(crate) struct NativeJsError {
 }
 
 impl JsError {
- pub(crate) fn create(js_error: Self) -> Error {
- js_error.into()
- }
-
  pub fn from_v8_exception(
  scope: &mut v8::HandleScope,
  exception: v8::Local<v8::Value>,

core/runtime.rs

@@ -162,7 +162,6 @@ pub(crate) struct JsRuntimeState {
  /// of the event loop.
  dyn_module_evaluate_idle_counter: u32,
  pub(crate) source_map_getter: Option<Box<dyn SourceMapGetter>>,
- pub(crate) js_error_create_fn: Rc<JsErrorCreateFn>,
  pub(crate) pending_ops: FuturesUnordered<PendingOpFuture>,
  pub(crate) unrefed_ops: HashSet<i32>,
  pub(crate) have_unpolled_ops: bool,
@@ -231,11 +230,6 @@ pub struct RuntimeOptions {
  /// Source map reference for errors.
  pub source_map_getter: Option<Box<dyn SourceMapGetter>>,
 
- /// Allows a callback to be set whenever a V8 exception is made. This allows
- /// the caller to wrap the JsError into an error. By default this callback
- /// is set to `JsError::create()`.
- pub js_error_create_fn: Option<Rc<JsErrorCreateFn>>,
-
  /// Allows to map error type to a string "class" used to represent
  /// error in JavaScript.
  pub get_error_class_fn: Option<GetErrorClassFn>,
@@ -294,10 +288,6 @@ impl JsRuntime {
 
  let has_startup_snapshot = options.startup_snapshot.is_some();
 
- let js_error_create_fn = options
- .js_error_create_fn
- .unwrap_or_else(|| Rc::new(JsError::create));
-
  // Add builtins extension
  options
  .extensions
@@ -387,7 +377,6 @@ impl JsRuntime {
  has_tick_scheduled: false,
  js_wasm_streaming_cb: None,
  source_map_getter: options.source_map_getter,
- js_error_create_fn,
  pending_ops: FuturesUnordered::new(),
  unrefed_ops: HashSet::new(),
  shared_array_buffer_store: options.shared_array_buffer_store,
@@ -1062,14 +1051,13 @@ pub(crate) fn exception_to_err_result<'s, T>(
  js_error.exception_message.trim_start_matches("Uncaught ")
  );
  }
- let js_error = (state_rc.borrow().js_error_create_fn)(js_error);
 
  if is_terminating_exception {
  // Re-enable exception termination.
  scope.terminate_execution();
  }
 
- Err(js_error)
+ Err(js_error.into())
 }
 
 // Related to module loading

runtime/examples/hello_runtime.rs

@@ -45,7 +45,7 @@ async fn main() -> Result<(), AnyError> {
  user_agent: "hello_runtime".to_string(),
  seed: None,
  source_map_getter: None,
- js_error_create_fn: None,
+ format_js_error_fn: None,
  web_worker_preload_module_cb,
  create_web_worker_cb,
  maybe_inspector_server: None,

runtime/js/11_workers.js

@@ -134,17 +134,16 @@
  const event = new ErrorEvent("error", {
  cancelable: true,
  message: e.message,
- lineno: e.lineNumber ? e.lineNumber + 1 : undefined,
- colno: e.columnNumber ? e.columnNumber + 1 : undefined,
+ lineno: e.lineNumber ? e.lineNumber : undefined,
+ colno: e.columnNumber ? e.columnNumber : undefined,
  filename: e.fileName,
  error: null,
  });
 
  this.dispatchEvent(event);
  // Don't bubble error event to window for loader errors (`!e.fileName`).
- // TODO(nayeemrmn): Currently these are never bubbled because worker
- // error event fields aren't populated correctly and `e.fileName` is
- // always empty.
+ // TODO(nayeemrmn): It's not correct to use `e.fileName` to detect user
+ // errors. It won't be there for non-awaited async ops for example.
  if (e.fileName && !event.defaultPrevented) {
  window.dispatchEvent(event);
  }

runtime/ops/worker_host.rs

@@ -11,6 +11,7 @@ use crate::web_worker::WebWorkerHandle;
 use crate::web_worker::WebWorkerType;
 use crate::web_worker::WorkerControlEvent;
 use crate::web_worker::WorkerId;
+use crate::worker::FormatJsErrorFn;
 use deno_core::error::AnyError;
 use deno_core::futures::future::LocalFutureObj;
 use deno_core::op;
@@ -54,6 +55,9 @@ pub type PreloadModuleCb = dyn Fn(WebWorker) -> LocalFutureObj<'static, Result<W
 #[derive(Clone)]
 pub struct CreateWebWorkerCbHolder(Arc<CreateWebWorkerCb>);
 
+#[derive(Clone)]
+pub struct FormatJsErrorFnHolder(Option<Arc<FormatJsErrorFn>>);
+
 /// A holder for callback that can used to preload some modules into a WebWorker
 /// before actual worker code is executed. It's a struct instead of a type
 /// because `GothamState` used in `OpState` overrides
@@ -106,6 +110,7 @@ pub type WorkersTable = HashMap<WorkerId, WorkerThread>;
 pub fn init(
  create_web_worker_cb: Arc<CreateWebWorkerCb>,
  preload_module_cb: Arc<PreloadModuleCb>,
+ format_js_error_fn: Option<Arc<FormatJsErrorFn>>,
 ) -> Extension {
  Extension::builder()
  .state(move |state| {
@@ -118,6 +123,9 @@ pub fn init(
  let preload_module_cb_holder =
  PreloadModuleCbHolder(preload_module_cb.clone());
  state.put::<PreloadModuleCbHolder>(preload_module_cb_holder);
+ let format_js_error_fn_holder =
+ FormatJsErrorFnHolder(format_js_error_fn.clone());
+ state.put::<FormatJsErrorFnHolder>(format_js_error_fn_holder);
 
  Ok(())
  })
@@ -193,6 +201,8 @@ fn op_create_worker(
  state.put::<CreateWebWorkerCbHolder>(create_web_worker_cb.clone());
  let preload_module_cb = state.take::<PreloadModuleCbHolder>();
  state.put::<PreloadModuleCbHolder>(preload_module_cb.clone());
+ let format_js_error_fn = state.take::<FormatJsErrorFnHolder>();
+ state.put::<FormatJsErrorFnHolder>(format_js_error_fn.clone());
  state.put::<WorkerId>(worker_id.next().unwrap());
 
  let module_specifier = deno_core::resolve_url(&specifier)?;
@@ -238,6 +248,7 @@ fn op_create_worker(
  module_specifier,
  maybe_source_code,
  preload_module_cb.0,
+ format_js_error_fn.0,
  )
  })?;