Skip to content
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

componentDidCatch doesn't catch some invariants #13820

Closed
mlityn opened this issue Oct 10, 2018 · 6 comments · Fixed by #14104
Closed

componentDidCatch doesn't catch some invariants #13820

mlityn opened this issue Oct 10, 2018 · 6 comments · Fixed by #14104

Comments

@mlityn
Copy link

mlityn commented Oct 10, 2018

Do you want to request a feature or report a bug?

A bug.

What is the current behavior?

componentDidCatch isn't called when a child component renders a void element containing another element. The app just crashes.
If it contains just text, the same error ("...is a void element tag and must neither have children nor use dangerouslySetInnerHTML") appears, but this time with componentDidCatch properly called.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem. Your bug will get fixed much faster if we can run your code and it doesn't have dependencies other than React. Paste the link to your JSFiddle (https://jsfiddle.net/Luktwrdm/) or CodeSandbox (https://codesandbox.io/s/new) example below:

https://codesandbox.io/s/0042z5wrlp
The behavior is different when you change Comp1 with Comp2.

What is the expected behavior?

componentDidCatch should be called in both cases.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

16.5.2.
In 16.4.2 it worked correctly.

@romainlq
Copy link

Hi, I would like to give a try to this issue !
Unfortunately, I'm used to React but not to the project itself, any idea where I should start looking ?

@krzysztofla
Copy link

I also would like to know where to take a look :)

@romainlq
Copy link

romainlq commented Oct 16, 2018

Humm, that is weird. When I tried the codesandbox example on a Windows a few hours earlier, I was able to reproduce the issue.
I just retried on my Mac and this time I have the same behavior for both components...
(I tried both on Chrome, v.70.0.3538.67 for my Mac)
Gonna keep looking into it
(By the way, the fix should be in the ReactDOMComponent-test.js file if I'm correct)

Edit : just tested back on my Windows PC, same behavior for both Components... 🤔

@GarethSmall
Copy link
Contributor

GarethSmall commented Oct 18, 2018

This may also be expected behavior, it is in the error boundary.

https://reactjs.org/docs/error-boundaries.html

screen shot 2018-10-18 at 4 43 55 pm

Here's the same example where comp2 is pass in as a child of the ErrorBoundary.

https://codesandbox.io/s/pj6oz1xk60

However with that being said, it is odd that one error is caught while the other is not.

@mlityn
Copy link
Author

mlityn commented Oct 26, 2018

https://codesandbox.io/s/pj6oz1xk60

You used react 16.4. As i wrote, the problem appeared in 16.5 (and it's still there in 16.6). Moreover, you should put this.props.children instead of Comp1 in line 19 of your example. When you do this, the behavior will be exactly as described in the first post.

@blixt
Copy link

blixt commented Oct 31, 2018

I've reproduced this problem separately: https://codesandbox.io/s/8npm2jq05l

Here are my findings:

  • The boundary only fails for specific invariant errors (in this case the "input cannot contain children" one)
  • The boundary works correctly for other invariant errors (such as returning undefined)
  • The boundary works in 16.4.2 (it is able to catch the error)
  • The boundary does not work in 16.5.0, 16.5.1, 16.5.2, 16.6.0

See the comments in the code for a few different ways to cause the error boundary to successfully trigger.

@gaearon gaearon changed the title componentDidCatch - a void element componentDidCatch doesn't catch some invariants Nov 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants