-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(react): Make fallback render types more accurate #7198
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
Conversation
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.
Can we type the state of this component more canonical, ie. coupling the existance of an error to the existence of the componentStack and eventId?
packages/react/src/errorboundary.tsx
Outdated
element = fallback({ | ||
error, | ||
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion | ||
componentStack: componentStack!, |
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.
Just to be clear (absolutely no expert here), we are 100% positive that when error
is set in the state, componentStack
and eventId
are also never null?
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.
Wait, nevermind, just read the related issue, seems good!
Great suggestion, pushed 6ade121 to address this. |
Awesome. Thanks! |
Replay SDK metrics 🚀Latest data for: 5359ba9
*) pp - percentage points - an absolute difference between two percentages.
Last updated: Thu, 16 Feb 2023 11:21:38 GMT |
fixes #4190
We set
error
,componentStack
, andeventId
at the same timesentry-javascript/packages/react/src/errorboundary.tsx
Line 106 in 2bdda84
This means that there is no way for
componentStack
andeventId
to benull
iferror
is defined. Making the TS types more accurate to reflect this.