Change DEV-only invariants to be warnings (facebook#11630)

* Change DEV-only invariant about instance.state type to a warning * Change DEV-only invariant childContextTypes check to a warning
raphamorim · Nov 27, 2017 · 7770535 · 7770535
1 parent 84367d4
commit 7770535
Show file tree

Hide file tree

Showing 6 changed files with 69 additions and 71 deletions.
diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegration-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegration-test.js
@@ -2671,26 +2671,27 @@ describe('ReactDOMServerIntegration', () => {
       },
     );
 
-    // TODO: this being DEV-only is likely a bug.
-    // https://github.com/facebook/react/issues/11618
-    if (__DEV__) {
-      itThrowsWhenRendering(
-        'if getChildContext exists without childContextTypes',
-        render => {
-          class MyComponent extends React.Component {
-            render() {
-              return <div />;
-            }
-            getChildContext() {
-              return {foo: 'bar'};
-            }
+    itRenders(
+      'if getChildContext exists but childContextTypes is missing with a warning',
+      async render => {
+        function HopefulChild(props, context) {
+          return context.foo || 'nope';
+        }
+        HopefulChild.contextTypes = {
+          foo: PropTypes.string,
+        };
+        class ForgetfulParent extends React.Component {
+          render() {
+            return <HopefulChild />;
           }
-          return render(<MyComponent />);
-        },
-        'MyComponent.getChildContext(): childContextTypes must be defined ' +
-          'in order to use getChildContext().',
-      );
-    }
+          getChildContext() {
+            return {foo: 'bar'};
+          }
+        }
+        const e = await render(<ForgetfulParent />, 1);
+        expect(e.textContent).toBe('nope');
+      },
+    );
 
     itThrowsWhenRendering(
       'if getChildContext returns a value not in childContextTypes',

diff --git a/packages/react-dom/src/server/ReactPartialRenderer.js b/packages/react-dom/src/server/ReactPartialRenderer.js
@@ -456,19 +456,22 @@ function resolve(
     var childContext;
     if (typeof inst.getChildContext === 'function') {
       var childContextTypes = Component.childContextTypes;
-      invariant(
-        typeof childContextTypes === 'object',
-        '%s.getChildContext(): childContextTypes must be defined in order to ' +
-          'use getChildContext().',
-        getComponentName(Component) || 'Unknown',
-      );
-      childContext = inst.getChildContext();
-      for (let contextKey in childContext) {
-        invariant(
-          contextKey in childContextTypes,
-          '%s.getChildContext(): key "%s" is not defined in childContextTypes.',
+      if (typeof childContextTypes === 'object') {
+        childContext = inst.getChildContext();
+        for (let contextKey in childContext) {
+          invariant(
+            contextKey in childContextTypes,
+            '%s.getChildContext(): key "%s" is not defined in childContextTypes.',
+            getComponentName(Component) || 'Unknown',
+            contextKey,
+          );
+        }
+      } else {
+        warning(
+          false,
+          '%s.getChildContext(): childContextTypes must be defined in order to ' +
+            'use getChildContext().',
           getComponentName(Component) || 'Unknown',
-          contextKey,
         );
       }
     }

diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.js b/packages/react-reconciler/src/ReactFiberClassComponent.js
@@ -328,14 +328,14 @@ export default function(
 
     const state = instance.state;
     if (state && (typeof state !== 'object' || isArray(state))) {
-      invariant(
+      warning(
         false,
         '%s.state: must be set to an object or null',
         getComponentName(workInProgress),
       );
     }
     if (typeof instance.getChildContext === 'function') {
-      invariant(
+      warning(
         typeof workInProgress.type.childContextTypes === 'object',
         '%s.getChildContext(): childContextTypes must be defined in order to ' +
           'use getChildContext().',

diff --git a/packages/react/src/__tests__/ReactCoffeeScriptClass-test.coffee b/packages/react/src/__tests__/ReactCoffeeScriptClass-test.coffee
@@ -149,7 +149,9 @@ describe 'ReactCoffeeScriptClass', ->
     expect(renderCount).toBe 1
     undefined
 
-  it 'should throw with non-object in the initial state property', ->
+  it 'should warn with non-object in the initial state property', ->
+    spyOnDev console, 'error'
+
     [['an array'], 'a string', 1234].forEach (state) ->
       class Foo extends React.Component
         constructor: ->
@@ -158,20 +160,14 @@ describe 'ReactCoffeeScriptClass', ->
         render: ->
           span()
 
+      test React.createElement(Foo), 'SPAN', ''
       if __DEV__
-        expect(->
-          test React.createElement(Foo), 'SPAN', ''
-        ).toThrowError(
+        expect(console.error.calls.count()).toBe 1
+        expect(console.error.calls.argsFor(0)[0]).toContain(
           'Foo.state: must be set to an object or null'
         )
-      else
-        # This is a difference between development and production.
-        # I'm not sure if this is intentional, as generally we avoid this.
-        # TODO: investigate if this was intentional or an oversight.
-        # https://github.com/facebook/react/issues/11618
-        expect(->
-          test React.createElement(Foo), 'SPAN', ''
-        ).not.toThrowError()
+        console.error.calls.reset()
+
     undefined
 
   it 'should render with null in the initial state property', ->

diff --git a/packages/react/src/__tests__/ReactES6Class-test.js b/packages/react/src/__tests__/ReactES6Class-test.js
@@ -163,7 +163,8 @@ describe('ReactES6Class', () => {
     expect(renderCount).toBe(1);
   });
 
-  it('should throw with non-object in the initial state property', () => {
+  it('should warn with non-object in the initial state property', () => {
+    spyOnDev(console, 'error');
     [['an array'], 'a string', 1234].forEach(function(state) {
       class Foo extends React.Component {
         constructor() {
@@ -174,16 +175,13 @@ describe('ReactES6Class', () => {
           return <span />;
         }
       }
+      test(<Foo />, 'SPAN', '');
       if (__DEV__) {
-        expect(() => test(<Foo />, 'SPAN', '')).toThrowError(
+        expect(console.error.calls.count()).toBe(1);
+        expect(console.error.calls.argsFor(0)[0]).toContain(
           'Foo.state: must be set to an object or null',
         );
-      } else {
-        // This is a difference between development and production.
-        // I'm not sure if this is intentional, as generally we avoid this.
-        // TODO: investigate if this was intentional or an oversight.
-        // https://github.com/facebook/react/issues/11618
-        expect(() => test(<Foo />, 'SPAN', '')).not.toThrowError();
+        console.error.calls.reset();
       }
     });
   });

diff --git a/packages/react/src/__tests__/ReactTypeScriptClass-test.ts b/packages/react/src/__tests__/ReactTypeScriptClass-test.ts
@@ -361,31 +361,31 @@ describe('ReactTypeScriptClass', function() {
     expect(renderCount).toBe(1);
   });
 
-  it('should throw with non-object in the initial state property', function() {
+  it('should warn with non-object in the initial state property', function() {
+    spyOnDev(console, 'error');
+    test(React.createElement(ArrayState), 'SPAN', '');
     if (__DEV__) {
-      expect(() => test(React.createElement(ArrayState), 'SPAN', ''))
-      .toThrowError(
+      expect((<any>console.error).calls.count()).toBe(1);
+      expect((<any>console.error).calls.argsFor(0)[0]).toContain(
         'ArrayState.state: must be set to an object or null'
       );
-      expect(() => test(React.createElement(StringState), 'SPAN', ''))
-      .toThrowError(
+      (<any>console.error).calls.reset()
+    }
+    test(React.createElement(StringState), 'SPAN', '');
+    if (__DEV__) {
+      expect((<any>console.error).calls.count()).toBe(1);
+      expect((<any>console.error).calls.argsFor(0)[0]).toContain(
         'StringState.state: must be set to an object or null'
       );
-      expect(() => test(React.createElement(NumberState), 'SPAN', ''))
-      .toThrowError(
+      (<any>console.error).calls.reset()
+    }
+    test(React.createElement(NumberState), 'SPAN', '');
+    if (__DEV__) {
+      expect((<any>console.error).calls.count()).toBe(1);
+      expect((<any>console.error).calls.argsFor(0)[0]).toContain(
         'NumberState.state: must be set to an object or null'
       );
-    } else {
-      // This is a difference between development and production.
-      // I'm not sure if this is intentional, as generally we avoid this.
-      // TODO: investigate if this was intentional or an oversight.
-      // https://github.com/facebook/react/issues/11618
-      expect(() => test(React.createElement(ArrayState), 'SPAN', ''))
-      .not.toThrowError();
-      expect(() => test(React.createElement(StringState), 'SPAN', ''))
-      .not.toThrowError();
-      expect(() => test(React.createElement(NumberState), 'SPAN', ''))
-      .not.toThrowError();
+      (<any>console.error).calls.reset()
     }
   });