-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 a handled
prop to ErrorBoundary
#14560
feat(react): Add a handled
prop to ErrorBoundary
#14560
Conversation
e2eb1fc
to
b837937
Compare
@Lms24 not sure what to do regarding the CI.
|
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.
Hey @HHK1 thanks for opening this PR! The proposed changes look good to me! I had some suggestions to streamline the implementation but looks fine! I'll start CI to check for the linting stuff but it most definitely will fail because there's still an .only
filter in the tests (we have a lint rule against accidentally merging this in).
I'd generally recommend to run yarn fix:biome
in the root directory of the repo to fix any formatting and biome lint issues.
I can take another look about remaining CI fails if things still fail after the changes.
packages/react/src/errorboundary.tsx
Outdated
const isHandled = this.props.handled === undefined ? !!this.props.fallback : this.props.handled; | ||
const eventId = captureReactException(error, errorInfo, { mechanism: { handled: isHandled } }); |
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.
a suggestion to make this a bit more concise and save some bytes of bundle size. I turned around the condition to reflect that the fallback is actually our fallback way of determining handled:
const isHandled = this.props.handled === undefined ? !!this.props.fallback : this.props.handled; | |
const eventId = captureReactException(error, errorInfo, { mechanism: { handled: isHandled } }); | |
const handled = this.props.handled != null ? this.props.handled : !!this.props.fallback; | |
const eventId = captureReactException(error, errorInfo, { mechanism: { handled } }); |
(!= null
can be used as a shorthand for !== undefined && !== 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.
Fixed in 11c408d
@@ -537,47 +537,47 @@ describe('ErrorBoundary', () => { | |||
expect(mockOnReset).toHaveBeenCalledTimes(1); | |||
expect(mockOnReset).toHaveBeenCalledWith(expect.any(Error), expect.any(String), expect.any(String)); | |||
}); | |||
it.only.each` |
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.
This we should most definitely remove :)
it.only.each` | |
it.each` |
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.
fixed in 9c1cf83
@Lms24 thanks for the review! I've pushed a couple fixups, once the conversations are resolved I'll squash them to have a single commit. I ran the formatting through the VSCode extension on the files I've touched, but I got the following kind of issues when running
The
It does seem like a local setup error. Rings any bell? Not a big deal but I was just curious as why it could fail. |
11c408d
to
c1e5bcc
Compare
@Lms24 just a soft ping if you could have a look 😇🙏 |
The previous behaviour was to rely on the presence of the `fallback` prop to decide if the error was considered handled or not. The new property lets the consumer explicitely choose what should the handled status be. If omitted, the old behaviour is still applied.
c1e5bcc
to
1a46add
Compare
@Lms24 👋 would it be possible to have a review on this please? |
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.
Apologies for the late review! This looks good to me now, thanks for contributing and for making the requested changes :)
Heads-up: I'll cherry pick this commit once it's merged onto our v8
branch to ensure we can include this in the next v8 release, probably sometime next week.
handled
prop to ErrorBoundaryhandled
prop to ErrorBoundary
The previous behaviour was to rely on the presence of the `fallback` prop to decide if the error was considered handled or not. The new property lets users explicitly choose what should the handled status be. If omitted, the old behaviour is still applied.
backport of #14560 --------- Co-authored-by: Henry Huck <[email protected]>
The previous behaviour was to rely on the presence of the
fallback
prop to decide if the error was considered handled or not. The new property lets the consumer explicitely choose what should the handled status be.If omitted, the old behaviour is still applied.
Before submitting a pull request, please take a look at our
Contributing guidelines and verify:
yarn lint
) & (yarn test
).-> I'm having some failures locally that seem unrelated to my changes. Waiting for a CI check to confirm