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

Improve warning for invalid class contextType #15142

Merged
merged 3 commits into from
Mar 19, 2019
Merged

Improve warning for invalid class contextType #15142

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c87d77e
Improve warning for invalid class contextType
gaearon Mar 18, 2019
9808cc3
Don't warn for null
gaearon Mar 19, 2019
a123591
Grammar
gaearon Mar 19, 2019
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
89 changes: 89 additions & 0 deletions packages/react-dom/src/__tests__/ReactServerRendering-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -943,4 +943,93 @@ describe('ReactDOMServer', () => {
{withoutStack: true},
);
});

it('should warn when class contextType is null', () => {
class Foo extends React.Component {
static contextType = null;
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 null.',
{withoutStack: true},
);
});

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 string.',
{withoutStack: true},
);
});
});
62 changes: 43 additions & 19 deletions packages/react-dom/src/server/ReactPartialRendererContext.js
View file
Open in desktop
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,50 @@ export function processContext(
threadID: ThreadID,
) {
const contextType = type.contextType;
if (typeof contextType === 'object' && contextType !== null) {
if (__DEV__) {
const isContextConsumer =
if (__DEV__) {
if ('contextType' in (type: any)) {
let isValid =
contextType !== null &&
contextType !== undefined &&
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',
);
contextType._context === undefined; // Not a <Context.Consumer>

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

let addendum = '';
if (contextType === null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sometimes it is nice to be able to conditionally set it:

class Foo {
  static contextType = isServerEnvironment ? null : Context;
}

This provides no way to do that while also ensuring you stay declarative.

Is null really a common mistake? I'm guessing not since it's the only one you don't have a descriptive message for.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope. Agree let’s omit it.

addendum = ' However, it is set to null.';
} else 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 ' + 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
49 changes: 37 additions & 12 deletions packages/react-reconciler/src/ReactFiberClassComponent.js
View file
Open in desktop
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,52 @@ function constructClassInstance(
let unmaskedContext = emptyContextObject;
let context = null;
const contextType = ctor.contextType;
if (typeof contextType === 'object' && contextType !== null) {
if (__DEV__) {
const isContextConsumer =

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

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

let addendum = '';
if (contextType === null) {
addendum = ' However, it is set to null.';
} else 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 ' + typeof contextType + '.';
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can add "a" here to make it a little clearer

addendum = ' However, it is set to a ' + typeof contextType + '.';

That way it reports "set to a string" instead of "set to string", which sounds better. Same goes for numbers, symbols, and functions.

} 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
89 changes: 89 additions & 0 deletions packages/react/src/__tests__/ReactContextValidator-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -578,6 +578,95 @@ describe('ReactContextValidator', () => {
);
});

it('should warn when class contextType is null', () => {
class Foo extends React.Component {
static contextType = null;
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 null.',
{withoutStack: true},
);
});

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 string.',
{withoutStack: true},
);
});

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

Expand Down