getDerivedStateFrom{Props,Catch} should update updateQueue.baseState (#12528)
Based on a bug found in UFI2. There have been several bugs related to the update queue (and specifically baseState) recently, so I'm going to follow-up with some refactoring to clean it up. This is a quick fix so we can ship a patch release.
Andrew Clark committed
Apr 3, 2018 at 13:02 UTC
0f2f90bd9a9daf241d691bf4af3ea2e3a263c0e3
2 files changed
+92
packages/react-reconciler/src/ReactFiberClassComponent.js
+44
@@ -839,6 +839,17 @@ export default function(
839
newState === null || newState === undefined
840
? derivedStateFromProps
841
: Object.assign({}, newState, derivedStateFromProps);
842
+
843
+ // Update the base state of the update queue.
844
+ // FIXME: This is getting ridiculous. Refactor plz!
845
+ const updateQueue = workInProgress.updateQueue;
846
+ if (updateQueue !== null) {
847
+ updateQueue.baseState = Object.assign(
848
+ {},
849
+ updateQueue.baseState,
850
+ derivedStateFromProps,
851
+ );
852
+ }
853
}
854
if (derivedStateFromCatch !== null && derivedStateFromCatch !== undefined) {
855
// Render-phase updates (like this) should not be added to the update queue,
@@ -848,6 +859,17 @@ export default function(
859
newState === null || newState === undefined
860
? derivedStateFromCatch
861
: Object.assign({}, newState, derivedStateFromCatch);
862
+
863
+ // Update the base state of the update queue.
864
+ // FIXME: This is getting ridiculous. Refactor plz!
865
+ const updateQueue = workInProgress.updateQueue;
866
+ if (updateQueue !== null) {
867
+ updateQueue.baseState = Object.assign(
868
+ {},
869
+ updateQueue.baseState,
870
+ derivedStateFromCatch,
871
+ );
872
+ }
873
}
874
875
if (
@@ -1016,6 +1038,17 @@ export default function(
1038
newState === null || newState === undefined
1039
? derivedStateFromProps
1040
: Object.assign({}, newState, derivedStateFromProps);
1041
+
1042
+ // Update the base state of the update queue.
1043
+ // FIXME: This is getting ridiculous. Refactor plz!
1044
+ const updateQueue = workInProgress.updateQueue;
1045
+ if (updateQueue !== null) {
1046
+ updateQueue.baseState = Object.assign(
1047
+ {},
1048
+ updateQueue.baseState,
1049
+ derivedStateFromProps,
1050
+ );
1051
+ }
1052
}
1053
if (derivedStateFromCatch !== null && derivedStateFromCatch !== undefined) {
1054
// Render-phase updates (like this) should not be added to the update queue,
@@ -1025,6 +1058,17 @@ export default function(
1058
newState === null || newState === undefined
1059
? derivedStateFromCatch
1060
: Object.assign({}, newState, derivedStateFromCatch);
1061
+
1062
+ // Update the base state of the update queue.
1063
+ // FIXME: This is getting ridiculous. Refactor plz!
1064
+ const updateQueue = workInProgress.updateQueue;
1065
+ if (updateQueue !== null) {
1066
+ updateQueue.baseState = Object.assign(
1067
+ {},
1068
+ updateQueue.baseState,
1069
+ derivedStateFromCatch,
1070
+ );
1071
+ }
1072
}
1073
1074
if (
packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.internal.js
+48
@@ -373,4 +373,52 @@ describe('ReactIncrementalUpdates', () => {
373
});
374
ReactNoop.flush();
375
});
376
+
377
+ it('getDerivedStateFromProps should update base state of updateQueue (based on product bug)', () => {
378
+ // Based on real-world bug.
379
+
380
+ let foo;
381
+ class Foo extends React.Component {
382
+ state = {value: 'initial state'};
383
+ static getDerivedStateFromProps() {
384
+ return {value: 'derived state'};
385
+ }
386
+ render() {
387
+ foo = this;
388
+ return (
389
+ <React.Fragment>
390
+ <span prop={this.state.value} />
391
+ <Bar />
392
+ </React.Fragment>
393
+ );
394
+ }
395
+ }
396
+
397
+ let bar;
398
+ class Bar extends React.Component {
399
+ render() {
400
+ bar = this;
401
+ return null;
402
+ }
403
+ }
404
+
405
+ ReactNoop.flushSync(() => {
406
+ ReactNoop.render(<Foo />);
407
+ });
408
+ expect(ReactNoop.getChildren()).toEqual([span('derived state')]);
409
+
410
+ ReactNoop.flushSync(() => {
411
+ // Triggers getDerivedStateFromProps again
412
+ ReactNoop.render(<Foo />);
413
+ // The noop callback is needed to trigger the specific internal path that
414
+ // led to this bug. Removing it causes it to "accidentally" work.
415
+ foo.setState({value: 'update state'}, function noop() {});
416
+ });
417
+ expect(ReactNoop.getChildren()).toEqual([span('derived state')]);
418
+
419
+ ReactNoop.flushSync(() => {
420
+ bar.setState({});
421
+ });
422
+ expect(ReactNoop.getChildren()).toEqual([span('derived state')]);
423
+ });
424
});