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.

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

Deprecate context object as a consumer and add a warning message #13829

Merged
merged 14 commits into from
Oct 12, 2018
25 changes: 24 additions & 1 deletion packages/react-reconciler/src/ReactFiberBeginWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -1096,12 +1096,35 @@ function updateContextProvider(
return workInProgress.child;
}

let hasWarnedAboutUsingContextAsConsumer = false;

function updateContextConsumer(
current: Fiber | null,
workInProgress: Fiber,
renderExpirationTime: ExpirationTime,
) {
const context: ReactContext<any> = workInProgress.type;
let context: ReactContext<any> = workInProgress.type;
// The logic below for Context differs depending on PROD or DEV mode. In
// DEV mode, we create a separate object for Context.Consumer that acts
// like a proxy to Context. This proxy object adds unnecessary code in PROD
// so we use the old behaviour (Context.Consumer references Context) to
// reduce size and overhead. The separate object references context via
// a property called "_context", which also gives us the ability to check
// in DEV mode if this property exists or not and warn if it does not.
if (__DEV__) {
if ((context: any)._context === undefined) {
if (!hasWarnedAboutUsingContextAsConsumer) {
hasWarnedAboutUsingContextAsConsumer = true;
warning(
false,
'Rendering <Context> directly is not supported and will be removed in ' +
'a future major release. Did you mean to render <Context.Consumer> instead?',
);
}
} else {
context = (context: any)._context;
}
}
const newProps = workInProgress.pendingProps;
const render = newProps.children;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1190,12 +1190,12 @@ describe('ReactNewContext', () => {

function FooAndBar() {
return (
<FooContext>
<FooContext.Consumer>
{foo => {
const bar = BarContext.unstable_read();
return <Text text={`Foo: ${foo}, Bar: ${bar}`} />;
}}
</FooContext>
</FooContext.Consumer>
);
}

Expand Down Expand Up @@ -1558,4 +1558,29 @@ Context fuzz tester error! Copy and paste the following line into the test suite
}
});
});

it('should warn with an error message when using context as a consumer in DEV', () => {
const BarContext = React.createContext({value: 'bar-initial'});
const BarConsumer = BarContext;

function Component() {
return (
<React.Fragment>
<BarContext.Provider value={{value: 'bar-updated'}}>
<BarConsumer>
{({value}) => <div actual={value} expected="bar-updated" />}
</BarConsumer>
</BarContext.Provider>
</React.Fragment>
);
}

expect(() => {
ReactNoop.render(<Component />);
ReactNoop.flush();
}).toWarnDev(
'Rendering <Context> directly is not supported and will be removed in ' +
'a future major release. Did you mean to render <Context.Consumer> instead?',
);
});
});
56 changes: 55 additions & 1 deletion packages/react/src/ReactContext.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,61 @@ export function createContext<T>(
$$typeof: REACT_PROVIDER_TYPE,
_context: context,
};
context.Consumer = context;

if (__DEV__) {
// A separate object, but proxies back to the original context object for
// backwards compatibility. It has a different $$typeof, so we can properly
// warn for the incorrect usage of Context as a Consumer.
const consumer = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: I'd probably call this Consumer

$$typeof: REACT_CONTEXT_TYPE,
_context: context,
Provider: context.Provider,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's make <Context.Consumer.Provider> warn as well.

};
// $FlowFixMe: Flow complains about not setting a value, which is intentional here
Object.defineProperties(consumer, {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it better to have consumer proxy to context, or the other way around? Why?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Whatever we read/write from/to most. I’m actually not sure which one that is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked and the context gets written to the most in our tests.

_calculateChangedBits: {
Copy link
Collaborator

Choose a reason for hiding this comment

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

_calculateChangedBits and unstable_read never change, can we directly point to the original without getters for them?

get() {
return context._calculateChangedBits;
},
set(_calculateChangedBits) {
context._calculateChangedBits = _calculateChangedBits;
},
},
_currentValue: {
get() {
return context._currentValue;
},
set(_currentValue) {
context._currentValue = _currentValue;
},
},
_currentValue2: {
get() {
return context._currentValue2;
},
set(_currentValue2) {
context._currentValue2 = _currentValue2;
},
},
Consumer: {
get() {
return context.Consumer;
Copy link
Collaborator

Choose a reason for hiding this comment

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

What if you render <Context.Consumer.Consumer>? Seems like that should also warn because it also relies on objects being shared.

Which makes me think: should the warning move into these getters instead? Fire for first getter accessed, ignore the rest. This could also nicely let us warn once per context type.

Copy link
Contributor Author

@trueadm trueadm Oct 12, 2018

Choose a reason for hiding this comment

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

Moving the warning into the getters will stop the error from coming up for just using <Context /> as React's internals will always read/write from the context and never the consumer – the backwards compatibility is there for cases where libraries might try and read/write to the consumer for whatever reason. I'll address the nested Consumer.Consumer issue.

},
},
unstable_read: {
get() {
return context.unstable_read;
},
set(unstable_read) {
context.unstable_read = unstable_read;
},
},
});
// $FlowFixMe: Flow complains about missing properties because it doesn't understand defineProperty
context.Consumer = consumer;
} else {
context.Consumer = context;
}
context.unstable_read = readContext.bind(null, context);

if (__DEV__) {
Expand Down