Skip to content

Commit

Permalink
Don't call componentDidUpdate if shouldComponentUpdate returns false
Browse files Browse the repository at this point in the history
This fix relies on the props and state objects being different
to know if we can avoid a componentDidUpdate. This is not a great
solution because we actually want to use the new props/state
object even if sCU returns false. That's the current semantics
and it can actually be important because new rerenders that are
able to reuse props objects are more likely to have the new props
object so we won't be able to quickly bail out next time.
I don't have a better idea atm though.
  • Loading branch information
sebmarkbage committed Oct 21, 2016
1 parent 8cac523 commit 2b3b7c6
Show file tree
Hide file tree
Showing 2 changed files with 107 additions and 1 deletion.
9 changes: 8 additions & 1 deletion src/renderers/shared/fiber/ReactFiberCompleteWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,14 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
// Transfer update queue to callbackList field so callbacks can be
// called during commit phase.
workInProgress.callbackList = workInProgress.updateQueue;
markUpdate(workInProgress);
if (current) {
if (current.memoizedProps !== workInProgress.memoizedProps ||
current.memoizedState !== workInProgress.memoizedState) {
markUpdate(workInProgress);
}
} else {
markUpdate(workInProgress);
}
return null;
case HostContainer:
transferOutput(workInProgress.child, workInProgress);
Expand Down
99 changes: 99 additions & 0 deletions src/renderers/shared/fiber/__tests__/ReactIncremental-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1234,4 +1234,103 @@ describe('ReactIncremental', () => {

});

it('skips will/DidUpdate when bailing unless an update was already in progress', () => {
var ops = [];

class LifeCycle extends React.Component {
componentWillMount() {
ops.push('componentWillMount');
}
componentDidMount() {
ops.push('componentDidMount');
}
componentWillReceiveProps(nextProps) {
ops.push('componentWillReceiveProps');
}
shouldComponentUpdate(nextProps) {
ops.push('shouldComponentUpdate');
// Bail
return this.props.x !== nextProps.x;
}
componentWillUpdate(nextProps) {
ops.push('componentWillUpdate');
}
componentDidUpdate(prevProps) {
ops.push('componentDidUpdate');
}
render() {
ops.push('render');
return <span />;
}
}

function Sibling() {
ops.push('render sibling');
return <span />;
}

function App(props) {
return [
<LifeCycle x={props.x} />,
<Sibling />,
];
}

ReactNoop.render(<App x={0} />);
ReactNoop.flush();

expect(ops).toEqual([
'componentWillMount',
'render',
'render sibling',
'componentDidMount',
]);

ops = [];

// Update to same props
ReactNoop.render(<App x={0} />);
ReactNoop.flush();

expect(ops).toEqual([
'componentWillReceiveProps',
'shouldComponentUpdate',
// no componenWillUpdate
// no render
'render sibling',
// no componentDidUpdate
]);

ops = [];

// Begin updating to new props...
ReactNoop.render(<App x={1} />);
ReactNoop.flushDeferredPri(30);

expect(ops).toEqual([
'componentWillReceiveProps',
'shouldComponentUpdate',
'componentWillUpdate',
'render',
'render sibling',
// no componentDidUpdate yet
]);

ops = [];

// ...but we'll interrupt it to rerender the same props.
ReactNoop.render(<App x={1} />);
ReactNoop.flush();

// We can bail out this time, but we must call componentDidUpdate.
expect(ops).toEqual([
'componentWillReceiveProps',
'shouldComponentUpdate',
// no componentWillUpdate
// no render
'render sibling',
'componentDidUpdate',
]);
});

});

0 comments on commit 2b3b7c6

Please sign in to comment.