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 support for React 17 Error Boundaries #3532

Merged
merged 8 commits into from May 14, 2021

Conversation

AbhiPrasad
Copy link
Member

@AbhiPrasad AbhiPrasad commented May 11, 2021

React 17 introduces better stack traces from error boundaries. This PR updates the @sentry/react error boundary to take advantage of that better component stack trace.

The current approach is to create a Sentry event with 2 exceptions. The first is the new React component stack trace offered by the error boundary, and the second is stack trace from standard JS error thrown. So it'll look something like this:

image

A look inside one of the frames:

image

I'm willing to change this approach if y'all have better ideas, so please drop feedback as necessary.

Things I still have to check:

  1. Source Maps ✅
  • Works, but for some reason there they are off by a line sometimes? This actually might be a React issue idk This happens due to how the componentStack is built by React. A consequence of this is that if you have an empty class component like:
class Wow extends React.Component {
  render() {
    return <SomeElement />
  }
}

the frame that is built will not be useful (won't point to the correct lines). Not sure how to fix this (or if it's worth the bandwidth)

  1. React Suspense fallback + React Context support ✅
  • Doesn't break the stack traces, but Suspense and Context components dont show up on the stack trace
Boo@http://localhost:3000/static/js/main.chunk.js:16750:13
Bam@http://localhost:3000/static/js/main.chunk.js:16761:73
Suspense
ErrorBoundary@http://localhost:3000/static/js/main.chunk.js:9232:43
TestApp@http://localhost:3000/static/js/main.chunk.js:16776:17
header
div
  1. Is this the approach we want to take ✅
  • I think constructing the events this way is fine, and I've adjusted the tests to account for it
  1. Do people want to have both stack traces? ✅
  • I think yes, always good to add more info, but it'll be interesting to get feedback around this.
  1. How do I test with React 16 and 17? Using package aliasing doesn't work due to React's JSX transform shenanigans. I could do a bit more involved integration test, but that seems like overkill. ❌
  • Ok update: it's hard :(

Before submitting a pull request, please take a look at our
Contributing guidelines and verify:

  • If you've added code that should be tested, please add tests.
  • Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

@github-actions
Copy link
Contributor

github-actions bot commented May 11, 2021

size-limit report

Path Size
@sentry/browser - CDN Bundle (gzipped) 20.64 KB (0%)
@sentry/browser - Webpack 21.52 KB (0%)
@sentry/react - Webpack 21.55 KB (0%)
@sentry/browser + @sentry/tracing - CDN Bundle (gzipped) 28.02 KB (0%)

@AbhiPrasad AbhiPrasad marked this pull request as ready for review May 12, 2021 17:15
@kamilogorek
Copy link
Contributor

the frame that is built will not be useful (won't point to the correct lines). Not sure how to fix this (or if it's worth the bandwidth)

Do we need that frame, or can it be dropped? If it can, then we have a mechanism for this in place which we already use for react (note that framesToPop is taken from the react codebase itself, the used to use it, not sure if they still do) - https://github.com/getsentry/sentry-javascript/blob/master/packages/browser/src/tracekit.ts#L63-L67

Is this the approach we want to take ✅

I think it's fine to ship it and gather feedback, looks like a fine approach for me personally.

How do I test with React 16 and 17?

Isn't v17 just an extension of v16? All tests written for v17 should also cover v16 in most of cases, so we shouldn't need to test both versions extensively.

Copy link
Contributor

@kamilogorek kamilogorek left a comment

Choose a reason for hiding this comment

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

Just a small nitpick, feel free to ignore if it's not actionable.

packages/react/src/errorboundary.tsx Outdated Show resolved Hide resolved
@AbhiPrasad AbhiPrasad enabled auto-merge (squash) May 14, 2021 12:17
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

2 participants