Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Don't bail on new context Provider if a legacy provider rendered above #12586

Merged
merged 2 commits into from
Apr 26, 2018
Merged

Don't bail on new context Provider if a legacy provider rendered above #12586

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9f1964c
Don't bail on new context Provider if a legacy provider rendered above
gaearon Apr 9, 2018
21f72a0
Avoid an extra variable
gaearon Apr 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 5 additions & 3 deletions packages/react-reconciler/src/ReactFiberBeginWork.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -859,8 +859,10 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(

const newProps = workInProgress.pendingProps;
const oldProps = workInProgress.memoizedProps;
let canBailOnProps = true;

if (hasLegacyContextChanged()) {
canBailOnProps = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this could be disabled on StrictMode?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't disable these bailouts in strict mode for other components so we probably shouldn't here either.

// Normally we can bail out on props equality but if context has changed
// we don't do the bailout and we have to reuse existing props instead.
} else if (oldProps === newProps) {
Expand Down Expand Up @@ -893,7 +895,7 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
} else {
if (oldProps.value === newProps.value) {
// No change. Bailout early if children are the same.
if (oldProps.children === newProps.children) {
if (oldProps.children === newProps.children && canBailOnProps) {
workInProgress.stateNode = 0;
pushProvider(workInProgress);
return bailoutOnAlreadyFinishedWork(current, workInProgress);
Expand All @@ -910,7 +912,7 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
(oldValue !== oldValue && newValue !== newValue) // eslint-disable-line no-self-compare
) {
// No change. Bailout early if children are the same.
if (oldProps.children === newProps.children) {
if (oldProps.children === newProps.children && canBailOnProps) {
workInProgress.stateNode = 0;
pushProvider(workInProgress);
return bailoutOnAlreadyFinishedWork(current, workInProgress);
Expand All @@ -933,7 +935,7 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(

if (changedBits === 0) {
// No change. Bailout early if children are the same.
if (oldProps.children === newProps.children) {
if (oldProps.children === newProps.children && canBailOnProps) {
workInProgress.stateNode = 0;
pushProvider(workInProgress);
return bailoutOnAlreadyFinishedWork(current, workInProgress);
Expand Down
66 changes: 66 additions & 0 deletions packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -847,6 +847,72 @@ describe('ReactNewContext', () => {
expect(ReactNoop.getChildren()).toEqual([span('Child')]);
});

it('provider does not bail out if legacy context changed above', () => {
const Context = React.createContext(0);

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

const children = <Child />;

class LegacyProvider extends React.Component {
static childContextTypes = {
legacyValue: () => {},
};
state = {legacyValue: 1};
getChildContext() {
return {legacyValue: this.state.legacyValue};
}
render() {
ReactNoop.yield('LegacyProvider');
return this.props.children;
}
}

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

const legacyProviderRef = React.createRef();
const appRef = React.createRef();

// Initial mount
ReactNoop.render(
<LegacyProvider ref={legacyProviderRef}>
<App ref={appRef} value={1}>
{children}
</App>
</LegacyProvider>,
);
expect(ReactNoop.flush()).toEqual(['LegacyProvider', 'App', 'Child']);
expect(ReactNoop.getChildren()).toEqual([span('Child')]);

// Update App with same value (should bail out)
appRef.current.setState({value: 1});
expect(ReactNoop.flush()).toEqual(['App']);
expect(ReactNoop.getChildren()).toEqual([span('Child')]);

// Update LegacyProvider (should not bail out)
legacyProviderRef.current.setState({value: 1});
expect(ReactNoop.flush()).toEqual(['LegacyProvider', 'App', 'Child']);
expect(ReactNoop.getChildren()).toEqual([span('Child')]);

// Update App with same value (should bail out)
appRef.current.setState({value: 1});
expect(ReactNoop.flush()).toEqual(['App']);
expect(ReactNoop.getChildren()).toEqual([span('Child')]);
});

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

Expand Down