Skip to content

Bug: ComponentDidCatch will not catch the same child component twice #20631

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

Closed
overly-engineered opened this issue Jan 20, 2021 · 6 comments
Closed
Labels
Resolution: Stale Automatically closed due to inactivity Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug

Comments

@overly-engineered
Copy link

overly-engineered commented Jan 20, 2021

We have a container element which is used to render children of different types. These children are specified by the user and can contain errors or bugs, we, therefore, have a fallback hierarchy which will fall back to the next child in a specified list and attempts to render that. This works fine as a solution until a user attempts to use the same component twice in a different configuration the second time which also fails. The parent component cannot catch the same child erroring twice but will catch multiple different children failing one after the other.

React version: At least v16.14.0+,

Reproduced on v16.14.0 and v17.0.1

Steps To Reproduce

  1. Main renders the child component.
  2. Child component throws an error.
  3. Main catches that error and attempts to render child component again
  4. Child throws another error.
  5. Main fails to catch the error a second time.

Link to code example:
React 17 - https://codesandbox.io/s/busy-waterfall-1u0fc?file=/src/App.js
React 18 - https://codesandbox.io/s/red-sunset-wv98ln?file=/package.json

The current behavior

Parent component is only capable of catching the same child once.

The expected behavior

Parent component would catch child component however many times it throws an error occurs.

I'm aware that we may well not be using the componentDidCatch method properly in this instance, but it worked as a really easy way to provide this fallback behaviour.

@overly-engineered overly-engineered added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Jan 20, 2021
@eps1lon
Copy link
Collaborator

eps1lon commented Jan 20, 2021

Thanks for the report.

A couple of notes about your reprop:
key={this.makeid(5)} means that on every render you're creating a new key. render needs to be side-effect free
Incidentally, this issue leads to will lead to Parent catching twice because you're rendering inside StrictMode which means React calls your render twice, you mount Child twice (because the key changes between render), call componentDidMount twice therefore throw two errors.

which will fall back to the next child in a specified list and attempts to render that

I'm not seeing that in your repro. I can only imagine that you have something like <Parent fallbacks={[<A />, <A trySomethingDifferent={true} />]} />. If React re-renders with <A trySomethingDifferent={true} /> it will compare it against the previous configuration and note that the types are equal (A === A). It will then only update the previous instance of A. However, if A only threw in componentDidMount <A trySomethingDifferent={true} /> will not throw again because it's already mounted.

If you want to make sure that every fallback is mounted, you can wrap it in a Fragment and change the key. For example, <React.Fragment key={this.state.fallbackCount}>{this.props.fallbacks[this.state.fallbackCount]}</React.Fragment> Strike-through text doesn't seem to work (https://codesandbox.io/s/amazing-architecture-07zy6?file=/src/App.js). I suspect the behavior is not supported considering

In the event of an error, you can render a fallback UI with componentDidCatch() by calling setState, but this will be deprecated in a future release. Use static getDerivedStateFromError() to handle fallback rendering instead.

-- https://reactjs.org/docs/react-component.html#componentdidcatch

Access to the current state in getDerivedStateFromError is already requested in #13954

@overly-engineered
Copy link
Author

Hi, @eps1lon thanks for getting back to me so quickly.

Ah yes, the addition of the key was an attempt to ensure that it would be considered a new component each time. I've removed it from the example.

Yes sorry, I aimed for the example to be as simple as possible, I've updated it now to include an array of children which are then iterated over when rendered. It does seem that regardless of where in the lifecycle the error is thrown that it is only ever caught once, without making a more complex example it's difficult to show the behaviour but it is seen clearly if the error is thrown from the render method, does that still mean that it is the type comparison that is the cause. For reference, the patch we have implemented temporarily is to unique the child array before we attempt to render so we avoid this issue.

I did wonder if this behaviour was unsupported if so that is no problem at all, we can investigate into like getDerivedStateFromError.

Copy link

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

@github-actions github-actions bot added the Resolution: Stale Automatically closed due to inactivity label Apr 10, 2024
@overly-engineered
Copy link
Author

Bump, still seems to be an error in React 18.2.0

@github-actions github-actions bot removed the Resolution: Stale Automatically closed due to inactivity label Apr 11, 2024
Copy link

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

@github-actions github-actions bot added the Resolution: Stale Automatically closed due to inactivity label Jul 10, 2024
Copy link

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please create a new issue with up-to-date information. Thank you!

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Stale Automatically closed due to inactivity Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug
Projects
None yet
Development

No branches or pull requests

2 participants