Skip to content

Commit

Permalink
Fix bug with double updates in a single batch (#6650)
Browse files Browse the repository at this point in the history
Fixes #2410. Fixes #6371. Fixes #6538.

I also manually tested the codepen in #3762 and verified it now works.
  • Loading branch information
sophiebits committed Apr 29, 2016
1 parent 44f8463 commit c1e3f7e
Show file tree
Hide file tree
Showing 4 changed files with 133 additions and 5 deletions.
8 changes: 5 additions & 3 deletions src/renderers/shared/reconciler/ReactCompositeComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,7 @@ var ReactCompositeComponentMixin = {
this._nativeContainerInfo = null;

// See ReactUpdateQueue
this._updateBatchNumber = null;
this._pendingElement = null;
this._pendingStateQueue = null;
this._pendingReplaceState = false;
Expand Down Expand Up @@ -765,16 +766,16 @@ var ReactCompositeComponentMixin = {
transaction,
this._context
);
}

if (this._pendingStateQueue !== null || this._pendingForceUpdate) {
} else if (this._pendingStateQueue !== null || this._pendingForceUpdate) {
this.updateComponent(
transaction,
this._currentElement,
this._currentElement,
this._context,
this._context
);
} else {
this._updateBatchNumber = null;
}
},

Expand Down Expand Up @@ -878,6 +879,7 @@ var ReactCompositeComponentMixin = {
);
}

this._updateBatchNumber = null;
if (shouldUpdate) {
this._pendingForceUpdate = false;
// Will set `this.props`, `this.state` and `this.context`.
Expand Down
18 changes: 17 additions & 1 deletion src/renderers/shared/reconciler/ReactReconciler.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
var ReactRef = require('ReactRef');
var ReactInstrumentation = require('ReactInstrumentation');

var invariant = require('invariant');

/**
* Helper to call ReactRef.attachRefs with this composite component, split out
* to avoid allocations in the transaction mount-ready queue.
Expand Down Expand Up @@ -190,8 +192,22 @@ var ReactReconciler = {
*/
performUpdateIfNecessary: function(
internalInstance,
transaction
transaction,
updateBatchNumber
) {
if (internalInstance._updateBatchNumber !== updateBatchNumber) {
// The component's enqueued batch number should always be the current
// batch or the following one.
invariant(
internalInstance._updateBatchNumber == null ||
internalInstance._updateBatchNumber === updateBatchNumber + 1,
'performUpdateIfNecessary: Unexpected batch number (current %s, ' +
'pending %s)',
updateBatchNumber,
internalInstance._updateBatchNumber
);
return;
}
if (__DEV__) {
if (internalInstance._debugID !== 0) {
ReactInstrumentation.debugTool.onBeginReconcilerTimer(
Expand Down
14 changes: 13 additions & 1 deletion src/renderers/shared/reconciler/ReactUpdates.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ var Transaction = require('Transaction');
var invariant = require('invariant');

var dirtyComponents = [];
var updateBatchNumber = 0;
var asapCallbackQueue = CallbackQueue.getPooled();
var asapEnqueued = false;

Expand Down Expand Up @@ -138,6 +139,13 @@ function runBatchedUpdates(transaction) {
// them before their children by sorting the array.
dirtyComponents.sort(mountOrderComparator);

// Any updates enqueued while reconciling must be performed after this entire
// batch. Otherwise, if dirtyComponents is [A, B] where A has children B and
// C, B could update twice in a single batch if C's render enqueues an update
// to B (since B would have already updated, we should skip it, and the only
// way we can know to do so is by checking the batch counter).
updateBatchNumber++;

for (var i = 0; i < len; i++) {
// If a component is unmounted before pending changes apply, it will still
// be here, but we assume that it has cleared its _pendingCallbacks and
Expand Down Expand Up @@ -166,7 +174,8 @@ function runBatchedUpdates(transaction) {

ReactReconciler.performUpdateIfNecessary(
component,
transaction.reconcileTransaction
transaction.reconcileTransaction,
updateBatchNumber
);

if (markerName) {
Expand Down Expand Up @@ -238,6 +247,9 @@ function enqueueUpdate(component) {
}

dirtyComponents.push(component);
if (component._updateBatchNumber == null) {
component._updateBatchNumber = updateBatchNumber + 1;
}
}

/**
Expand Down
98 changes: 98 additions & 0 deletions src/renderers/shared/reconciler/__tests__/ReactUpdates-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1024,4 +1024,102 @@ describe('ReactUpdates', function() {
'to be a function. Instead received: Foo (keys: a, b).'
);
});

it('does not update one component twice in a batch (#2410)', function() {
var Parent = React.createClass({
getChild: function() {
return this.refs.child;
},
render: function() {
return <Child ref="child" />;
},
});

var renderCount = 0;
var postRenderCount = 0;
var once = false;
var Child = React.createClass({
getInitialState: function() {
return {updated: false};
},
componentWillUpdate: function() {
if (!once) {
once = true;
this.setState({updated: true});
}
},
componentDidMount: function() {
expect(renderCount).toBe(postRenderCount + 1);
postRenderCount++;
},
componentDidUpdate: function() {
expect(renderCount).toBe(postRenderCount + 1);
postRenderCount++;
},
render: function() {
expect(renderCount).toBe(postRenderCount);
renderCount++;
return <div />;
},
});

var parent = ReactTestUtils.renderIntoDocument(<Parent />);
var child = parent.getChild();
ReactDOM.unstable_batchedUpdates(function() {
parent.forceUpdate();
child.forceUpdate();
});
});

it('does not update one component twice in a batch (#6371)', function() {
var callbacks = [];
function emitChange() {
callbacks.forEach(c => c());
}

class App extends React.Component {
constructor(props) {
super(props);
this.state = { showChild: true };
}
componentDidMount() {
console.log('about to remove child via set state');
this.setState({ showChild: false });
}
render() {
return (
<div>
<ForceUpdatesOnChange />
{this.state.showChild && <EmitsChangeOnUnmount />}
</div>
);
}
}

class EmitsChangeOnUnmount extends React.Component {
componentWillUnmount() {
emitChange();
}
render() {
return null;
}
}

class ForceUpdatesOnChange extends React.Component {
componentDidMount() {
this.onChange = () => this.forceUpdate();
this.onChange();
callbacks.push(this.onChange);
}
componentWillUnmount() {
callbacks = callbacks.filter((c) => c !== this.onChange);
}
render() {
return <div key={Math.random()} onClick={function() {}} />;
}
}

ReactDOM.render(<App />, document.createElement('div'));
});

});

0 comments on commit c1e3f7e

Please sign in to comment.