Merge pull request #3003 from finos/new-error-ux

New error UX

Author: texodus

Cargo.lock

@@ -93,7 +93,7 @@ function tests(context, compare) {
         // NOTE This is the error message we expect when `restore()` is called
         // without a `Table`, subject to change.
         expect(result).toEqual(
-            "Error: Trying to draw the viewer with no table attached"
+            "Error: Failed to construct table from JsValue(undefined)"
         );
         await page.evaluate(async () => {
             await workspace.replaceTable(

packages/perspective-workspace/test/js/table.spec.js

@@ -67,7 +67,7 @@ type Box2Fn<I, J, O> = Box<dyn Fn(I, J) -> O + Send + Sync + 'static>;
 
 type Subscriptions<C> = Arc<RwLock<HashMap<u32, C>>>;
 type OnErrorCallback =
-    Box2Fn<Option<String>, Option<ReconnectCallback>, BoxFuture<'static, Result<(), ClientError>>>;
+    Box2Fn<ClientError, Option<ReconnectCallback>, BoxFuture<'static, Result<(), ClientError>>>;
 type OnceCallback = Box<dyn FnOnce(Response) -> ClientResult<()> + Send + Sync + 'static>;
 type SendCallback = Arc<
     dyn for<'a> Fn(&'a Request) -> BoxFuture<'a, Result<(), Box<dyn Error + Send + Sync>>>
@@ -241,7 +241,7 @@ impl Client {
     /// Handle an exception from the underlying transport.
     pub async fn handle_error<T, U>(
         &self,
-        message: Option<String>,
+        message: ClientError,
         reconnect: Option<T>,
     ) -> ClientResult<()>
     where
@@ -262,12 +262,14 @@ impl Client {
         }));
 
         tasks.await.into_iter().collect::<Result<(), _>>()?;
+        self.subscriptions.write().await.clear();
+        self.subscriptions_once.write().await.clear();
         Ok(())
     }
 
     pub async fn on_error<T, U, V>(&self, on_error: T) -> ClientResult<u32>
     where
-        T: Fn(Option<String>, Option<ReconnectCallback>) -> U + Clone + Send + Sync + 'static,
+        T: Fn(ClientError, Option<ReconnectCallback>) -> U + Clone + Send + Sync + 'static,
         U: Future<Output = V> + Send + 'static,
         V: Into<Result<(), ClientError>> + Sync + 'static,
     {

rust/perspective-client/src/rust/client.rs

@@ -25,7 +25,7 @@ use thiserror::*;
 
 use crate::proto;
 
-#[derive(Error, Debug)]
+#[derive(Clone, Error, Debug)]
 pub enum ClientError {
     #[error("View not found")]
     ViewNotFound,
@@ -61,7 +61,7 @@ pub enum ClientError {
     BadTableOptions,
 
     #[error("External error: {0}")]
-    ExternalError(#[from] Box<dyn std::error::Error + Send + Sync>),
+    ExternalError(Arc<Box<dyn std::error::Error + Send + Sync>>),
 
     #[error("Undecipherable proto message")]
     ProtoError(#[from] prost::EncodeError),
@@ -72,6 +72,12 @@ pub enum ClientError {
 
 pub type ClientResult<T> = Result<T, ClientError>;
 
+impl From<Box<dyn std::error::Error + Send + Sync>> for ClientError {
+    fn from(value: Box<dyn std::error::Error + Send + Sync>) -> Self {
+        ClientError::ExternalError(Arc::new(value))
+    }
+}
+
 impl<'a, A> From<std::sync::PoisonError<std::sync::MutexGuard<'a, A>>> for ClientError {
     fn from(_: std::sync::PoisonError<std::sync::MutexGuard<'a, A>>) -> Self {
         ClientError::Internal("Lock Error".to_owned())

rust/perspective-client/src/rust/utils/mod.rs

@@ -69,6 +69,7 @@ ts-rs = { version = "10.0.0", features = [
     "serde-json-impl",
     "no-serde-warnings",
 ] }
+thiserror = "1.0.55"
 tracing = { version = ">=0.1.36" }
 tracing-subscriber = "0.3.15"
 console_error_panic_hook = "0.1.6"

rust/perspective-js/Cargo.toml

@@ -26,9 +26,9 @@ use wasm_bindgen::prelude::*;
 use wasm_bindgen_derive::TryFromJsValue;
 use wasm_bindgen_futures::{JsFuture, future_to_promise};
 
-use crate::TableDataExt;
 pub use crate::table::*;
 use crate::utils::{ApiError, ApiResult, JsValueSerdeExt, LocalPollLoop};
+use crate::{TableDataExt, apierror};
 
 #[wasm_bindgen]
 extern "C" {
@@ -72,7 +72,6 @@ impl ProxySession {
         Ok(())
     }
 
-
     pub async fn close(self) {
         self.0.close().await;
     }
@@ -221,14 +220,10 @@ impl Client {
 
     #[doc(hidden)]
     #[wasm_bindgen]
-    pub async fn handle_error(
-        &self,
-        error: Option<String>,
-        reconnect: Option<Function>,
-    ) -> ApiResult<()> {
+    pub async fn handle_error(&self, error: String, reconnect: Option<Function>) -> ApiResult<()> {
         self.client
             .handle_error(
-                error,
+                ClientError::Unknown(error),
                 reconnect.map(|reconnect| {
                     let reconnect =
                         JsReconnect::from(move |()| match reconnect.call0(&JsValue::UNDEFINED) {
@@ -262,7 +257,7 @@ impl Client {
     #[wasm_bindgen]
     pub async fn on_error(&self, callback: Function) -> ApiResult<u32> {
         let callback = JsReconnect::from(
-            move |(message, reconnect): (Option<String>, Option<ReconnectCallback>)| {
+            move |(message, reconnect): (ClientError, Option<ReconnectCallback>)| {
                 let cl: Closure<dyn Fn() -> js_sys::Promise> = Closure::new(move || {
                     let reconnect = reconnect.clone();
                     future_to_promise(async move {
@@ -276,7 +271,7 @@ impl Client {
 
                 if let Err(e) = callback.call2(
                     &JsValue::UNDEFINED,
-                    &JsValue::from(message),
+                    &JsValue::from(apierror!(ClientError(message))),
                     &cl.into_js_value(),
                 ) {
                     tracing::warn!("{:?}", e);

rust/perspective-js/src/rust/client.rs

@@ -16,6 +16,7 @@ use perspective_client::{ColumnType, TableData, TableReadFormat, UpdateData};
 use wasm_bindgen::convert::TryFromJsValue;
 use wasm_bindgen::prelude::*;
 
+use crate::apierror;
 use crate::utils::{ApiError, ApiResult, JsValueSerdeExt, ToApiError};
 pub use crate::view::*;
 
@@ -40,7 +41,6 @@ impl Vec<(String, ColumnType)> {
 #[ext]
 pub(crate) impl TableData {
     fn from_js_value(value: &JsValue, format: Option<TableReadFormat>) -> ApiResult<TableData> {
-        let err_fn = || JsValue::from(format!("Failed to construct Table {:?}", value));
         if let Some(result) = UpdateData::from_js_value_partial(value, format)? {
             Ok(result.into())
         } else if value.is_instance_of::<Object>() && Reflect::has(value, &"__get_model".into())? {
@@ -71,10 +71,10 @@ pub(crate) impl TableData {
                 let json = JSON::stringify(value)?.as_string().into_apierror()?;
                 Ok(UpdateData::JsonColumns(json).into())
             } else {
-                Err(err_fn().into())
+                Err(apierror!(TableError(value.clone())))
             }
         } else {
-            Err(err_fn().into())
+            Err(apierror!(TableError(value.clone())))
         }
     }
 }
@@ -85,9 +85,8 @@ pub(crate) impl UpdateData {
         value: &JsValue,
         format: Option<TableReadFormat>,
     ) -> ApiResult<Option<UpdateData>> {
-        let err_fn = || JsValue::from(format!("Failed to construct Table {:?}", value));
         if value.is_undefined() {
-            Err(err_fn().into())
+            Err(apierror!(TableError(value.clone())))
         } else if value.is_string() {
             match format {
                 None | Some(TableReadFormat::Csv) => {