Improve warning for invalid class contextType (#15142)

* Improve warning for invalid class contextType * Don't warn for null * Grammar
facebook · Mar 19, 2019 · 7ad7386 · 7ad7386
1 parent 1e3364e
commit 7ad7386
Show file tree

Hide file tree

Showing 4 changed files with 243 additions and 33 deletions.
diff --git a/packages/react-dom/src/__tests__/ReactServerRendering-test.js b/packages/react-dom/src/__tests__/ReactServerRendering-test.js
@@ -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},
+    );
+  });
 });
diff --git a/packages/react-dom/src/server/ReactPartialRendererContext.js b/packages/react-dom/src/server/ReactPartialRendererContext.js
@@ -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);
@@ -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 {

diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.js b/packages/react-reconciler/src/ReactFiberClassComponent.js
@@ -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';
@@ -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 {
+          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);

diff --git a/packages/react/src/__tests__/ReactContextValidator-test.js b/packages/react/src/__tests__/ReactContextValidator-test.js
@@ -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();