- 
          
 - 
                Notifications
    
You must be signed in to change notification settings  - Fork 1.7k
 
          feat(react): Add a handled prop to ErrorBoundary
          #14560
        
          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
              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
| 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 <henryhuck@hotmail.fr>
The previous behaviour was to rely on the presence of the
fallbackprop 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