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

Improve warning for invalid class contextType #15142

Merged
merged 3 commits into from
Mar 19, 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 82 additions & 0 deletions packages/react-dom/src/__tests__/ReactServerRendering-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -943,4 +943,86 @@ describe('ReactDOMServer', () => {
{withoutStack: true},
);
});

it('should not warn when class contextType is null', () => {
class Foo extends React.Component {
static contextType = null; // Handy for conditional declaration
render() {
return this.context.hello.world;
}
}

expect(() => {
ReactDOMServer.renderToString(<Foo />);
}).toThrow("Cannot read property 'world' of undefined");
});

it('should warn when class contextType is undefined', () => {
class Foo extends React.Component {
// This commonly happens with circular deps
// https://github.com/facebook/react/issues/13969
static contextType = undefined;
render() {
return this.context.hello.world;
}
}

expect(() => {
expect(() => {
ReactDOMServer.renderToString(<Foo />);
}).toThrow("Cannot read property 'world' of undefined");
}).toWarnDev(
'Foo defines an invalid contextType. ' +
'contextType should point to the Context object returned by React.createContext(). ' +
'However, it is set to undefined. ' +
'This can be caused by a typo or by mixing up named and default imports. ' +
'This can also happen due to a circular dependency, ' +
'so try moving the createContext() call to a separate file.',
{withoutStack: true},
);
});

it('should warn when class contextType is an object', () => {
class Foo extends React.Component {
// Can happen due to a typo
static contextType = {
x: 42,
y: 'hello',
};
render() {
return this.context.hello.world;
}
}

expect(() => {
expect(() => {
ReactDOMServer.renderToString(<Foo />);
}).toThrow("Cannot read property 'hello' of undefined");
}).toWarnDev(
'Foo defines an invalid contextType. ' +
'contextType should point to the Context object returned by React.createContext(). ' +
'However, it is set to an object with keys {x, y}.',
{withoutStack: true},
);
});

it('should warn when class contextType is a primitive', () => {
class Foo extends React.Component {
static contextType = 'foo';
render() {
return this.context.hello.world;
}
}

expect(() => {
expect(() => {
ReactDOMServer.renderToString(<Foo />);
}).toThrow("Cannot read property 'world' of undefined");
}).toWarnDev(
'Foo defines an invalid contextType. ' +
'contextType should point to the Context object returned by React.createContext(). ' +
'However, it is set to a string.',
{withoutStack: true},
);
});
});
63 changes: 43 additions & 20 deletions packages/react-dom/src/server/ReactPartialRendererContext.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,19 +10,19 @@
import type {ThreadID} from './ReactThreadIDAllocator';
import type {ReactContext} from 'shared/ReactTypes';

import {REACT_CONTEXT_TYPE} from 'shared/ReactSymbols';
import {REACT_CONTEXT_TYPE, REACT_PROVIDER_TYPE} from 'shared/ReactSymbols';
import ReactSharedInternals from 'shared/ReactSharedInternals';
import getComponentName from 'shared/getComponentName';
import warningWithoutStack from 'shared/warningWithoutStack';
import checkPropTypes from 'prop-types/checkPropTypes';

let ReactDebugCurrentFrame;
let didWarnAboutInvalidateContextType;
if (__DEV__) {
ReactDebugCurrentFrame = ReactSharedInternals.ReactDebugCurrentFrame;
didWarnAboutInvalidateContextType = new Set();
}

const didWarnAboutInvalidateContextType = {};

export const emptyObject = {};
if (__DEV__) {
Object.freeze(emptyObject);
Expand Down Expand Up @@ -75,26 +75,49 @@ export function processContext(
threadID: ThreadID,
) {
const contextType = type.contextType;
if (typeof contextType === 'object' && contextType !== null) {
if (__DEV__) {
const isContextConsumer =
contextType.$$typeof === REACT_CONTEXT_TYPE &&
contextType._context !== undefined;
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.%s instead?',
name,
isContextConsumer ? 'Consumer' : 'Provider',
);
if (__DEV__) {
if ('contextType' in (type: any)) {
let isValid =
// Allow null for conditional declaration
contextType === null ||
(contextType !== undefined &&
contextType.$$typeof === REACT_CONTEXT_TYPE &&
contextType._context === undefined); // Not a <Context.Consumer>

if (!isValid && !didWarnAboutInvalidateContextType.has(type)) {
didWarnAboutInvalidateContextType.add(type);

let addendum = '';
if (contextType === undefined) {
addendum =
' However, it is set to undefined. ' +
'This can be caused by a typo or by mixing up named and default imports. ' +
'This can also happen due to a circular dependency, so ' +
'try moving the createContext() call to a separate file.';
} else if (typeof contextType !== 'object') {
addendum = ' However, it is set to a ' + typeof contextType + '.';
} else if (contextType.$$typeof === REACT_PROVIDER_TYPE) {
addendum = ' Did you accidentally pass the Context.Provider instead?';
} else if (contextType._context !== undefined) {
// <Context.Consumer>
addendum = ' Did you accidentally pass the Context.Consumer instead?';
} else {
addendum =
' However, it is set to an object with keys {' +
Object.keys(contextType).join(', ') +
'}.';
}
warningWithoutStack(
false,
'%s defines an invalid contextType. ' +
'contextType should point to the Context object returned by React.createContext().%s',
getComponentName(type) || 'Component',
addendum,
);
}
}
}
if (typeof contextType === 'object' && contextType !== null) {
validateContextBounds(contextType, threadID);
return contextType[threadID];
} else {
Expand Down
50 changes: 37 additions & 13 deletions packages/react-reconciler/src/ReactFiberClassComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import shallowEqual from 'shared/shallowEqual';
import getComponentName from 'shared/getComponentName';
import invariant from 'shared/invariant';
import warningWithoutStack from 'shared/warningWithoutStack';
import {REACT_CONTEXT_TYPE} from 'shared/ReactSymbols';
import {REACT_CONTEXT_TYPE, REACT_PROVIDER_TYPE} from 'shared/ReactSymbols';

import {startPhaseTimer, stopPhaseTimer} from './ReactDebugFiberPerf';
import {resolveDefaultProps} from './ReactFiberLazyComponent';
Expand Down Expand Up @@ -513,27 +513,51 @@ function constructClassInstance(
let unmaskedContext = emptyContextObject;
let context = null;
const contextType = ctor.contextType;
if (typeof contextType === 'object' && contextType !== null) {
if (__DEV__) {
const isContextConsumer =
contextType.$$typeof === REACT_CONTEXT_TYPE &&
contextType._context !== undefined;
if (
(contextType.$$typeof !== REACT_CONTEXT_TYPE || isContextConsumer) &&
!didWarnAboutInvalidateContextType.has(ctor)
) {

if (__DEV__) {
if ('contextType' in ctor) {
let isValid =
// Allow null for conditional declaration
contextType === null ||
(contextType !== undefined &&
contextType.$$typeof === REACT_CONTEXT_TYPE &&
contextType._context === undefined); // Not a <Context.Consumer>

if (!isValid && !didWarnAboutInvalidateContextType.has(ctor)) {
didWarnAboutInvalidateContextType.add(ctor);

let addendum = '';
if (contextType === undefined) {
addendum =
' However, it is set to undefined. ' +
'This can be caused by a typo or by mixing up named and default imports. ' +
'This can also happen due to a circular dependency, so ' +
'try moving the createContext() call to a separate file.';
} else if (typeof contextType !== 'object') {
addendum = ' However, it is set to a ' + typeof contextType + '.';
} else if (contextType.$$typeof === REACT_PROVIDER_TYPE) {
addendum = ' Did you accidentally pass the Context.Provider instead?';
} else if (contextType._context !== undefined) {
// <Context.Consumer>
addendum = ' Did you accidentally pass the Context.Consumer instead?';
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add another branch for arrays?

else if (Array.isArray(contextType)) {
  addendum = 'However, it is set to an array.'
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Seems very unlikely to happen

Copy link
Contributor

Choose a reason for hiding this comment

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

🤷‍♂️ seems like it could happen if you import the wrong value. I figured that if it happens it would be weird to see.

However, it is set to an object with keys {0, 1, 2, 3, 4, 5, 6...100000}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Arrays are pretty rare as export values so I don't think it's worth handling.

addendum =
' However, it is set to an object with keys {' +
Object.keys(contextType).join(', ') +
'}.';
}
warningWithoutStack(
false,
'%s defines an invalid contextType. ' +
'contextType should point to the Context object returned by React.createContext(). ' +
'Did you accidentally pass the Context.%s instead?',
'contextType should point to the Context object returned by React.createContext().%s',
getComponentName(ctor) || 'Component',
isContextConsumer ? 'Consumer' : 'Provider',
addendum,
);
}
}
}

if (typeof contextType === 'object' && contextType !== null) {
context = readContext((contextType: any));
} else {
unmaskedContext = getUnmaskedContext(workInProgress, ctor, true);
Expand Down
81 changes: 81 additions & 0 deletions packages/react/src/__tests__/ReactContextValidator-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -578,6 +578,87 @@ describe('ReactContextValidator', () => {
);
});

it('should not warn when class contextType is null', () => {
class Foo extends React.Component {
static contextType = null; // Handy for conditional declaration
render() {
return this.context.hello.world;
}
}
expect(() => {
ReactTestUtils.renderIntoDocument(<Foo />);
}).toThrow("Cannot read property 'world' of undefined");
});

it('should warn when class contextType is undefined', () => {
class Foo extends React.Component {
// This commonly happens with circular deps
// https://github.com/facebook/react/issues/13969
static contextType = undefined;
render() {
return this.context.hello.world;
}
}

expect(() => {
expect(() => {
ReactTestUtils.renderIntoDocument(<Foo />);
}).toThrow("Cannot read property 'world' of undefined");
}).toWarnDev(
'Foo defines an invalid contextType. ' +
'contextType should point to the Context object returned by React.createContext(). ' +
'However, it is set to undefined. ' +
'This can be caused by a typo or by mixing up named and default imports. ' +
'This can also happen due to a circular dependency, ' +
'so try moving the createContext() call to a separate file.',
{withoutStack: true},
);
});

it('should warn when class contextType is an object', () => {
class Foo extends React.Component {
// Can happen due to a typo
static contextType = {
x: 42,
y: 'hello',
};
render() {
return this.context.hello.world;
}
}

expect(() => {
expect(() => {
ReactTestUtils.renderIntoDocument(<Foo />);
}).toThrow("Cannot read property 'hello' of undefined");
}).toWarnDev(
'Foo defines an invalid contextType. ' +
'contextType should point to the Context object returned by React.createContext(). ' +
'However, it is set to an object with keys {x, y}.',
{withoutStack: true},
);
});

it('should warn when class contextType is a primitive', () => {
class Foo extends React.Component {
static contextType = 'foo';
render() {
return this.context.hello.world;
}
}

expect(() => {
expect(() => {
ReactTestUtils.renderIntoDocument(<Foo />);
}).toThrow("Cannot read property 'world' of undefined");
}).toWarnDev(
'Foo defines an invalid contextType. ' +
'contextType should point to the Context object returned by React.createContext(). ' +
'However, it is set to a string.',
{withoutStack: true},
);
});

it('should warn if you define contextType on a function component', () => {
const Context = React.createContext();

Expand Down