From ea622e52c20b3e1b04862d41754dad5fb5c581c8 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 15 Dec 2016 17:34:24 -0800 Subject: [PATCH 1/4] Use warning instead of console.error --- .../shared/fiber/ReactFiberUpdateQueue.js | 37 +++++++++++-------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/src/renderers/shared/fiber/ReactFiberUpdateQueue.js b/src/renderers/shared/fiber/ReactFiberUpdateQueue.js index 50f8ceeb47082..1dffcc62d03b3 100644 --- a/src/renderers/shared/fiber/ReactFiberUpdateQueue.js +++ b/src/renderers/shared/fiber/ReactFiberUpdateQueue.js @@ -25,6 +25,8 @@ const { TaskPriority, } = require('ReactPriorityLevel'); +const warning = require('warning'); + type PartialState = $Subtype | (prevState: State, props: Props) => $Subtype; @@ -193,21 +195,26 @@ function insertUpdate(fiber : Fiber, update : Update, methodName : ?string) : vo 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.' - ); + if (__DEV__ && typeof methodName === 'string') { + if (queue1.isProcessing || (queue2 && queue2.isProcessing)) { + if (methodName === 'setState') { + warning( + false, + '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 { + warning( + false, + '%s was called from inside the updater function of setState. A ' + + 'function passed as the first argument of setState ' + + 'should not contain any side-effects.', + methodName + ); + } } } From dfe3d089cc1e58be0a8afdecebc389b87746d4e9 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 15 Dec 2016 18:10:25 -0800 Subject: [PATCH 2/4] Clean up some style nits and other code detritus in the update queue --- .../shared/fiber/ReactFiberUpdateQueue.js | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/src/renderers/shared/fiber/ReactFiberUpdateQueue.js b/src/renderers/shared/fiber/ReactFiberUpdateQueue.js index 1dffcc62d03b3..544db31edb80d 100644 --- a/src/renderers/shared/fiber/ReactFiberUpdateQueue.js +++ b/src/renderers/shared/fiber/ReactFiberUpdateQueue.js @@ -408,7 +408,6 @@ function beginUpdateQueue( // 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) { @@ -426,7 +425,6 @@ function beginUpdateQueue( // use the original `prevState`, not the accumulated `state` state = getStateFromUpdate(update, instance, prevState, props); dontMutatePrevState = true; - isEmpty = false; } else { partialState = getStateFromUpdate(update, instance, state, props); if (partialState) { @@ -436,7 +434,6 @@ function beginUpdateQueue( state = Object.assign(state, partialState); } dontMutatePrevState = false; - isEmpty = false; } } if (update.isForced) { @@ -447,9 +444,10 @@ function beginUpdateQueue( callbackList.last.next = update; callbackList.last = update; } else { + const callbackUpdate = cloneUpdate(update); callbackList = { - first: update, - last: update, + first: callbackUpdate, + last: callbackUpdate, hasForceUpdate: false, }; } @@ -458,18 +456,12 @@ function beginUpdateQueue( update = update.next; } - if (isEmpty) { - // None of the updates contained state. Use the original state object. - state = prevState; - } - if (!queue.first && !queue.hasForceUpdate) { // Queue is now empty workInProgress.updateQueue = null; } workInProgress.callbackList = callbackList; - workInProgress.memoizedState = state; if (__DEV__) { queue.isProcessing = false; From f070aaa1b84a239a0ed8aeb98ebc185ae925b6f3 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 15 Dec 2016 18:15:03 -0800 Subject: [PATCH 3/4] Remove confusing for-loop in favor of a helper function This logic is hopefully easier to follow. Added more inline comments to explain how the algorithm works. --- .../shared/fiber/ReactFiberUpdateQueue.js | 93 ++++++++++--------- 1 file changed, 49 insertions(+), 44 deletions(-) diff --git a/src/renderers/shared/fiber/ReactFiberUpdateQueue.js b/src/renderers/shared/fiber/ReactFiberUpdateQueue.js index 544db31edb80d..721ff0259cbc4 100644 --- a/src/renderers/shared/fiber/ReactFiberUpdateQueue.js +++ b/src/renderers/shared/fiber/ReactFiberUpdateQueue.js @@ -162,6 +162,26 @@ function insertUpdateIntoQueue(queue, update, insertAfter, insertBefore) { } } +// Returns the update after which the incoming update should be inserted into +// the queue, or null if it should be inserted at beginning. +function findInsertionPosition(queue, update) : Update | null { + const priorityLevel = update.priorityLevel; + let insertAfter = null; + let insertBefore = null; + if (queue.last && comparePriority(queue.last.priorityLevel, priorityLevel) <= 0) { + // Fast path for the common case where the update should be inserted at + // the end of the queue. + insertAfter = queue.last; + } else { + insertBefore = queue.first; + while (insertBefore && comparePriority(insertBefore.priorityLevel, priorityLevel) <= 0) { + insertAfter = insertBefore; + insertBefore = insertBefore.next; + } + } + return insertAfter; +} + // The work-in-progress queue is a subset of the current queue (if it exists). // We need to insert the incoming update into both lists. However, it's possible // that the correct position in one list will be different from the position in @@ -189,7 +209,6 @@ 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, methodName : ?string) : void { const queue1 = ensureUpdateQueue(fiber); const queue2 = fiber.alternate ? ensureUpdateQueue(fiber.alternate) : null; @@ -218,54 +237,40 @@ function insertUpdate(fiber : Fiber, update : Update, methodName : ?string) : vo } } - const priorityLevel = update.priorityLevel; + // Find the insertion position in the first queue. + const insertAfter1 = findInsertionPosition(queue1, update); + const insertBefore1 = insertAfter1 ? insertAfter1.next : queue1.first; - let queue = queue1; - let insertAfter1; - let insertBefore1; - let insertAfter2; - let insertBefore2; - for (let i = 0; queue && i < 2; i++) { - let insertAfter = null; - let insertBefore = null; - if (queue.last && comparePriority(queue.last.priorityLevel, priorityLevel) <= 0) { - // Fast path for the common case where the update should be inserted at - // the end of the queue. - insertAfter = queue.last; - } else { - insertBefore = queue.first; - while (insertBefore && comparePriority(insertBefore.priorityLevel, priorityLevel) <= 0) { - insertAfter = insertBefore; - insertBefore = insertBefore.next; - } - } - if (i === 0) { - insertAfter1 = insertAfter; - insertBefore1 = insertBefore; - queue = queue2; - } else { - insertAfter2 = insertAfter; - insertBefore2 = insertBefore; - queue = null; - } + if (!queue2) { + // If there's no alternate queue, there's nothing else to do but insert. + insertUpdateIntoQueue(queue1, update, insertAfter1, insertBefore1); + return; } - const update1 = update; - insertUpdateIntoQueue(queue1, update1, insertAfter1, insertBefore1); + // If there is an alternate queue, find the insertion position. + const insertAfter2 = findInsertionPosition(queue2, update); + const insertBefore2 = insertAfter2 ? insertAfter2.next : queue2.first; - if (queue2) { - let update2; - if (insertBefore1 === insertBefore2) { - // The update is inserted into the same position of both lists. There's no - // need to clone the update. - update2 = update1; - } else { - // The update is inserted into two separate positions. Make a clone of the - // update and insert it twice. One or the other will be dropped the next - // time we commit. - update2 = cloneUpdate(update1); - } + // Now we can insert into the first queue. This must come after finding both + // insertion positions because it mutates the list. + insertUpdateIntoQueue(queue1, update, insertAfter1, insertBefore1); + + if (insertBefore1 !== insertBefore2) { + // The insertion positions are different, so we need to clone the update and + // insert the clone into the alternate queue. + const update2 = cloneUpdate(update); insertUpdateIntoQueue(queue2, update2, insertAfter2, insertBefore2); + } else { + // The insertion positions are the same, so when we inserted into the first + // queue, it also inserted into the alternate. All we need to do is update + // the alternate queue's `first` and `last` pointers, in case they + // have changed. + if (!insertAfter2) { + queue2.first = update; + } + if (!insertBefore2) { + queue2.last = null; + } } } From 7b6c94c767d9825d37f71b9b0a3aa1d746ed3ada Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 15 Dec 2016 18:26:12 -0800 Subject: [PATCH 4/4] Don't drop updates from queue when calling replaceState We should be able to support passing a function to replaceState, which receives the accumulation of all the previously applied state updates. Which means we shouldn't drop those updates from the queue. Technically, we could drop them only when an object is passed to replaceState, but that seems like more trouble than it's worth. --- scripts/fiber/tests-passing.txt | 1 + .../shared/fiber/ReactFiberUpdateQueue.js | 34 +------------------ .../__tests__/ReactIncrementalUpdates-test.js | 28 ++++++++++++--- .../ReactCompositeComponentState-test.js | 14 +++++++- 4 files changed, 38 insertions(+), 39 deletions(-) diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 5b821222ca214..78ed9f2cdf4c7 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -1236,6 +1236,7 @@ src/renderers/shared/fiber/__tests__/ReactIncrementalUpdates-test.js * only drops updates with equal or lesser priority when replaceState is called * can abort an update, schedule additional updates, and resume * can abort an update, schedule a replaceState, and resume +* passes accumulation of previous updates to replaceState updater function * does not call callbacks that are scheduled by another callback until a later commit * gives setState during reconciliation the same priority as whatever level is currently reconciling * enqueues setState inside an updater function as if the in-progress update is progressed (and warns) diff --git a/src/renderers/shared/fiber/ReactFiberUpdateQueue.js b/src/renderers/shared/fiber/ReactFiberUpdateQueue.js index 721ff0259cbc4..a251d3c506a33 100644 --- a/src/renderers/shared/fiber/ReactFiberUpdateQueue.js +++ b/src/renderers/shared/fiber/ReactFiberUpdateQueue.js @@ -309,36 +309,6 @@ function addReplaceUpdate( next: null, }; - // Drop all updates with equal priority - let queue = ensureUpdateQueue(fiber); - for (let i = 0; queue && i < 2; i++) { - let replaceAfter = null; - let replaceBefore = queue.first; - let comparison = 255; - while (replaceBefore && - (comparison = comparePriority(replaceBefore.priorityLevel, priorityLevel)) <= 0) { - if (comparison < 0) { - replaceAfter = replaceBefore; - } - replaceBefore = replaceBefore.next; - } - - if (replaceAfter) { - replaceAfter.next = replaceBefore; - } else { - queue.first = replaceBefore; - } - - if (!replaceBefore) { - queue.last = replaceAfter; - } - - if (fiber.alternate) { - queue = ensureUpdateQueue(fiber.alternate); - } else { - queue = null; - } - } if (__DEV__) { insertUpdate(fiber, update, 'replaceState'); } else { @@ -426,9 +396,7 @@ function beginUpdateQueue( let partialState; if (update.isReplace) { - // A replace should drop all previous updates in the queue, so - // use the original `prevState`, not the accumulated `state` - state = getStateFromUpdate(update, instance, prevState, props); + state = getStateFromUpdate(update, instance, state, props); dontMutatePrevState = true; } else { partialState = getStateFromUpdate(update, instance, state, props); diff --git a/src/renderers/shared/fiber/__tests__/ReactIncrementalUpdates-test.js b/src/renderers/shared/fiber/__tests__/ReactIncrementalUpdates-test.js index 1b6627e2eea8e..73f97d3db1aa0 100644 --- a/src/renderers/shared/fiber/__tests__/ReactIncrementalUpdates-test.js +++ b/src/renderers/shared/fiber/__tests__/ReactIncrementalUpdates-test.js @@ -203,9 +203,7 @@ describe('ReactIncrementalUpdates', () => { render() { ops.push('render'); instance = this; - return ( - - ); + return ; }, }); @@ -244,11 +242,31 @@ describe('ReactIncrementalUpdates', () => { instance.setState(createUpdate('g')); ReactNoop.flush(); - // Ensure that updater function d is never called. - expect(progressedUpdates).toEqual(['e', 'f', 'g']); + expect(progressedUpdates).toEqual(['d', 'e', 'f', 'g']); expect(instance.state).toEqual({ e: 'e', f: 'f', g: 'g' }); }); + it('passes accumulation of previous updates to replaceState updater function', () => { + let instance; + const Foo = React.createClass({ + getInitialState() { + return {}; + }, + render() { + instance = this; + return ; + }, + }); + ReactNoop.render(); + ReactNoop.flush(); + + instance.setState({ a: 'a' }); + instance.setState({ b: 'b' }); + instance.replaceState(previousState => ({ previousState })); + ReactNoop.flush(); + expect(instance.state).toEqual({ previousState: { a: 'a', b : 'b' } }); + }); + it('does not call callbacks that are scheduled by another callback until a later commit', () => { let ops = []; class Foo extends React.Component { diff --git a/src/renderers/shared/shared/__tests__/ReactCompositeComponentState-test.js b/src/renderers/shared/shared/__tests__/ReactCompositeComponentState-test.js index 7f8fe7a4c80f5..781ab0132b668 100644 --- a/src/renderers/shared/shared/__tests__/ReactCompositeComponentState-test.js +++ b/src/renderers/shared/shared/__tests__/ReactCompositeComponentState-test.js @@ -193,7 +193,19 @@ describe('ReactCompositeComponent-state', () => { // setState({color:'green'}) only enqueues a pending state. ['componentWillReceiveProps-end', 'yellow'], // pending state queue is processed - // before-setState-receiveProps never called, due to replaceState. + ); + + if (ReactDOMFeatureFlags.useFiber) { + // In Stack, this is never called because replaceState drops all updates + // from the queue. In Fiber, we keep updates in the queue to support + // replaceState(prevState => newState). + // TODO: Fix Stack to match Fiber. + expected.push( + ['before-setState-receiveProps', 'yellow'], + ); + } + + expected.push( ['before-setState-again-receiveProps', undefined], ['after-setState-receiveProps', 'green'], ['shouldComponentUpdate-currentState', 'yellow'],