Skip to content

Commit

Permalink
Merge pull request #8028 from sebmarkbage/nodidupdateforbailout
Browse files Browse the repository at this point in the history
[Fiber] Don't call componentDidUpdate if shouldComponentUpdate returns false
  • Loading branch information
sebmarkbage authored Oct 25, 2016
2 parents 3d48139 + 46bde58 commit 51e1937
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 @@ -1281,4 +1281,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 componentWillUpdate
// 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 51e1937

Please sign in to comment.