Fixed a batched-state update bug with getDerivedStateFromProps (#12408)
Brian Vaughn committed
Mar 21, 2018 at 11:42 UTC
dc48326cd5fd85fdc344c62b5e0ada479d749b4b
3 files changed
+76
-26
packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js
+45
@@ -830,4 +830,49 @@ describe('ReactComponentLifeCycle', () => {
830
'UNSAFE_componentWillUpdate',
831
]);
832
});
833
+
834
+ it('should not override state with stale values if prevState is spread within getDerivedStateFromProps', () => {
835
+ const divRef = React.createRef();
836
+ let childInstance;
837
+
838
+ class Child extends React.Component {
839
+ state = {local: 0};
840
+ static getDerivedStateFromProps(nextProps, prevState) {
841
+ return {...prevState, remote: nextProps.remote};
842
+ }
843
+ updateState = () => {
844
+ this.setState(state => ({local: state.local + 1}));
845
+ this.props.onChange(this.state.remote + 1);
846
+ };
847
+ render() {
848
+ childInstance = this;
849
+ return (
850
+ <div onClick={this.updateState} ref={divRef}>{`remote:${
851
+ this.state.remote
852
+ }, local:${this.state.local}`}</div>
853
+ );
854
+ }
855
+ }
856
+
857
+ class Parent extends React.Component {
858
+ state = {value: 0};
859
+ handleChange = value => {
860
+ this.setState({value});
861
+ };
862
+ render() {
863
+ return <Child remote={this.state.value} onChange={this.handleChange} />;
864
+ }
865
+ }
866
+
867
+ ReactTestUtils.renderIntoDocument(<Parent />);
868
+ expect(divRef.current.textContent).toBe('remote:0, local:0');
869
+
870
+ // Trigger setState() calls
871
+ childInstance.updateState();
872
+ expect(divRef.current.textContent).toBe('remote:1, local:1');
873
+
874
+ // Trigger batched setState() calls
875
+ ReactTestUtils.Simulate.click(divRef.current);
876
+ expect(divRef.current.textContent).toBe('remote:2, local:2');
877
+ });
878
});
packages/react-reconciler/src/ReactFiberBeginWork.js
+1
@@ -612,6 +612,7 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
612
workInProgress,
613
value,
614
props,
615
+ workInProgress.memoizedState,
616
);
617
618
if (partialState !== null && partialState !== undefined) {
packages/react-reconciler/src/ReactFiberClassComponent.js
+30
-26
@@ -448,6 +448,7 @@ export default function(
448
workInProgress,
449
instance,
450
props,
451
+ state,
452
);
453
454
if (partialState !== null && partialState !== undefined) {
@@ -534,7 +535,8 @@ export default function(
535
function callGetDerivedStateFromProps(
536
workInProgress: Fiber,
537
instance: any,
537
- props: any,
538
+ nextProps: any,
539
+ prevState: any,
540
) {
541
const {type} = workInProgress;
542
@@ -567,17 +569,13 @@ export default function(
569
workInProgress.mode & StrictMode)
570
) {
571
// Invoke method an extra time to help detect side-effects.
570
- type.getDerivedStateFromProps.call(
571
- null,
572
- props,
573
- workInProgress.memoizedState,
574
- );
572
+ type.getDerivedStateFromProps.call(null, nextProps, prevState);
573
}
574
575
const partialState = type.getDerivedStateFromProps.call(
576
null,
579
- props,
580
- workInProgress.memoizedState,
577
+ nextProps,
578
+ prevState,
579
);
580
581
if (__DEV__) {
@@ -698,15 +696,6 @@ export default function(
696
}
697
}
698
701
- let derivedStateFromProps;
702
- if (oldProps !== newProps) {
703
- derivedStateFromProps = callGetDerivedStateFromProps(
704
- workInProgress,
705
- instance,
706
- newProps,
707
- );
708
- }
709
-
699
// Compute the next state using the memoized state and the update queue.
700
const oldState = workInProgress.memoizedState;
701
// TODO: Previous state can be null.
@@ -743,6 +732,18 @@ export default function(
732
newState = oldState;
733
}
734
735
+ let derivedStateFromProps;
736
+ if (oldProps !== newProps) {
737
+ // The prevState parameter should be the partially updated state.
738
+ // Otherwise, spreading state in return values could override updates.
739
+ derivedStateFromProps = callGetDerivedStateFromProps(
740
+ workInProgress,
741
+ instance,
742
+ newProps,
743
+ newState,
744
+ );
745
+ }
746
+
747
if (derivedStateFromProps !== null && derivedStateFromProps !== undefined) {
748
// Render-phase updates (like this) should not be added to the update queue,
749
// So that multiple render passes do not enqueue multiple updates.
@@ -867,15 +868,6 @@ export default function(
868
}
869
}
870
870
- let derivedStateFromProps;
871
- if (oldProps !== newProps) {
872
- derivedStateFromProps = callGetDerivedStateFromProps(
873
- workInProgress,
874
- instance,
875
- newProps,
876
- );
877
- }
878
-
871
// Compute the next state using the memoized state and the update queue.
872
const oldState = workInProgress.memoizedState;
873
// TODO: Previous state can be null.
@@ -912,6 +904,18 @@ export default function(
904
newState = oldState;
905
}
906
907
+ let derivedStateFromProps;
908
+ if (oldProps !== newProps) {
909
+ // The prevState parameter should be the partially updated state.
910
+ // Otherwise, spreading state in return values could override updates.
911
+ derivedStateFromProps = callGetDerivedStateFromProps(
912
+ workInProgress,
913
+ instance,
914
+ newProps,
915
+ newState,
916
+ );
917
+ }
918
+
919
if (derivedStateFromProps !== null && derivedStateFromProps !== undefined) {
920
// Render-phase updates (like this) should not be added to the update queue,
921
// So that multiple render passes do not enqueue multiple updates.