-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
fix(react): Handle case where error.cause already defined #7557
Changes from all commits
e37e6e7
d0c99b2
c550c7f
54aaa3e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -66,6 +66,25 @@ const INITIAL_STATE = { | |
eventId: null, | ||
}; | ||
|
||
function setCause(error: Error & { cause?: Error }, cause: Error): void { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks weird to me:
I have no idea why you try to put this error as a cause deeply in the error stack, but that looks really suspicious to me. errorBoundaryError should rather be the parent top-level error, not the most deeply nested cause. |
||
const seenErrors = new WeakMap<Error, boolean>(); | ||
|
||
function recurse(error: Error & { cause?: Error }, cause: Error): void { | ||
// If we've already seen the error, there is a recursive loop somewhere in the error's | ||
// cause chain. Let's just bail out then to prevent a stack overflow. | ||
if (seenErrors.has(error)) { | ||
return; | ||
} | ||
if (error.cause) { | ||
seenErrors.set(error, true); | ||
return recurse(error.cause, cause); | ||
} | ||
error.cause = cause; | ||
} | ||
|
||
recurse(error, cause); | ||
} | ||
|
||
/** | ||
* A ErrorBoundary component that logs errors to Sentry. Requires React >= 16. | ||
* NOTE: If you are a Sentry user, and you are seeing this stack frame, it means the | ||
|
@@ -93,7 +112,7 @@ class ErrorBoundary extends React.Component<ErrorBoundaryProps, ErrorBoundarySta | |
errorBoundaryError.stack = componentStack; | ||
|
||
// Using the `LinkedErrors` integration to link the errors together. | ||
error.cause = errorBoundaryError; | ||
setCause(error, errorBoundaryError); | ||
} | ||
|
||
if (beforeCapture) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,14 +3,8 @@ import { fireEvent, render, screen } from '@testing-library/react'; | |
import * as React from 'react'; | ||
import { useState } from 'react'; | ||
|
||
import type { | ||
ErrorBoundaryProps} from '../src/errorboundary'; | ||
import { | ||
ErrorBoundary, | ||
isAtLeastReact17, | ||
UNKNOWN_COMPONENT, | ||
withErrorBoundary, | ||
} from '../src/errorboundary'; | ||
import type { ErrorBoundaryProps } from '../src/errorboundary'; | ||
import { ErrorBoundary, isAtLeastReact17, UNKNOWN_COMPONENT, withErrorBoundary } from '../src/errorboundary'; | ||
|
||
const mockCaptureException = jest.fn(); | ||
const mockShowReportDialog = jest.fn(); | ||
|
@@ -39,7 +33,13 @@ function Bam(): JSX.Element { | |
return <Boo title={title} />; | ||
} | ||
|
||
const TestApp: React.FC<ErrorBoundaryProps> = ({ children, ...props }) => { | ||
interface TestAppProps extends ErrorBoundaryProps { | ||
errorComp?: JSX.Element; | ||
} | ||
|
||
const TestApp: React.FC<TestAppProps> = ({ children, errorComp, ...props }) => { | ||
// eslint-disable-next-line no-param-reassign | ||
const customErrorComp = errorComp || <Bam />; | ||
const [isError, setError] = React.useState(false); | ||
return ( | ||
<ErrorBoundary | ||
|
@@ -51,7 +51,7 @@ const TestApp: React.FC<ErrorBoundaryProps> = ({ children, ...props }) => { | |
} | ||
}} | ||
> | ||
{isError ? <Bam /> : children} | ||
{isError ? customErrorComp : children} | ||
<button | ||
data-testid="errorBtn" | ||
onClick={() => { | ||
|
@@ -299,6 +299,87 @@ describe('ErrorBoundary', () => { | |
expect(error.cause).not.toBeDefined(); | ||
}); | ||
|
||
it('handles when `error.cause` is nested', () => { | ||
const mockOnError = jest.fn(); | ||
|
||
function CustomBam(): JSX.Element { | ||
const firstError = new Error('bam'); | ||
const secondError = new Error('bam2'); | ||
const thirdError = new Error('bam3'); | ||
// @ts-ignore Need to set cause on error | ||
secondError.cause = firstError; | ||
// @ts-ignore Need to set cause on error | ||
thirdError.cause = secondError; | ||
throw thirdError; | ||
} | ||
|
||
render( | ||
<TestApp fallback={<p>You have hit an error</p>} onError={mockOnError} errorComp={<CustomBam />}> | ||
<h1>children</h1> | ||
</TestApp>, | ||
); | ||
|
||
expect(mockOnError).toHaveBeenCalledTimes(0); | ||
expect(mockCaptureException).toHaveBeenCalledTimes(0); | ||
|
||
const btn = screen.getByTestId('errorBtn'); | ||
fireEvent.click(btn); | ||
|
||
expect(mockCaptureException).toHaveBeenCalledTimes(1); | ||
expect(mockCaptureException).toHaveBeenLastCalledWith(expect.any(Error), { | ||
contexts: { react: { componentStack: expect.any(String) } }, | ||
}); | ||
|
||
expect(mockOnError.mock.calls[0][0]).toEqual(mockCaptureException.mock.calls[0][0]); | ||
|
||
const thirdError = mockCaptureException.mock.calls[0][0]; | ||
const secondError = thirdError.cause; | ||
const firstError = secondError.cause; | ||
const cause = firstError.cause; | ||
expect(cause.stack).toEqual(mockCaptureException.mock.calls[0][1].contexts.react.componentStack); | ||
expect(cause.name).toContain('React ErrorBoundary'); | ||
expect(cause.message).toEqual(thirdError.message); | ||
Comment on lines
+338
to
+341
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The The root cause (deeply nested cause) is something low level like "undefined is not a function", not "Sentry Error Boundary captured an error" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We patch the error boundary error at the end so that it does not affect issue grouping - since there are some flaws to the synthetic stacktrace generated by the error boundary error (since This is the most ergonomic way to get the error into sentry (and sourcemapped so you can see actual filenames/source context) without affecting existing/new grouping configurations. You're right that this is conceptually wrong - but IMO the drawback is acceptable to make sure users get the best experience possible while staying within the limitations of both the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Re: the imperfections in component stacks, see: facebook/react#18561
Relying on the original error is the best way we can get the best issue grouping for our users. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for explaining. Not sure exactly what's going on here but if you know what you are doing 👍 . Will test this on Sentry soon and see if it works as I expect it to. |
||
}); | ||
|
||
it('handles when `error.cause` is recursive', () => { | ||
const mockOnError = jest.fn(); | ||
|
||
function CustomBam(): JSX.Element { | ||
const firstError = new Error('bam'); | ||
const secondError = new Error('bam2'); | ||
// @ts-ignore Need to set cause on error | ||
firstError.cause = secondError; | ||
// @ts-ignore Need to set cause on error | ||
secondError.cause = firstError; | ||
throw firstError; | ||
} | ||
|
||
render( | ||
<TestApp fallback={<p>You have hit an error</p>} onError={mockOnError} errorComp={<CustomBam />}> | ||
<h1>children</h1> | ||
</TestApp>, | ||
); | ||
|
||
expect(mockOnError).toHaveBeenCalledTimes(0); | ||
expect(mockCaptureException).toHaveBeenCalledTimes(0); | ||
|
||
const btn = screen.getByTestId('errorBtn'); | ||
fireEvent.click(btn); | ||
|
||
expect(mockCaptureException).toHaveBeenCalledTimes(1); | ||
expect(mockCaptureException).toHaveBeenLastCalledWith(expect.any(Error), { | ||
contexts: { react: { componentStack: expect.any(String) } }, | ||
}); | ||
|
||
expect(mockOnError.mock.calls[0][0]).toEqual(mockCaptureException.mock.calls[0][0]); | ||
|
||
const error = mockCaptureException.mock.calls[0][0]; | ||
const cause = error.cause; | ||
// We need to make sure that recursive error.cause does not cause infinite loop | ||
expect(cause.stack).not.toEqual(mockCaptureException.mock.calls[0][1].contexts.react.componentStack); | ||
expect(cause.name).not.toContain('React ErrorBoundary'); | ||
}); | ||
|
||
it('calls `beforeCapture()` when an error occurs', () => { | ||
const mockBeforeCapture = jest.fn(); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unlikely that it is gonna happen but this could infinitely recurse. Are we worried about that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are right, let me be a little more defensive and track the visited nodes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done with d0c99b2