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.

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

feat(react): Add Error Boundary component #2647

Merged
merged 17 commits into from Jun 8, 2020

Conversation

AbhiPrasad
Copy link
Member

@AbhiPrasad AbhiPrasad commented Jun 5, 2020

Following up #2631

Motivation

This PR implements an Error Boundary React component. See https://reactjs.org/docs/error-boundaries.html for more details.

This is an important API for us to support, as error boundaries are the defacto way to catch and deal with errors in React.

Implementation

The implementation was inspired by the way we do error boundaries at Sentry as well as bvaughn's excellent error boundary implementation

The error boundary provides the following options:

type FallbackRender = (fallback: {
  error: Error | null;
  componentStack: string | null;
  resetError(): void;
}) => React.ReactNode;

export type ErrorBoundaryProps = {
  // if we should render Sentry's report dialog or not
  showDialog?: boolean;
  dialogOptions?: Sentry.ReportDialogOptions;
  // If render key changes and there is an error, the component is reset
  renderKey?: string | number;
  // We provide two ways to render a fallback component,
  // rendering just a component takes priority, but we
  // also support rendering using render props.
  fallback?: React.ReactNode | FallbackRender;
   // on error callback
  onError?(error: Error, componentStack: string): void;
  // callback onComponentDidMount
  onMount?(): void;
  // when `resetErrorBoundary` is called from `fallbackRender`
  onReset?(error: Error | null, componentStack: string | null): void;
  // callback onComponentWillUnmount
  onUnmount?(error: Error | null, componentStack: string | null): void;
};

We recommend using fallbackRender to render the fallback component, as it uses a render props approach. This passes resetErrorBoundary as render props, allowing the user to easily reset the error boundary.

The error boundary is available as a standalone component with

<ErrorBoundary>
  <Component />
</ErrorBoundary>

or as a HOC

withErrorBoundary(Component, errorBoundaryProps)

Testing

I switched to @testing-library/react, it's pretty good (and seems to be the standard way to do tests in React now). It's also 209.5 kb minified, while enzyme is 463.2 kb, and the yarn installs definitely add up.

Current test coverage

File % Stmts % Branch % Funcs % Lines Uncovered Line #s
All files 91.49 72.73 90 90.91
errorboundary.tsx 88.89 50 77.78 87.5 93,95,101,105,106
profiler.tsx 95 100 100 94.59 53,56

Future

After this gets merged in, we move on to creating a hooks based Profiler. I'm also looking to see how difficult it is to support react-router and similar.

@getsentry-bot
Copy link
Contributor

getsentry-bot commented Jun 5, 2020

Messages
📖

@sentry/browser bundle gzip'ed minified size: (ES5: 17.0732 kB) (ES6: 16.1367 kB)

📖 ✅ TSLint passed

Generated by 🚫 dangerJS against 5174791

Copy link
Member

@HazAT HazAT left a comment

Choose a reason for hiding this comment

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

Soooo good, so many tests ❤️

Small nits + don't forget Changelog

packages/react/src/errorboundary.tsx Outdated Show resolved Hide resolved
packages/react/src/errorboundary.tsx Outdated Show resolved Hide resolved
@AbhiPrasad
Copy link
Member Author

AbhiPrasad commented Jun 5, 2020

Update: Addressed review comments. Also added a renderKey prop, which will update the component if changed. This provides an easy way for the user to reset errors without having to rely on calling a reset function (for example, if the window.location changes, the component will reset) No longer doing this.

@AbhiPrasad AbhiPrasad force-pushed the abhi/feat/react-error-boundary branch from 424ff83 to b7d0a32 Compare June 5, 2020 13:30
Copy link
Member

@HazAT HazAT left a comment

Choose a reason for hiding this comment

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

🥇

@AbhiPrasad
Copy link
Member Author

Hmm so I'm using Sentry.captureException(error, { contexts: { react: { componentStack } } });, and though an error event is created in Sentry, the react context does not show up. Testing with demo here: https://github.com/AbhiPrasad/sentry-react-demo

Copy link
Member

@billyvg billyvg left a comment

Choose a reason for hiding this comment

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

LGTM

Are there plans to update our app usage of error boundaries?

packages/react/src/errorboundary.tsx Outdated Show resolved Hide resolved
packages/react/src/errorboundary.tsx Show resolved Hide resolved
@AbhiPrasad
Copy link
Member Author

@billyvg we will be updating the usage of Profiler and ErrorBoundary in getsentry/sentry to test this before we GA the package.

@AbhiPrasad AbhiPrasad force-pushed the abhi/feat/react-error-boundary branch 4 times, most recently from 52ef24a to 110e338 Compare June 8, 2020 18:47
@AbhiPrasad AbhiPrasad force-pushed the abhi/feat/react-error-boundary branch from 110e338 to 5174791 Compare June 8, 2020 19:14
@AbhiPrasad AbhiPrasad merged commit 47b654c into master Jun 8, 2020
@AbhiPrasad AbhiPrasad deleted the abhi/feat/react-error-boundary branch June 8, 2020 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants