fix(react): Handle case where error.cause already defined #7557
Conversation
AbhiPrasad
requested review from
mydea and
lforst
and removed request for
a team
size-limit report 📦
|
| @@ -66,6 +66,13 @@ const INITIAL_STATE = { | |||
| eventId: null, | |||
| }; | |||
|
|
|||
| function setCause(error: Error & { cause?: Error }, cause: Error): void { | |||
There was a problem hiding this comment.
Unlikely that it is gonna happen but this could infinitely recurse. Are we worried about that?
There was a problem hiding this comment.
you are right, let me be a little more defensive and track the visited nodes.
packages/react/src/errorboundary.tsx
Outdated
| @@ -66,6 +66,25 @@ const INITIAL_STATE = { | |||
| eventId: null, | |||
| }; | |||
|
|
|||
| function setCause(error: Error & { cause?: Error }, cause: Error): void { | |||
| const seenErrors = new Map<Error, boolean>(); | |||
There was a problem hiding this comment.
Should we make this a WeakMap to avoid GC issues? Probably doesn't matter but doesn't hurt I guess..?
AbhiPrasad
changed the title
| @@ -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.
This looks weird to me:
- fn signature:
setCause(error, cause) - usage:
setCause(error, errorBoundaryError);
errorBoundaryError is NOT the cause of the error being captured by the boundary. errorBoundaryError is an artificial error that you created at the top of the tree.
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 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); |
There was a problem hiding this comment.
The errorBoundaryError shouldn't be deeply nested, it should be the parent instead. The fact that an error has been captured by the boundary is not the "root cause" of the problem, it's the consequence.
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.
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 componenStack is generated under the hood by throwing errors and walking up the component tree, it can sometimes point to the wrong line of code).
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 componentStack and the sentry event schema.
There was a problem hiding this comment.
Re: the imperfections in component stacks, see: facebook/react#18561
The idea is to throw as early as possible. Ideally it's the first line such as the built-in super call of a class or the destructuring of props in the argument position. It could however be slightly later which might be slightly confusing to see the line of the first hook for example. Source code aware tooling could adjust this number to the previous function definition before the offset. If it doesn't throw at all such as if no hooks and no props are used, then we can't figure out the line number.
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.
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.
ref: #1401 (comment)
If
error.causeis already defined, attempt to walk down the error chain to set theReactErrorBoundaryerror.