Merge pull request #8528 from acdlite/callbacksemantics

Give setState callbacks componentWillUpdate semantics

scripts/fiber/tests-failing.txt

@@ -82,9 +82,6 @@ src/renderers/shared/shared/__tests__/ReactCompositeComponent-test.js
 * should warn about `setState` in getChildContext
 * should update refs if shouldComponentUpdate gives false
 
-src/renderers/shared/shared/__tests__/ReactCompositeComponentNestedState-test.js
-* should provide up to date values for props
-
 src/renderers/shared/shared/__tests__/ReactCompositeComponentState-test.js
 * should update state when called from child cWRP
 

scripts/fiber/tests-passing.txt

@@ -505,6 +505,7 @@ src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js
 * should render strings as children
 * should render numbers as children
 * should be called a callback argument
+* should call a callback argument when the same element is re-rendered
 * should render a component returning strings directly from render
 * should render a component returning numbers directly from render
 * finds the DOM Text node of a string child
@@ -1323,6 +1324,9 @@ src/renderers/shared/shared/__tests__/ReactCompositeComponentDOMMinimalism-test.
 * should not render extra nodes for non-interpolated text
 * should not render extra nodes for non-interpolated text
 
+src/renderers/shared/shared/__tests__/ReactCompositeComponentNestedState-test.js
+* should provide up to date values for props
+
 src/renderers/shared/shared/__tests__/ReactCompositeComponentState-test.js
 * should support setting state
 * should call componentDidUpdate of children first

src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js

@@ -64,6 +64,35 @@ describe('ReactDOMFiber', () => {
     expect(called).toEqual(true);
   });
 
+  it('should call a callback argument when the same element is re-rendered', () => {
+    class Foo extends React.Component {
+      render() {
+        return <div>Foo</div>;
+      }
+    }
+    const element = <Foo />;
+
+    // mounting phase
+    let called = false;
+    ReactDOM.render(
+      element,
+      container,
+      () => called = true
+    );
+    expect(called).toEqual(true);
+
+    // updating phase
+    called = false;
+    ReactDOM.unstable_batchedUpdates(() => {
+      ReactDOM.render(
+        element,
+        container,
+        () => called = true
+      );
+    });
+    expect(called).toEqual(true);
+  });
+
   if (ReactDOMFeatureFlags.useFiber) {
     it('should render a component returning strings directly from render', () => {
       const Text = ({value}) => value;

src/renderers/shared/shared/__tests__/ReactCompositeComponentNestedState-test.js

@@ -111,8 +111,8 @@ describe('ReactCompositeComponentNestedState-state', () => {
       ['setState-this', 'dark blue', 'blue'],
       ['setState-args', 'dark blue', 'green'],
       ['render', 'light green', 'green'],
-      ['parent-after-setState', 'green'],
       ['after-setState', 'light green', 'green'],
+      ['parent-after-setState', 'green'],
     ]);
   });
 });

src/renderers/shared/stack/reconciler/ReactCompositeComponent.js

@@ -780,6 +780,16 @@ var ReactCompositeComponent = {
         this._context
       );
     } else {
+      var callbacks = this._pendingCallbacks;
+      this._pendingCallbacks = null;
+      if (callbacks) {
+        for (var j = 0; j < callbacks.length; j++) {
+          transaction.getReactMountReady().enqueue(
+            callbacks[j],
+            this.getPublicInstance()
+          );
+        }
+      }
       this._updateBatchNumber = null;
     }
   },
@@ -848,6 +858,11 @@ var ReactCompositeComponent = {
       }
     }
 
+    // If updating happens to enqueue any new updates, we shouldn't execute new
+    // callbacks until the next render happens, so stash the callbacks first.
+    var callbacks = this._pendingCallbacks;
+    this._pendingCallbacks = null;
+
     var nextState = this._processPendingState(nextProps, nextContext);
     var shouldUpdate = true;
 
@@ -901,6 +916,15 @@ var ReactCompositeComponent = {
       inst.state = nextState;
       inst.context = nextContext;
     }
+
+    if (callbacks) {
+      for (var j = 0; j < callbacks.length; j++) {
+        transaction.getReactMountReady().enqueue(
+          callbacks[j],
+          this.getPublicInstance()
+        );
+      }
+    }
   },
 
   _processPendingState: function(props, context) {

src/renderers/shared/stack/reconciler/ReactUpdates.js

@@ -135,12 +135,6 @@ function runBatchedUpdates(transaction) {
     // that performUpdateIfNecessary is a noop.
     var component = dirtyComponents[i];
 
-    // If performUpdateIfNecessary happens to enqueue any new updates, we
-    // shouldn't execute the callbacks until the next render happens, so
-    // stash the callbacks first
-    var callbacks = component._pendingCallbacks;
-    component._pendingCallbacks = null;
-
     var markerName;
     if (ReactFeatureFlags.logTopLevelRenders) {
       var namedComponent = component;
@@ -161,15 +155,6 @@ function runBatchedUpdates(transaction) {
     if (markerName) {
       console.timeEnd(markerName);
     }
-
-    if (callbacks) {
-      for (var j = 0; j < callbacks.length; j++) {
-        transaction.reconcileTransaction.getReactMountReady().enqueue(
-          callbacks[j],
-          component.getPublicInstance()
-        );
-      }
-    }
   }
 }