From 0ca1cea26ae66fa26973c4f3912da7e7b8ed47f5 Mon Sep 17 00:00:00 2001
From: Andrew Clark <acdlite@me.com>
Date: Sat, 6 Aug 2016 16:47:48 -0700
Subject: [PATCH] Don't mutate current tree before work is committed.

We should be able to abort an update without any side-effects to the
current tree. This fixes a few cases where that was broken.

The callback list should only ever be set on the workInProgress.
There's no reason to add it to the current tree because they're not
needed after they are called during the commit phase.

Also found a bug where the memoizedProps were set to null in the
case of an update, because the pendingProps were null. Fixed by
transfering the props from the instance, like we were already doing
with state.

Added a test to ensure that setState can be called inside a
callback.
---
 .../shared/fiber/ReactFiberCommitWork.js      | 10 +++--
 .../shared/fiber/ReactFiberCompleteWork.js    |  8 ++--
 .../shared/fiber/ReactFiberScheduler.js       |  3 --
 .../fiber/__tests__/ReactIncremental-test.js  | 38 +++++++++++++++++++
 4 files changed, 49 insertions(+), 10 deletions(-)

diff --git a/src/renderers/shared/fiber/ReactFiberCommitWork.js b/src/renderers/shared/fiber/ReactFiberCommitWork.js
index d859003383..beb773a2e7 100644
--- a/src/renderers/shared/fiber/ReactFiberCommitWork.js
+++ b/src/renderers/shared/fiber/ReactFiberCommitWork.js
@@ -32,12 +32,16 @@ module.exports = function<T, P, I, C>(config : HostConfig<T, P, I, C>) {
   function commitWork(current : ?Fiber, finishedWork : Fiber) : void {
     switch (finishedWork.tag) {
       case ClassComponent: {
+        // Clear updates from current fiber. This must go before the callbacks
+        // are reset, in case an update is triggered from inside a callback. Is
+        // this safe? Relies on the assumption that work is only committed if
+        // the update queue is empty.
+        if (finishedWork.alternate) {
+          finishedWork.alternate.updateQueue = null;
+        }
         if (finishedWork.callbackList) {
           const { callbackList } = finishedWork;
           finishedWork.callbackList = null;
-          if (finishedWork.alternate) {
-            finishedWork.alternate.callbackList = null;
-          }
           callCallbacks(callbackList, finishedWork.stateNode);
         }
         // TODO: Fire componentDidMount/componentDidUpdate, update refs
diff --git a/src/renderers/shared/fiber/ReactFiberCompleteWork.js b/src/renderers/shared/fiber/ReactFiberCompleteWork.js
index 6d6f9598c4..d4687f4574 100644
--- a/src/renderers/shared/fiber/ReactFiberCompleteWork.js
+++ b/src/renderers/shared/fiber/ReactFiberCompleteWork.js
@@ -132,14 +132,14 @@ module.exports = function<T, P, I, C>(config : HostConfig<T, P, I, C>) {
         transferOutput(workInProgress.child, workInProgress);
         // Don't use the state queue to compute the memoized state. We already
         // merged it and assigned it to the instance. Transfer it from there.
-        const state = workInProgress.stateNode.state;
+        // Also need to transfer the props, because pendingProps will be null
+        // in the case of an update
+        const { state, props } = workInProgress.stateNode;
         workInProgress.memoizedState = state;
+        workInProgress.memoizedProps = props;
         // Transfer update queue to callbackList field so callbacks can be
         // called during commit phase.
         workInProgress.callbackList = workInProgress.updateQueue;
-        if (current) {
-          current.callbackList = workInProgress.callbackList;
-        }
         markForPostEffect(workInProgress);
         return null;
       case HostContainer:
diff --git a/src/renderers/shared/fiber/ReactFiberScheduler.js b/src/renderers/shared/fiber/ReactFiberScheduler.js
index 122a64fcd5..874c24928c 100644
--- a/src/renderers/shared/fiber/ReactFiberScheduler.js
+++ b/src/renderers/shared/fiber/ReactFiberScheduler.js
@@ -144,9 +144,6 @@ module.exports = function<T, P, I, C>(config : HostConfig<T, P, I, C>) {
       // to the system not to redo any work here.
       workInProgress.pendingProps = null;
       workInProgress.updateQueue = null;
-      if (current) {
-        current.updateQueue = null;
-      }
 
       const returnFiber = workInProgress.return;
 
diff --git a/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js b/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js
index 5213acfa48..c543de4549 100644
--- a/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js
+++ b/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js
@@ -654,6 +654,44 @@ describe('ReactIncremental', () => {
     expect(instance.state.num).toEqual(6);
   });
 
+  it('can call setState inside update callback', () => {
+    let instance;
+    class Bar extends React.Component {
+      constructor() {
+        super();
+        this.state = { num: 1 };
+        instance = this;
+      }
+      render() {
+        return <div>{this.props.children}</div>;
+      }
+    }
+
+    function Foo({ multiplier }) {
+      return (
+        <div>
+          <Bar multiplier={multiplier} />
+        </div>
+      );
+    }
+
+    function updater(state, props) {
+      return { num: state.num * props.multiplier };
+    }
+
+    function callback() {
+      this.setState({ called: true });
+    }
+
+    ReactNoop.render(<Foo multiplier={2} />);
+    ReactNoop.flush();
+    instance.setState(updater);
+    instance.setState(updater, callback);
+    ReactNoop.flush();
+    expect(instance.state.num).toEqual(4);
+    expect(instance.state.called).toEqual(true);
+  });
+
   it('can replaceState', () => {
     let instance;
     const Bar = React.createClass({