diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index ebe26c9d9d..91f4eecbc1 100644 --- a/scripts/fiber/tests-passing.txt +++ b/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 diff --git a/src/renderers/shared/fiber/ReactFiberUpdateQueue.js b/src/renderers/shared/fiber/ReactFiberUpdateQueue.js index 9c9aadccbd..5df5520b15 100644 --- a/src/renderers/shared/fiber/ReactFiberUpdateQueue.js +++ b/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; diff --git a/src/renderers/shared/fiber/__tests__/ReactIncrementalUpdates-test.js b/src/renderers/shared/fiber/__tests__/ReactIncrementalUpdates-test.js index 857a0f7e60..af1f9621c5 100644 --- a/src/renderers/shared/fiber/__tests__/ReactIncrementalUpdates-test.js +++ b/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 ; + } + } + + ReactNoop.render(); + 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(); + }); });