From 7770535f723e6d2c6d50c8bb4839770246d7c18b Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 22 Nov 2017 18:58:53 +0000 Subject: [PATCH] Change DEV-only invariants to be warnings (#11630) * Change DEV-only invariant about instance.state type to a warning * Change DEV-only invariant childContextTypes check to a warning --- .../ReactDOMServerIntegration-test.js | 39 ++++++++++--------- .../src/server/ReactPartialRenderer.js | 27 +++++++------ .../src/ReactFiberClassComponent.js | 4 +- .../ReactCoffeeScriptClass-test.coffee | 20 ++++------ .../react/src/__tests__/ReactES6Class-test.js | 14 +++---- .../__tests__/ReactTypeScriptClass-test.ts | 36 ++++++++--------- 6 files changed, 69 insertions(+), 71 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegration-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegration-test.js index c698f6ce72b88..efc2e7e514bdc 100644 --- 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
; - } - 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 ; } - return render(); - }, - 'MyComponent.getChildContext(): childContextTypes must be defined ' + - 'in order to use getChildContext().', - ); - } + getChildContext() { + return {foo: 'bar'}; + } + } + const e = await render(, 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 index 64ac4c8f4e050..bf63c063f5942 100644 --- 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 index 9c98d8042e957..68faabeccca48 100644 --- 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 index 7918b82bd6e19..865487aaaa838 100644 --- 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 index 7e289f1a790ae..de03e19254817 100644 --- 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 ; } } + test(, 'SPAN', ''); if (__DEV__) { - expect(() => test(, '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(, '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 index 267a4f61d85a2..9fa16260c712a 100644 --- 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((console.error).calls.count()).toBe(1); + expect((console.error).calls.argsFor(0)[0]).toContain( 'ArrayState.state: must be set to an object or null' ); - expect(() => test(React.createElement(StringState), 'SPAN', '')) - .toThrowError( + (console.error).calls.reset() + } + test(React.createElement(StringState), 'SPAN', ''); + if (__DEV__) { + expect((console.error).calls.count()).toBe(1); + expect((console.error).calls.argsFor(0)[0]).toContain( 'StringState.state: must be set to an object or null' ); - expect(() => test(React.createElement(NumberState), 'SPAN', '')) - .toThrowError( + (console.error).calls.reset() + } + test(React.createElement(NumberState), 'SPAN', ''); + if (__DEV__) { + expect((console.error).calls.count()).toBe(1); + expect((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(); + (console.error).calls.reset() } });