Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Don't bail out on referential equality of Consumer's props.children function #12470

Merged
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions packages/react-reconciler/src/ReactFiberBeginWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -994,10 +994,10 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
changedBits,
renderExpirationTime,
);
} else if (oldProps !== null && oldProps.children === newProps.children) {
// No change. Bailout early if children are the same.
return bailoutOnAlreadyFinishedWork(current, workInProgress);
}
// There is no bailout on `children` equality because we expect people
// to often pass a bound method as a child, but it may reference
// `this.state` or `this.props` (and thus needs to re-render on `setState`).

const render = newProps.children;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -727,39 +727,103 @@ describe('ReactNewContext', () => {
expect(ReactNoop.getChildren()).toEqual([span('Child')]);
});

it('consumer bails out if children and value are unchanged (like sCU)', () => {
it('consumer bails out if value is unchanged and something above bailed out', () => {
const Context = React.createContext(0);

function Child() {
ReactNoop.yield('Child');
return <span prop="Child" />;
function renderChildValue(value) {
ReactNoop.yield('Consumer');
return <span prop={value} />
}

function renderConsumer(context) {
return <Child context={context} />;
function ChildWithInlineRenderCallback() {
ReactNoop.yield('ChildWithInlineRenderCallback');
// Note: we are intentionally passing an inline arrow. Don't refactor.
return <Context.Consumer>{value => renderChildValue(value)}</Context.Consumer>;
}

function App(props) {
ReactNoop.yield('App');
return (
<Context.Provider value={props.value}>
<Context.Consumer>{renderConsumer}</Context.Consumer>
</Context.Provider>
);
function ChildWithCachedRenderCallback() {
ReactNoop.yield('ChildWithCachedRenderCallback');
return <Context.Consumer>{renderChildValue}</Context.Consumer>;
}

class PureIndirection extends React.PureComponent {
render() {
ReactNoop.yield('PureIndirection');
return (
<React.Fragment>
<ChildWithInlineRenderCallback />
<ChildWithCachedRenderCallback />
</React.Fragment>
);
}
}

class App extends React.Component {
render() {
ReactNoop.yield('App');
return (
<Context.Provider value={this.props.value}>
<PureIndirection />
</Context.Provider>
);
}
}

// Initial mount
ReactNoop.render(<App value={1} />);
expect(ReactNoop.flush()).toEqual(['App', 'Child']);
expect(ReactNoop.getChildren()).toEqual([span('Child')]);
let inst;
ReactNoop.render(<App value={1} ref={ref => (inst = ref)} />);
expect(ReactNoop.flush()).toEqual(['App', 'PureIndirection', 'ChildWithInlineRenderCallback', 'Consumer', 'ChildWithCachedRenderCallback', 'Consumer']);
expect(ReactNoop.getChildren()).toEqual([span(1), span(1)]);

// Update (bailout)
ReactNoop.render(<App value={1} ref={ref => (inst = ref)} />);
expect(ReactNoop.flush()).toEqual(['App']);
expect(ReactNoop.getChildren()).toEqual([span(1), span(1)]);

// Update (no bailout)
ReactNoop.render(<App value={2} ref={ref => (inst = ref)} />);
expect(ReactNoop.flush()).toEqual(['App', 'Consumer', 'Consumer']);
expect(ReactNoop.getChildren()).toEqual([span(2), span(2)]);
});

// Context consumer bails out on propagating "deep" updates when `value` hasn't changed.
// However, it doesn't bail out from rendering if the component above it re-rendered anyway.
// If we bailed out on referential equality, it would be confusing that you
// can call this.setState(), but an autobound render callback "blocked" the update.
// https://github.com/facebook/react/pull/12470#issuecomment-376917711
it('consumer does not bail out if there were no bailouts above it', () => {
const Context = React.createContext(0);

class App extends React.Component {
state = {
text: 'hello',
};

renderConsumer = context => {
ReactNoop.yield('App#renderConsumer');
return <span prop={this.state.text} />;
};

render() {
ReactNoop.yield('App');
return (
<Context.Provider value={this.props.value}>
<Context.Consumer>{this.renderConsumer}</Context.Consumer>
</Context.Provider>
);
}
}

// Initial mount
let inst;
ReactNoop.render(<App value={1} ref={ref => (inst = ref)} />);
expect(ReactNoop.flush()).toEqual(['App', 'App#renderConsumer']);
expect(ReactNoop.getChildren()).toEqual([span('hello')]);

// Update
ReactNoop.render(<App value={1} />);
expect(ReactNoop.flush()).toEqual([
'App',
// Child does not re-render
]);
expect(ReactNoop.getChildren()).toEqual([span('Child')]);
inst.setState({text: 'goodbye'});
expect(ReactNoop.flush()).toEqual(['App', 'App#renderConsumer']);
expect(ReactNoop.getChildren()).toEqual([span('goodbye')]);
});

// This is a regression case for https://github.com/facebook/react/issues/12389.
Expand Down