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.
facebook · Apr 3, 2018 · 0f2f90b · 0f2f90b
1 parent da4e855
commit 0f2f90b
Show file tree

Hide file tree

Showing 2 changed files with 92 additions and 0 deletions.
diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.js b/packages/react-reconciler/src/ReactFiberClassComponent.js
@@ -839,6 +839,17 @@ export default function(
         newState === null || newState === undefined
           ? derivedStateFromProps
           : Object.assign({}, newState, derivedStateFromProps);
+
+      // Update the base state of the update queue.
+      // FIXME: This is getting ridiculous. Refactor plz!
+      const updateQueue = workInProgress.updateQueue;
+      if (updateQueue !== null) {
+        updateQueue.baseState = Object.assign(
+          {},
+          updateQueue.baseState,
+          derivedStateFromProps,
+        );
+      }
     }
     if (derivedStateFromCatch !== null && derivedStateFromCatch !== undefined) {
       // Render-phase updates (like this) should not be added to the update queue,
@@ -848,6 +859,17 @@ export default function(
         newState === null || newState === undefined
           ? derivedStateFromCatch
           : Object.assign({}, newState, derivedStateFromCatch);
+
+      // Update the base state of the update queue.
+      // FIXME: This is getting ridiculous. Refactor plz!
+      const updateQueue = workInProgress.updateQueue;
+      if (updateQueue !== null) {
+        updateQueue.baseState = Object.assign(
+          {},
+          updateQueue.baseState,
+          derivedStateFromCatch,
+        );
+      }
     }
 
     if (
@@ -1016,6 +1038,17 @@ export default function(
         newState === null || newState === undefined
           ? derivedStateFromProps
           : Object.assign({}, newState, derivedStateFromProps);
+
+      // Update the base state of the update queue.
+      // FIXME: This is getting ridiculous. Refactor plz!
+      const updateQueue = workInProgress.updateQueue;
+      if (updateQueue !== null) {
+        updateQueue.baseState = Object.assign(
+          {},
+          updateQueue.baseState,
+          derivedStateFromProps,
+        );
+      }
     }
     if (derivedStateFromCatch !== null && derivedStateFromCatch !== undefined) {
       // Render-phase updates (like this) should not be added to the update queue,
@@ -1025,6 +1058,17 @@ export default function(
         newState === null || newState === undefined
           ? derivedStateFromCatch
           : Object.assign({}, newState, derivedStateFromCatch);
+
+      // Update the base state of the update queue.
+      // FIXME: This is getting ridiculous. Refactor plz!
+      const updateQueue = workInProgress.updateQueue;
+      if (updateQueue !== null) {
+        updateQueue.baseState = Object.assign(
+          {},
+          updateQueue.baseState,
+          derivedStateFromCatch,
+        );
+      }
     }
 
     if (

diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.internal.js
@@ -373,4 +373,52 @@ describe('ReactIncrementalUpdates', () => {
     });
     ReactNoop.flush();
   });
+
+  it('getDerivedStateFromProps should update base state of updateQueue (based on product bug)', () => {
+    // Based on real-world bug.
+
+    let foo;
+    class Foo extends React.Component {
+      state = {value: 'initial state'};
+      static getDerivedStateFromProps() {
+        return {value: 'derived state'};
+      }
+      render() {
+        foo = this;
+        return (
+          <React.Fragment>
+            <span prop={this.state.value} />
+            <Bar />
+          </React.Fragment>
+        );
+      }
+    }
+
+    let bar;
+    class Bar extends React.Component {
+      render() {
+        bar = this;
+        return null;
+      }
+    }
+
+    ReactNoop.flushSync(() => {
+      ReactNoop.render(<Foo />);
+    });
+    expect(ReactNoop.getChildren()).toEqual([span('derived state')]);
+
+    ReactNoop.flushSync(() => {
+      // Triggers getDerivedStateFromProps again
+      ReactNoop.render(<Foo />);
+      // The noop callback is needed to trigger the specific internal path that
+      // led to this bug. Removing it causes it to "accidentally" work.
+      foo.setState({value: 'update state'}, function noop() {});
+    });
+    expect(ReactNoop.getChildren()).toEqual([span('derived state')]);
+
+    ReactNoop.flushSync(() => {
+      bar.setState({});
+    });
+    expect(ReactNoop.getChildren()).toEqual([span('derived state')]);
+  });
 });