Call componentDidLeave after setState in ReactTransitionGroup #7084
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
This approach makes one of the test cases fail so I'm using isMounted(). Is that because setState can be synchronous? |
|
Are you saying that in a child's |
|
Yes, I know and I first tried to solve the problem in a different way if you look my first commit, but unfortunately it fails for one test case because you can not be sure anymore that componentWillUnmount will be called after componentDidLeave (though I don't think that should be considered a problem). isMounted() is considered an anti-pattern in the documentation, but it is already used in some test cases and in ReactCSSTransitionGroup. |
|
Not in the child. I imagined this scenario: you use the componentDidLeave hook from the children to tell the parent that a child left. When the last child of your group left and your group is empty maybe you would like to remove the group from the DOM (the outer div/span/somethig else that wraps the children). So you call in your parent some state update that would do that for you. This would trigger the warning. |
These are likely legacy implementations (The
I'm not extremely familiar with |
It is true. I still prefer to just call the componentDidLeave hook after setState.
Yes,but with one difference: you need to add this: Otherwise React will not call the function passed to setState() in ReactTransitionGroup. (I think it is because of the difference between setState being synchronous or asynchronous depending how it was called). The reason why use timeouts it is exactly control the transition time. You will need that in a real application of the ReactTransitionGroup. |
|
At least for this case we could also use componentWillUnmount to flag if setState can be called or not. |
|
Thanks for clarifying, I can see the behavior you're describing. This seems to represent the exact anit-pattern that Is there a reason you want to completely unmount componentWillLeave: function (callback) {
if (this.props.id === 0) {
setTimeout(() => {
callback()
this.props.onAllChildrenLeft()
}, 0);
}
else {
callback();
}
}This would make sure |
Maybe it is the opposite, it is because the setState() is flushed right away that the warning happens, I think that is the behavior when using any kind of update that is not related to Synthetic Events.
That solves the warning, but adding a timeout to the setState that removes the group also does: We could use componentWillUnmount instead of componentDidLeave too, but that wasn't what I had in mind when I opened the pull request. In both of our approaches, the developer needs to know the internals of ReactTransitionGroup to understand what he is doing. Looking at the API would make perfect sense to add the callback in componentDidLeave(), I mean, why add your callback in componentWillLeave when you want to know if a child left? But, the problem with the componentDidLeave() callback is that is being called before the component actually leaves.
Well, we could use display:none, but would be preferable to remove the entire node so React can just ignore it. Again, when I opened the pull request I wasn't looking for a solution for this problem, but for a fix for something that IMHO is an API issue. |
I'm not entirely sure what you mean, the synthetic event system doesn't have a direct connection to how React manages You get the warning because you're pushing the I'm interested in what some of the React team thinks (cc @jimfb @spicyj) |
I don't have knowledge about the internals to know what is happening so I will not start a discuss here, this is more like a guess and a question at the same time. I just read (don't have a really good source to point here) that setState calls happening because of an (synthetic) event handler are batched, while the ones happening because of a setTimeout or other asynchronous function need to be flushed right away. I'm not sure if I get it what you mean by "top of the call stack". In my mind is like that:
The problem for me is that the setState in 5 happens before the setState in 6.
I also don't like it. I pointed another two ways of solving it, one using componentWillUnmount and the other moving the componentDidLeave call to the bottom of _handleDoneLeaving. |
I'm pretty sure that's not how state updates are queued. After it does that, it processes your |
|
About the setState OK, you must know better than me how his implementation works, and does not affect this pull request directly.
You are misunderstanding the workflow. What you described is not my architecture:
The setState giving the warning is not mine, is from ReactTransitionGroup. You should give a look at how the ReactCSSTransitionGroup is implemented. We should let the others give a look because we already discussed too much in this pull request and it is becoming difficult to read. |
|
Generally, if a function calls Is there a reason we can’t do this here? I’ll try to take a closer look later today but that’s the general guideline. |
|
Could #6710 be another symptom of the same problem? |
|
@gaearon the setTimeout is cleaned in the componentWillUnmount(). The problem is not that. When _handleDoneLeaving() is executed means the timeout is not in the queue anymore because it just got dispatched. In other words, the callback passed to componentWillLeave(callback) (remember that componentWillLeave() is where the timeout is created) is in fact: ReactTransitionGroup._handleDoneLeaving() function. You can reproduce the problem we are talking about using the ReactCSSTransitionGroup adding some lines of code to this component. |
|
I don't think #6710 is the same problem, because there he is checking if |
|
Would delaying |
|
If you are taking about the _handleDoneLeaving() It doesn't solve. That is what I tried before, but it fails the test cases because you can not guarantee anymore that componentWillUnmount() will be called after componentDidLeave(). And this order is something that the test cases takes into consideration. You also would need to have different approaches for when the component really will leave and when he was supposed to leave, but he entered again (in the source code of _handleDoneLeaving() in ReactTransitionGroup this is the "if" case), otherwise you would call component.componentWillEnter() before component.componentDidLeave() |
|
Eh. I’m going to tentatively accept it because:
So unless @spicyj or @zpao have reservations about this I’m going to merge it in a few days. |
I feel like none of those are really great justifications. Using This issue boils down to the fact that the API allows/encourages enqueing a Could |
Good point. I wasn’t aware
IMO “fake” lifecycle hooks like |
|
In this case I’m closing this particular PR. You are welcome to look for another way to solve this problem, but I think at this point it might be easier for you to fork |
If the developer uses the componentDidLeave hook to call setState() and remove the react transition group component, then the setState in _handleDoneLeaving() in ReactTransitionGroup.js will generate the warning:
To fix that we just need to place componentDidLeave() call after the setState in _handleDoneLeaving() so any setState called by the user will be placed in the queue after the setState in _handleDoneLeaving() or use isMounted().