Fixed a batched-state update bug with getDerivedStateFromProps #12408
Conversation
|
Once this PR is approved and merged- (assuming it is)- I'll cut another alpha release with it and the recent context infinite loop bugfix (#12402). |
|
|
||
| let derivedStateFromProps; | ||
| if (oldProps !== newProps) { | ||
| // In this case, the prevState parameter should be the partially updated state, |
There was a problem hiding this comment.
Conceptually, what you're calling prevState is the work-in-progress state. So your comment here applies to all cases; it's not special.
There was a problem hiding this comment.
"in this case" just referred to the update case. The other two invocations of this method are mounting cases. But I'll re-word the comment.
| @@ -867,19 +867,11 @@ export default function( | |||
| } | |||
| } | |||
|
|
|||
| let derivedStateFromProps; | |||
There was a problem hiding this comment.
Why did you change the order of getDerivedStateFromCatch and getDerivedStateFromProps? The new order seems correct, just curious what led you to make the change in this diff.
It looks like the order in the "resume mount" path is different now, though. Should be consistent regardless.
There was a problem hiding this comment.
Why did you change the order of
getDerivedStateFromCatchandgetDerivedStateFromProps?
This was incidental. I changed the order of processUpdateQueue and getDerivedStateFromProps. I did that because it seemed easier to get the correct partial state value that way than trying to mess with the update queue and it's memoized vs base state.
It looks like the order in the "resume mount" path is different now, though. Should be consistent regardless.
Fair 👍
There was a problem hiding this comment.
One more thing: could you add a test so the order doesn't incidentally change in the future?
| } | ||
|
|
||
| const instance = ReactTestUtils.renderIntoDocument(<Parent />); | ||
| const element = ReactDOM.findDOMNode(instance); |
There was a problem hiding this comment.
Nit: let's not use findDOMNode. Maybe a ref instead? I know it's just a test, but we should still try to avoid using legacy APIs :D
There was a problem hiding this comment.
I confess, I often copy paste an existing test when writing a new one to save a few seconds.
|
Back to you @acdlite, with the one change you requested. |
|
Chatted in meat space. Merging! |
Addresses a batched state update bug reported in #12047.