Fix bug with double updates in a single batch #6650
Conversation
ghost
added
| this.updateComponent( | ||
| transaction, | ||
| this._currentElement, | ||
| this._currentElement, | ||
| this._context, | ||
| this._context | ||
| ); | ||
| } else { | ||
| this._updateBatchNumber = null; |
There was a problem hiding this comment.
This line seems to imply that enqueueUpdate is getting called on a component that does not need to be updated (meets neither of the two conditions above) - which seems bad/wrong. What am I missing here?
There was a problem hiding this comment.
Yeah. :( setState calls enqueueSetState and enqueueCallback which each create a separate batch. That is almost certainly unintended but I left it as-is for now.
There was a problem hiding this comment.
That is, enqueueCallback marks the component dirty which is harmless when done in conjunction with enqueueSetState but kinda silly when it can get split into a separate batch.
|
Looks good to me. Let's ship it. 👍 |
|
👍 |
Fixes facebook#2410. Fixes facebook#6371. Fixes facebook#6538. I also manually tested the codepen in facebook#3762 and verified it now works.
|
@spicyj updated the pull request. |
|
As discussed in person: If componentWillUnmount is allowed to have side-effects (which I think it needs to because unsubscribing is a side-effect) which includes setState on another component, then it should be outside of the reconciliation pass which is supposed to be side-effect free. Maybe we should queue/defer unmounts? |
|
Going to bump this to 15.x since it doesn't apply cleanly without some of the things we're taking there. Shouldn't matter too much in terms of timing. |
The code here feels a little convoluted but I don't know how to make it clearer. Suggestions welcome.
cc @facebook/react-core
Fixes #2410. Fixes #6371. Fixes #6538.
I also manually tested the codepen in #3762 and verified it now works.