Handle setState inside an updater function

The update is scheduled as if the current processing update has already
been processed; if it has the same or higher priority, it will be
flushed in the same batch.

We also print a warning.

scripts/fiber/tests-passing.txt

@@ -1237,6 +1237,7 @@ src/renderers/shared/fiber/__tests__/ReactIncrementalUpdates-test.js
 * can abort an update, schedule additional updates, and resume
 * can abort an update, schedule a replaceState, and resume
 * does not call callbacks that are scheduled by another callback until a later commit
+* enqueues setState inside an updater function as if the in-progress update is progressed (and warns)
 
 src/renderers/shared/fiber/__tests__/ReactTopLevelFragment-test.js
 * should render a simple fragment at the top of a component

src/renderers/shared/fiber/ReactFiberUpdateQueue.js

@@ -55,6 +55,9 @@ type Update = {
 export type UpdateQueue = {
   first: Update | null,
   last: Update | null,
+
+  // Dev only
+  isProcessing?: boolean,
 };
 
 function comparePriority(a : PriorityLevel, b : PriorityLevel) : number {
@@ -90,10 +93,21 @@ function ensureUpdateQueue(fiber : Fiber) : UpdateQueue {
     // We already have an update queue.
     return fiber.updateQueue;
   }
-  const queue = {
-    first: null,
-    last: null,
-  };
+
+  let queue;
+  if (__DEV__) {
+    queue = {
+      first: null,
+      last: null,
+      isProcessing: false,
+    };
+  } else {
+    queue = {
+      first: null,
+      last: null,
+    };
+  }
+
   fiber.updateQueue = queue;
   return queue;
 }
@@ -170,10 +184,30 @@ function insertUpdateIntoQueue(queue, update, insertAfter, insertBefore) {
 //
 // However, if incoming update is inserted into the same position of both lists,
 // we shouldn't make a copy.
-function insertUpdate(fiber : Fiber, update : Update) : void {
+
+function insertUpdate(fiber : Fiber, update : Update, methodName : ?string) : void {
   const queue1 = ensureUpdateQueue(fiber);
   const queue2 = fiber.alternate ? ensureUpdateQueue(fiber.alternate) : null;
 
+  // Warn if an update is scheduled from inside an updater function.
+  if (__DEV__ && typeof methodName === 'string' && (queue1.isProcessing || (queue2 && queue2.isProcessing))) {
+    if (methodName === 'setState') {
+      console.error(
+        'setState was called from inside the updater function of another' +
+        'setState. A function passed as the first argument of setState ' +
+        'should not contain any side-effects. Return a partial state object ' +
+        'instead of calling setState again. Example: ' +
+        'this.setState(function(state) { return { count: state.count + 1 }; })'
+      );
+    } else {
+      console.error(
+        `${methodName} was called from inside the updater function of ` +
+        'setState. A function passed as the first argument of setState ' +
+        'should not contain any side-effects.'
+      );
+    }
+  }
+
   const priorityLevel = update.priorityLevel;
 
   let queue = queue1;
@@ -238,7 +272,11 @@ function addUpdate(
     isForced: false,
     next: null,
   };
-  insertUpdate(fiber, update);
+  if (__DEV__) {
+    insertUpdate(fiber, update, 'setState');
+  } else {
+    insertUpdate(fiber, update);
+  }
 }
 exports.addUpdate = addUpdate;
 
@@ -286,8 +324,11 @@ function addReplaceUpdate(
       queue = null;
     }
   }
-
-  insertUpdate(fiber, update);
+  if (__DEV__) {
+    insertUpdate(fiber, update, 'replaceState');
+  } else {
+    insertUpdate(fiber, update);
+  }
 }
 exports.addReplaceUpdate = addReplaceUpdate;
 
@@ -300,7 +341,11 @@ function addForceUpdate(fiber : Fiber, priorityLevel : PriorityLevel) : void {
     isForced: true,
     next: null,
   };
-  insertUpdate(fiber, update);
+  if (__DEV__) {
+    insertUpdate(fiber, update, 'forceUpdate');
+  } else {
+    insertUpdate(fiber, update);
+  }
 }
 exports.addForceUpdate = addForceUpdate;
 
@@ -341,15 +386,28 @@ function beginUpdateQueue(
   props : any,
   priorityLevel : PriorityLevel
 ) : any {
+  if (__DEV__) {
+    // Set this flag so we can warn if setState is called inside the update
+    // function of another setState.
+    queue.isProcessing = true;
+  }
+
   // Applies updates with matching priority to the previous state to create
   // a new state object.
-
   let state = prevState;
   let dontMutatePrevState = true;
   let isEmpty = true;
   let callbackList = null;
   let update = queue.first;
   while (update && comparePriority(update.priorityLevel, priorityLevel) <= 0) {
+    // Remove each update from the queue right before it is processed. That way
+    // if setState is called from inside an updater function, the new update
+    // will be inserted in the correct position.
+    queue.first = update.next;
+    if (!queue.first) {
+      queue.last = null;
+    }
+
     let partialState;
     if (update.isReplace) {
       // A replace should drop all previous updates in the queue, so
@@ -392,9 +450,7 @@ function beginUpdateQueue(
     state = prevState;
   }
 
-  if (update) {
-    queue.first = update;
-  } else {
+  if (!queue.first) {
     // Queue is now empty
     workInProgress.updateQueue = null;
   }
@@ -402,6 +458,10 @@ function beginUpdateQueue(
   workInProgress.callbackList = callbackList;
   workInProgress.memoizedState = state;
 
+  if (__DEV__) {
+    queue.isProcessing = false;
+  }
+
   return state;
 }
 exports.beginUpdateQueue = beginUpdateQueue;

src/renderers/shared/fiber/__tests__/ReactIncrementalUpdates-test.js

@@ -245,4 +245,67 @@ describe('ReactIncrementalUpdates', () => {
       'callback b',
     ]);
   });
+
+  it('enqueues setState inside an updater function as if the in-progress update is progressed (and warns)', () => {
+    spyOn(console, 'error');
+    let instance;
+    let ops = [];
+    class Foo extends React.Component {
+      state = {};
+      componentDidMount() {
+        ops.push('componentDidMount');
+        this.setState(function a() {
+          // Force update b to have Task priority
+          ReactNoop.syncUpdates(() => {
+            this.setState({ b: 'b' });
+          });
+          return { a: 'a' };
+        });
+      }
+      render() {
+        ops.push('render');
+        instance = this;
+        return <span prop={Object.keys(this.state).join('')} />;
+      }
+    }
+
+    ReactNoop.render(<Foo />);
+    ReactNoop.flush();
+    expectDev(console.error.calls.count()).toBe(1);
+    expect(ReactNoop.getChildren()).toEqual([span('ab')]);
+    expect(ops).toEqual([
+      // Initial render
+      'render',
+      'componentDidMount',
+      // Updates a and b both have Task priority. Update b is enqueued while
+      // update a is being processed, but it should be inserted into the queue
+      // as if update a is already processed. Then processing continues. Because
+      // they have the same priority, update b is processed in the same batch.
+      // So there should only be a single render below.
+      'render',
+    ]);
+
+    ops = [];
+
+    ReactNoop.performAnimationWork(() => {
+      instance.setState(function c() {
+        // Update d happens during the begin phase, so it has low priority.
+        this.setState({ d: 'd' });
+        return { c: 'c' };
+      });
+    });
+
+    ReactNoop.flush();
+    expect(ReactNoop.getChildren()).toEqual([span('abcd')]);
+    expect(ops).toEqual([
+      // Update c has animation priority. Update d is enqueued while c is being
+      // processed with animation priority. Because d is low priority, it is not
+      // processed until the next render. So there should be two renders below.
+      'render',
+      'render',
+    ]);
+
+    expectDev(console.error.calls.count()).toBe(2);
+    console.error.calls.reset();
+  });
 });