fix: fix #189 remove the redundant uncaught error and error stack trace from the console

[benhuangbmj]

What: Fixing #189

Why: By removing the redundant message in the console, it will improve developer experience and reduce the chance of causing confusion.

How: I add a isShowBoundary property to the error prop in the state encapsulated in useErrorBoundary.ts. Then in ErrorBoundary.ts, I add an event listener to the window object listening to the "error" event. If the isShowBoundary flag is true, then the default event will be prevented, and the console.error method will be overridden, either. In the componentDidCatch method, the original console.error method will be recovered.

Checklist:

• Documentation
• Tests
• Ready to be merged
  

Since I needed to put an additional property to an object (the error prop in the state), I did need to modify the type of the original object. I tried my best to imitate the style of the code. Please feel free to comment on the style. Thanks.

[benhuangbmj]

@bvaughn Thank you for the very thoughtful comments. I will need to take some time to digest them,

[benhuangbmj]

I add a new prop option suppressLogging to ErrorBoundary, so that the suppress-logging behavior becomes optional. In order for this to work, I add some condition-trigger logic to shouldComponentUpdate and componentDidCatch. It seems like the same error would be logged twice. If error._suppressLogging is true, the first logging would be intercepted anyway, and the second logging relies on the suppressLogging prop of ErrorBoundary.

[bvaughn]

I'm not sure I understand what the new prop is meant to do.

[bvaughn]

Seems like this intentionally breaks the generics type for the hook. I guess you had to do this, to support the non-object case in suppressLoggingForError but if that's the case we should either remove the generic parameter from suppressLoggingForError (since it's not a guarantee) or maybe change it to require extending Error (so then the non-object case in suppressLoggingForError would be unsupported from a types perspective at least)?

function suppressLoggingForError<ErrorType extends Error>(error: ErrorType): StateError<ErrorType> {

[benhuangbmj]

I am sorry, but I don't quite get this one. Why removing the generic parameter from suppressLoggingForError would be of help? I noticed that in the generic parameter of useErrorBoundary, <TError = any>, so I assume TError can be very flexible yet maintaining it can assure some basic type check. Please shed more light on this.

[benhuangbmj]

Upon your suggestion, I wrap the suppress-logging trigger in the showBoundary function utilizing the ErrorBoundaryContext. Here is the workflow:

flowchart TD
showBoundary1 -- error --> window -- log --> console
showBoundary2 -- error --> window -- suppress --> console
showBoundary2 -- addEventListener: handleSuppressLogging --> window
componentDidCatch2 -- removeEventListener: handleSuppressLogging --> window
componentDidCatch1 -- removeEventListener: handleSuppressLogging --> window
subgraph Outter ErrorBoundary
handleSuppressLogging1[handleSuppressLogging] --> componentDidCatch1[componentDidCatch]
handleSuppressLogging1 -- save to --> context1
prop1[suppressLogging = false] -- save to --> context1[Outter Context]
context1 -- pass to --> showBoundary1[showBoundary]
subgraph Outter Child 
showBoundary1[showBoundary]
end
subgraph Inner ErrorBoundary
handleSuppressLogging2[handleSuppressLogging] --> componentDidCatch2[componentDidCatch]
handleSuppressLogging2 -- save to --> context2
prop2[suppressLogging = true] -- save to --> context2[Inner Context]
context2 -- pass to --> showBoundary2
subgraph Inner Child
showBoundary2[showBoundary]
end
end
end

Loading

Furthermore, I postpone the addition of the window event listener util it's necessary, and remove it as soon as the error is caught by the nearest error boundary. Please let me know what you think about this design. Thanks.

P.S. I did learn a whole lot through this process and from your advice. Thanks again.

[bvaughn]

This got lost in a previous round of discussion, but the default and else cases here potentially break the generic signature above.

Let's say we had a custom error type (e.g. class MyCustomError extends Error) that had some additional information in it. Let's say error handling code special handled this custom error type (e.g. error instanceof MyCustomError). We'd break that logic in these two cases because we instantiate a vanilla Error.

This might be an unlikely case but I think it's probably still not good for us to have a misleading generic type. That being said, I don't think this generic type is actually really doing anything that useful and we can just remove it entirely.

As a small efficiency nit too, I think we could lift the suppressLoggingForError method to the module level.

So...

diff --git a/src/useErrorBoundary.ts b/src/useErrorBoundary.ts
index d215e58..30bec77 100644
--- a/src/useErrorBoundary.ts
+++ b/src/useErrorBoundary.ts
@@ -1,55 +1,25 @@
 import { useContext, useMemo, useState } from "react";
 import { assertErrorBoundaryContext } from "./assertErrorBoundaryContext";
-import { ErrorBoundaryContext } from "./ErrorBoundaryContext";
+import {
+  ErrorBoundaryContext,
+  ErrorBoundaryContextType,
+} from "./ErrorBoundaryContext";
 
-type StateError<TError> =
-  | (TError & { _suppressLogging: boolean })
-  | (Error & { _suppressLogging: boolean });
-
-type UseErrorBoundaryState<TError> =
-  | { error: StateError<TError>; hasError: true }
+type UseErrorBoundaryState =
+  | { error: unknown; hasError: true }
   | { error: null; hasError: false };
 
-export type UseErrorBoundaryApi<TError> = {
+export type UseErrorBoundaryApi = {
   resetBoundary: () => void;
-  showBoundary: (error: TError) => void;
+  showBoundary: (error: unknown) => void;
 };
 
-export function useErrorBoundary<TError = any>(): UseErrorBoundaryApi<TError> {
+export function useErrorBoundary(): UseErrorBoundaryApi {
   const context = useContext(ErrorBoundaryContext);
 
   assertErrorBoundaryContext(context);
 
-  const suppressLoggingForError = function <TError>(
-    error: TError
-  ): StateError<TError> {
-    const suppressLogging = {
-      _suppressLogging: context.suppressLogging as boolean,
-    };
-    if (error != null) {
-      switch (typeof error) {
-        case "object": {
-          if (Object.isExtensible(error)) {
-            (error as StateError<TError>)._suppressLogging =
-              context.suppressLogging;
-            return error as StateError<TError>;
-          } else {
-            return Object.assign(
-              Object.create(error as object),
-              suppressLogging
-            );
-          }
-        }
-        default: {
-          return Object.assign(Error(error as string), suppressLogging);
-        }
-      }
-    } else {
-      return Object.assign(Error(undefined), suppressLogging);
-    }
-  };
-
-  const [state, setState] = useState<UseErrorBoundaryState<TError>>({
+  const [state, setState] = useState<UseErrorBoundaryState>({
     error: null,
     hasError: false,
   });
@@ -60,12 +30,12 @@ export function useErrorBoundary<TError = any>(): UseErrorBoundaryApi<TError> {
         context.resetErrorBoundary();
         setState({ error: null, hasError: false });
       },
-      showBoundary: (error: TError) => {
+      showBoundary: (error: unknown) => {
         if (context.suppressLogging) {
           window.addEventListener("error", context.handleSuppressLogging);
         }
         setState({
-          error: suppressLoggingForError(error),
+          error: suppressLoggingForError(error, context),
           hasError: true,
         });
       },
@@ -79,3 +49,30 @@ export function useErrorBoundary<TError = any>(): UseErrorBoundaryApi<TError> {
 
   return memoized;
 }
+
+function suppressLoggingForError(
+  error: unknown,
+  context: ErrorBoundaryContextType
+): unknown {
+  const suppressLogging = {
+    _suppressLogging: context.suppressLogging as boolean,
+  };
+
+  if (error != null) {
+    switch (typeof error) {
+      case "object": {
+        if (Object.isExtensible(error)) {
+          (error as any)._suppressLogging = context.suppressLogging;
+          return error;
+        } else {
+          return Object.assign(Object.create(error as object), suppressLogging);
+        }
+      }
+      default: {
+        return Object.assign(Error(error as string), suppressLogging);
+      }
+    }
+  } else {
+    return Object.assign(Error(undefined), suppressLogging);
+  }
+}

[bvaughn]

We can't do this. It would break uncaught exception behavior (see facebook/react#28515) and cause a memory leak (since window would have a reference to this error boundary instance and anything that it references as well).

[benhuangbmj]

Moving forward, I guess I would dive deeper into the React Internals and try to understand how the error handling works in there. Thanks.