Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 7 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1cb275b
fix: fix #189 remove the redundant uncaught error and error stack tra…
benhuangbmj Aug 5, 2024
08bb7fc
change: wrap the event listener attaching in the lifecycle methods
benhuangbmj Aug 14, 2024
bcc460f
feat: add suppressLogging to the prop type
benhuangbmj Aug 14, 2024
195a724
refactor: rename isShowBoundary to _suppressLogging
benhuangbmj Aug 14, 2024
a14b929
change: add the suppressLogging prop and the handleSuppressLogging fu…
benhuangbmj Aug 25, 2024
0e6bd3b
change: remove the suppress-logging logic from ErrorBoundary.ts
benhuangbmj Aug 25, 2024
abca669
change: wrap the suppress-logging logic in the showBoundary function
benhuangbmj Aug 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions src/ErrorBoundary.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ const initialState: ErrorBoundaryState = {
error: null,
};

function handleSuppressLogging(event: ErrorEvent) {
if (event.error._suppressLogging) event.preventDefault();
}

export class ErrorBoundary extends Component<
ErrorBoundaryProps,
ErrorBoundaryState
Expand All @@ -29,6 +33,10 @@ export class ErrorBoundary extends Component<
this.state = initialState;
}

static defaultProps = {
suppressLogging: false,
bvaughn marked this conversation as resolved.
Show resolved Hide resolved
};

static getDerivedStateFromError(error: Error) {
return { didCatch: true, error };
}
Expand All @@ -46,8 +54,14 @@ export class ErrorBoundary extends Component<
}
}

componentWillUnmount() {
window.removeEventListener("error", handleSuppressLogging);
}

componentDidCatch(error: Error, info: ErrorInfo) {
this.props.onError?.(error, info);
window.addEventListener("error", () => {}); // Some test cases will fail without this line, somehow.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

window.removeEventListener("error", handleSuppressLogging);
}

componentDidUpdate(
Expand Down Expand Up @@ -114,6 +128,8 @@ export class ErrorBoundary extends Component<
didCatch,
error,
resetErrorBoundary: this.resetErrorBoundary,
suppressLogging: this.props.suppressLogging as boolean,
handleSuppressLogging,
},
},
childToRender
Expand Down
2 changes: 2 additions & 0 deletions src/ErrorBoundaryContext.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ export type ErrorBoundaryContextType = {
didCatch: boolean;
error: any;
resetErrorBoundary: (...args: any[]) => void;
suppressLogging: boolean;
handleSuppressLogging: (event: ErrorEvent) => void;
};

export const ErrorBoundaryContext =
Expand Down
4 changes: 3 additions & 1 deletion src/assertErrorBoundaryContext.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@ export function assertErrorBoundaryContext(
if (
value == null ||
typeof value.didCatch !== "boolean" ||
typeof value.resetErrorBoundary !== "function"
typeof value.resetErrorBoundary !== "function" ||
typeof value.suppressLogging !== "boolean" ||
typeof value.handleSuppressLogging !== "function"
) {
throw new Error("ErrorBoundaryContext not found");
}
Expand Down
1 change: 1 addition & 0 deletions src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ type ErrorBoundarySharedProps = PropsWithChildren<{
| { reason: "keys"; prev: any[] | undefined; next: any[] | undefined }
) => void;
resetKeys?: any[];
suppressLogging?: boolean;
}>;

export type ErrorBoundaryPropsWithComponent = ErrorBoundarySharedProps & {
Expand Down
45 changes: 41 additions & 4 deletions src/useErrorBoundary.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,12 @@ import { useContext, useMemo, useState } from "react";
import { assertErrorBoundaryContext } from "./assertErrorBoundaryContext";
import { ErrorBoundaryContext } from "./ErrorBoundaryContext";

type StateError<TError> =
| (TError & { _suppressLogging: boolean })
| (Error & { _suppressLogging: boolean });

type UseErrorBoundaryState<TError> =
| { error: TError; hasError: true }
| { error: StateError<TError>; hasError: true }
| { error: null; hasError: false };

export type UseErrorBoundaryApi<TError> = {
Expand All @@ -16,6 +20,35 @@ export function useErrorBoundary<TError = any>(): UseErrorBoundaryApi<TError> {

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);
}
};
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
+  }
+}


const [state, setState] = useState<UseErrorBoundaryState<TError>>({
error: null,
hasError: false,
Expand All @@ -27,11 +60,15 @@ export function useErrorBoundary<TError = any>(): UseErrorBoundaryApi<TError> {
context.resetErrorBoundary();
setState({ error: null, hasError: false });
},
showBoundary: (error: TError) =>
showBoundary: (error: TError) => {
if (context.suppressLogging) {
window.addEventListener("error", context.handleSuppressLogging);
}
setState({
error,
error: suppressLoggingForError(error),
hasError: true,
}),
});
},
}),
[context.resetErrorBoundary]
);
Expand Down