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.
Andrew Clark committed
Aug 6, 2016 at 16:47 UTC
0ca1cea26ae66fa26973c4f3912da7e7b8ed47f5
4 files changed
+49
-10
src/renderers/shared/fiber/ReactFiberCommitWork.js
+7
-3
@@ -32,12 +32,16 @@ module.exports = function<T, P, I, C>(config : HostConfig<T, P, I, C>) {
32
function commitWork(current : ?Fiber, finishedWork : Fiber) : void {
33
switch (finishedWork.tag) {
34
case ClassComponent: {
35
+ // Clear updates from current fiber. This must go before the callbacks
36
+ // are reset, in case an update is triggered from inside a callback. Is
37
+ // this safe? Relies on the assumption that work is only committed if
38
+ // the update queue is empty.
39
+ if (finishedWork.alternate) {
40
+ finishedWork.alternate.updateQueue = null;
41
+ }
42
if (finishedWork.callbackList) {
43
const { callbackList } = finishedWork;
44
finishedWork.callbackList = null;
38
- if (finishedWork.alternate) {
39
- finishedWork.alternate.callbackList = null;
40
- }
45
callCallbacks(callbackList, finishedWork.stateNode);
46
}
47
// TODO: Fire componentDidMount/componentDidUpdate, update refs
src/renderers/shared/fiber/ReactFiberCompleteWork.js
+4
-4
@@ -132,14 +132,14 @@ module.exports = function<T, P, I, C>(config : HostConfig<T, P, I, C>) {
132
transferOutput(workInProgress.child, workInProgress);
133
// Don't use the state queue to compute the memoized state. We already
134
// merged it and assigned it to the instance. Transfer it from there.
135
- const state = workInProgress.stateNode.state;
135
+ // Also need to transfer the props, because pendingProps will be null
136
+ // in the case of an update
137
+ const { state, props } = workInProgress.stateNode;
138
workInProgress.memoizedState = state;
139
+ workInProgress.memoizedProps = props;
140
// Transfer update queue to callbackList field so callbacks can be
141
// called during commit phase.
142
workInProgress.callbackList = workInProgress.updateQueue;
140
- if (current) {
141
- current.callbackList = workInProgress.callbackList;
142
- }
143
markForPostEffect(workInProgress);
144
return null;
145
case HostContainer:
src/renderers/shared/fiber/ReactFiberScheduler.js
-3
@@ -144,9 +144,6 @@ module.exports = function<T, P, I, C>(config : HostConfig<T, P, I, C>) {
144
// to the system not to redo any work here.
145
workInProgress.pendingProps = null;
146
workInProgress.updateQueue = null;
147
- if (current) {
148
- current.updateQueue = null;
149
- }
147
148
const returnFiber = workInProgress.return;
149
src/renderers/shared/fiber/__tests__/ReactIncremental-test.js
+38
@@ -654,6 +654,44 @@ describe('ReactIncremental', () => {
654
expect(instance.state.num).toEqual(6);
655
});
656
657
+ it('can call setState inside update callback', () => {
658
+ let instance;
659
+ class Bar extends React.Component {
660
+ constructor() {
661
+ super();
662
+ this.state = { num: 1 };
663
+ instance = this;
664
+ }
665
+ render() {
666
+ return <div>{this.props.children}</div>;
667
+ }
668
+ }
669
+
670
+ function Foo({ multiplier }) {
671
+ return (
672
+ <div>
673
+ <Bar multiplier={multiplier} />
674
+ </div>
675
+ );
676
+ }
677
+
678
+ function updater(state, props) {
679
+ return { num: state.num * props.multiplier };
680
+ }
681
+
682
+ function callback() {
683
+ this.setState({ called: true });
684
+ }
685
+
686
+ ReactNoop.render(<Foo multiplier={2} />);
687
+ ReactNoop.flush();
688
+ instance.setState(updater);
689
+ instance.setState(updater, callback);
690
+ ReactNoop.flush();
691
+ expect(instance.state.num).toEqual(4);
692
+ expect(instance.state.called).toEqual(true);
693
+ });
694
+
695
it('can replaceState', () => {
696
let instance;
697
const Bar = React.createClass({