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

Warn for Context.Consumer with contextType #14831

Merged
merged 1 commit into from Mar 18, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
39 changes: 39 additions & 0 deletions packages/react-dom/src/__tests__/ReactServerRendering-test.js
Expand Up @@ -904,4 +904,43 @@ describe('ReactDOMServer', () => {
' in App (at **)',
]);
});

it('should warn if an invalid contextType is defined', () => {
const Context = React.createContext();

class ComponentA extends React.Component {
// It should warn for both Context.Consumer and Context.Provider
static contextType = Context.Consumer;
render() {
return <div />;
}
}
class ComponentB extends React.Component {
static contextType = Context.Provider;
render() {
return <div />;
}
}

expect(() => {
ReactDOMServer.renderToString(<ComponentA />);
}).toWarnDev(
'Warning: ComponentA defines an invalid contextType. ' +
'contextType should point to the Context object returned by React.createContext(). ' +
'Did you accidentally pass the Context.Consumer instead?',
{withoutStack: true},
);

// Warnings should be deduped by component type
ReactDOMServer.renderToString(<ComponentA />);

expect(() => {
ReactDOMServer.renderToString(<ComponentB />);
}).toWarnDev(
'Warning: ComponentB defines an invalid contextType. ' +
'contextType should point to the Context object returned by React.createContext(). ' +
'Did you accidentally pass the Context.Provider instead?',
{withoutStack: true},
);
});
});
8 changes: 6 additions & 2 deletions packages/react-dom/src/server/ReactPartialRendererContext.js
Expand Up @@ -77,16 +77,20 @@ export function processContext(
const contextType = type.contextType;
if (typeof contextType === 'object' && contextType !== null) {
if (__DEV__) {
if (contextType.$$typeof !== REACT_CONTEXT_TYPE) {
const isContextConsumer =
contextType.$$typeof === REACT_CONTEXT_TYPE &&
contextType._context !== undefined;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was it also called context in some patch release?

Copy link
Contributor Author

@aweary aweary Feb 12, 2019

Choose a reason for hiding this comment

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

Yeah, it was changed to _context in #12501. Is it worth checking? I think a lot more would be broken if you were trying to render a tree created with a version of React that still used context since we don't check for it in the context code paths.

if (contextType.$$typeof !== REACT_CONTEXT_TYPE || isContextConsumer) {
let name = getComponentName(type) || 'Component';
if (!didWarnAboutInvalidateContextType[name]) {
didWarnAboutInvalidateContextType[name] = true;
warningWithoutStack(
false,
'%s defines an invalid contextType. ' +
'contextType should point to the Context object returned by React.createContext(). ' +
'Did you accidentally pass the Context.Provider instead?',
'Did you accidentally pass the Context.%s instead?',
name,
isContextConsumer ? 'Consumer' : 'Provider',
);
}
}
Expand Down
8 changes: 6 additions & 2 deletions packages/react-reconciler/src/ReactFiberClassComponent.js
Expand Up @@ -515,17 +515,21 @@ function constructClassInstance(
const contextType = ctor.contextType;
if (typeof contextType === 'object' && contextType !== null) {
if (__DEV__) {
const isContextConsumer =
contextType.$$typeof === REACT_CONTEXT_TYPE &&
contextType._context !== undefined;
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 context value actually is undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't the context value tracked as _currentValue on this _context element?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh right.

if (
contextType.$$typeof !== REACT_CONTEXT_TYPE &&
(contextType.$$typeof !== REACT_CONTEXT_TYPE || isContextConsumer) &&
!didWarnAboutInvalidateContextType.has(ctor)
) {
didWarnAboutInvalidateContextType.add(ctor);
warningWithoutStack(
false,
'%s defines an invalid contextType. ' +
'contextType should point to the Context object returned by React.createContext(). ' +
'Did you accidentally pass the Context.Provider instead?',
'Did you accidentally pass the Context.%s instead?',
getComponentName(ctor) || 'Component',
isContextConsumer ? 'Consumer' : 'Provider',
);
}
}
Expand Down
7 changes: 4 additions & 3 deletions packages/react/src/__tests__/ReactContextValidator-test.js
Expand Up @@ -541,9 +541,10 @@ describe('ReactContextValidator', () => {

it('should warn if an invalid contextType is defined', () => {
const Context = React.createContext();

// This tests that both Context.Consumer and Context.Provider
// warn about invalid contextType.
class ComponentA extends React.Component {
static contextType = Context.Provider;
static contextType = Context.Consumer;
render() {
return <div />;
}
Expand All @@ -560,7 +561,7 @@ describe('ReactContextValidator', () => {
}).toWarnDev(
'Warning: ComponentA defines an invalid contextType. ' +
'contextType should point to the Context object returned by React.createContext(). ' +
'Did you accidentally pass the Context.Provider instead?',
'Did you accidentally pass the Context.Consumer instead?',
{withoutStack: true},
);

Expand Down