From 0ef3e10b99790b7f72b1c7710ec8678345c83bdb Mon Sep 17 00:00:00 2001 From: Dean Brophy Date: Sat, 28 Oct 2017 13:20:52 -0600 Subject: [PATCH 1/9] Constructor test and fix complete --- .../src/ReactFiberClassComponent.js | 100 ++++++++++-------- .../ReactFiberClassComponent-test.js | 60 +++++++++++ 2 files changed, 115 insertions(+), 45 deletions(-) create mode 100644 packages/react-reconciler/src/__tests__/ReactFiberClassComponent-test.js diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.js b/packages/react-reconciler/src/ReactFiberClassComponent.js index 396be01fb8ab0..77c789009f2db 100644 --- a/packages/react-reconciler/src/ReactFiberClassComponent.js +++ b/packages/react-reconciler/src/ReactFiberClassComponent.js @@ -9,19 +9,19 @@ 'use strict'; -import type {Fiber} from './ReactFiber'; -import type {ExpirationTime} from './ReactFiberExpirationTime'; +import type {Fiber } from './ReactFiber'; +import type {ExpirationTime } from './ReactFiberExpirationTime'; -var {Update} = require('shared/ReactTypeOfSideEffect'); +var { Update } = require('shared/ReactTypeOfSideEffect'); var ReactFeatureFlags = require('shared/ReactFeatureFlags'); -var {isMounted} = require('shared/ReactFiberTreeReflection'); +var { isMounted } = require('shared/ReactFiberTreeReflection'); var ReactInstanceMap = require('shared/ReactInstanceMap'); var emptyObject = require('fbjs/lib/emptyObject'); var getComponentName = require('shared/getComponentName'); var shallowEqual = require('fbjs/lib/shallowEqual'); var invariant = require('fbjs/lib/invariant'); -var {AsyncUpdates} = require('./ReactTypeOfInternalContext'); +var { AsyncUpdates } = require('./ReactTypeOfInternalContext'); var { cacheContext, getMaskedContext, @@ -32,7 +32,7 @@ var { insertUpdateIntoFiber, processUpdateQueue, } = require('./ReactFiberUpdateQueue'); -var {hasContextChanged} = require('./ReactFiberContext'); +var { hasContextChanged } = require('./ReactFiberContext'); const fakeInternalInstance = {}; const isArray = Array.isArray; @@ -40,13 +40,13 @@ const isArray = Array.isArray; if (__DEV__) { var warning = require('fbjs/lib/warning'); - var {startPhaseTimer, stopPhaseTimer} = require('./ReactDebugFiberPerf'); + var { startPhaseTimer, stopPhaseTimer } = require('./ReactDebugFiberPerf'); - var warnOnInvalidCallback = function(callback: mixed, callerName: string) { + var warnOnInvalidCallback = function (callback: mixed, callerName: string) { warning( callback === null || typeof callback === 'function', '%s(...): Expected the last optional `callback` argument to be a ' + - 'function. Instead received: %s.', + 'function. Instead received: %s.', callerName, callback, ); @@ -59,22 +59,22 @@ if (__DEV__) { // exception. Object.defineProperty(fakeInternalInstance, '_processChildContext', { enumerable: false, - value: function() { + value: function () { invariant( false, '_processChildContext is not available in React 16+. This likely ' + - 'means you have multiple copies of React and are attempting to nest ' + - 'a React 15 tree inside a React 16 tree using ' + - "unstable_renderSubtreeIntoContainer, which isn't supported. Try " + - 'to make sure you have only one copy of React (and ideally, switch ' + - 'to ReactDOM.createPortal).', + 'means you have multiple copies of React and are attempting to nest ' + + 'a React 15 tree inside a React 16 tree using ' + + "unstable_renderSubtreeIntoContainer, which isn't supported. Try " + + 'to make sure you have only one copy of React (and ideally, switch ' + + 'to ReactDOM.createPortal).', ); }, }); Object.freeze(fakeInternalInstance); } -module.exports = function( +module.exports = function ( scheduleWork: (fiber: Fiber, expirationTime: ExpirationTime) => void, computeExpirationForFiber: (fiber: Fiber) => ExpirationTime, memoizeProps: (workInProgress: Fiber, props: any) => void, @@ -178,7 +178,7 @@ module.exports = function( warning( shouldUpdate !== undefined, '%s.shouldComponentUpdate(): Returned undefined instead of a ' + - 'boolean value. Make sure to return true or false.', + 'boolean value. Make sure to return true or false.', getComponentName(workInProgress) || 'Unknown', ); } @@ -201,12 +201,22 @@ module.exports = function( if (__DEV__) { const name = getComponentName(workInProgress); const renderPresent = instance.render; - warning( - renderPresent, - '%s(...): No `render` method found on the returned component ' + + + if (type.prototype && type.prototype.render !== undefined) { + warning( + renderPresent, + '%s(...): No `render` method found on the returned component ' + + 'instance: is the `constructor` defined well?', + name, + ); + } else { + warning( + renderPresent, + '%s(...): No `render` method found on the returned component ' + 'instance: you may have forgotten to define `render`.', - name, - ); + name, + ); + } const noGetInitialStateOnES6 = !instance.getInitialState || instance.getInitialState.isReactClassApproved || @@ -214,8 +224,8 @@ module.exports = function( warning( noGetInitialStateOnES6, 'getInitialState was defined on %s, a plain JavaScript class. ' + - 'This is only supported for classes created using React.createClass. ' + - 'Did you mean to define a state property instead?', + 'This is only supported for classes created using React.createClass. ' + + 'Did you mean to define a state property instead?', name, ); const noGetDefaultPropsOnES6 = @@ -224,22 +234,22 @@ module.exports = function( warning( noGetDefaultPropsOnES6, 'getDefaultProps was defined on %s, a plain JavaScript class. ' + - 'This is only supported for classes created using React.createClass. ' + - 'Use a static property to define defaultProps instead.', + 'This is only supported for classes created using React.createClass. ' + + 'Use a static property to define defaultProps instead.', name, ); const noInstancePropTypes = !instance.propTypes; warning( noInstancePropTypes, 'propTypes was defined as an instance property on %s. Use a static ' + - 'property to define propTypes instead.', + 'property to define propTypes instead.', name, ); const noInstanceContextTypes = !instance.contextTypes; warning( noInstanceContextTypes, 'contextTypes was defined as an instance property on %s. Use a static ' + - 'property to define contextTypes instead.', + 'property to define contextTypes instead.', name, ); const noComponentShouldUpdate = @@ -247,9 +257,9 @@ module.exports = function( warning( noComponentShouldUpdate, '%s has a method called ' + - 'componentShouldUpdate(). Did you mean shouldComponentUpdate()? ' + - 'The name is phrased as a question because the function is ' + - 'expected to return a value.', + 'componentShouldUpdate(). Did you mean shouldComponentUpdate()? ' + + 'The name is phrased as a question because the function is ' + + 'expected to return a value.', name, ); if ( @@ -260,8 +270,8 @@ module.exports = function( warning( false, '%s has a method called shouldComponentUpdate(). ' + - 'shouldComponentUpdate should not be used when extending React.PureComponent. ' + - 'Please extend React.Component if shouldComponentUpdate is used.', + 'shouldComponentUpdate should not be used when extending React.PureComponent. ' + + 'Please extend React.Component if shouldComponentUpdate is used.', getComponentName(workInProgress) || 'A pure component', ); } @@ -270,8 +280,8 @@ module.exports = function( warning( noComponentDidUnmount, '%s has a method called ' + - 'componentDidUnmount(). But there is no such lifecycle method. ' + - 'Did you mean componentWillUnmount()?', + 'componentDidUnmount(). But there is no such lifecycle method. ' + + 'Did you mean componentWillUnmount()?', name, ); const noComponentWillRecieveProps = @@ -279,14 +289,14 @@ module.exports = function( warning( noComponentWillRecieveProps, '%s has a method called ' + - 'componentWillRecieveProps(). Did you mean componentWillReceiveProps()?', + 'componentWillRecieveProps(). Did you mean componentWillReceiveProps()?', name, ); const hasMutatedProps = instance.props !== workInProgress.pendingProps; warning( instance.props === undefined || !hasMutatedProps, '%s(...): When calling super() in `%s`, make sure to pass ' + - "up the same props that your component's constructor was passed.", + "up the same props that your component's constructor was passed.", name, name, ); @@ -294,7 +304,7 @@ module.exports = function( warning( noInstanceDefaultProps, 'Setting defaultProps as an instance property on %s is not supported and will be ignored.' + - ' Instead, define defaultProps as a static property on %s.', + ' Instead, define defaultProps as a static property on %s.', name, name, ); @@ -312,7 +322,7 @@ module.exports = function( invariant( typeof workInProgress.type.childContextTypes === 'object', '%s.getChildContext(): childContextTypes must be defined in order to ' + - 'use getChildContext().', + 'use getChildContext().', getComponentName(workInProgress), ); } @@ -367,8 +377,8 @@ module.exports = function( warning( false, '%s.componentWillMount(): Assigning directly to this.state is ' + - "deprecated (except inside a component's " + - 'constructor). Use setState instead.', + "deprecated (except inside a component's " + + 'constructor). Use setState instead.', getComponentName(workInProgress), ); } @@ -396,8 +406,8 @@ module.exports = function( warning( false, '%s.componentWillReceiveProps(): Assigning directly to ' + - "this.state is deprecated (except inside a component's " + - 'constructor). Use setState instead.', + "this.state is deprecated (except inside a component's " + + 'constructor). Use setState instead.', getComponentName(workInProgress), ); } @@ -423,7 +433,7 @@ module.exports = function( invariant( props, 'There must be pending props for an initial mount. This error is ' + - 'likely caused by a bug in React. Please file an issue.', + 'likely caused by a bug in React. Please file an issue.', ); const unmaskedContext = getUnmaskedContext(workInProgress); @@ -586,7 +596,7 @@ module.exports = function( invariant( newProps != null, 'There should always be pending or memoized props. This error is ' + - 'likely caused by a bug in React. Please file an issue.', + 'likely caused by a bug in React. Please file an issue.', ); } const oldContext = instance.context; diff --git a/packages/react-reconciler/src/__tests__/ReactFiberClassComponent-test.js b/packages/react-reconciler/src/__tests__/ReactFiberClassComponent-test.js new file mode 100644 index 0000000000000..c8c06eeb8f6f0 --- /dev/null +++ b/packages/react-reconciler/src/__tests__/ReactFiberClassComponent-test.js @@ -0,0 +1,60 @@ + + +describe('ReactFiberClassComponent', () => { + let React; + let ReactNoop; + + beforeEach(() => { + jest.resetModules(); + React = require('react'); + ReactNoop = require('react-noop-renderer'); + }); + + it('should render a class if nominal', () => { + class NominalComponent extends React.Component { + render() { + return
; + } + } + + ReactNoop.render(); + ReactNoop.flush(); + }); + + it('should return a meaningful warning when constructor is returned', () => { + spyOn(console, 'error'); + class RenderTextInvalidConstructor extends React.Component { + constructor(props) { + super(props); + return { something: false }; + } + + render() { + return
; + } + } + + ReactNoop.render(); + ReactNoop.flushUnitsOfWork(2); + + const error = console.error.calls.mostRecent().args[0]; + + expectDev(error).toBe('Warning: RenderTextInvalidConstructor(...): No `render` method found on the returned ' + + 'component instance: is the `constructor` defined well?'); + + }); + + it('should return error if render is not defined', () => { + spyOn(console, 'error'); + class RenderTestUndefinedRender extends React.Component { + } + + ReactNoop.render(); + ReactNoop.flushUnitsOfWork(2); + + const error = console.error.calls.mostRecent().args[0]; + + expectDev(error).toBe('Warning: RenderTestUndefinedRender(...): No `render` method found on the returned ' + + 'component instance: you may have forgotten to define `render`.'); + }); +}); From 1034a1978e414acffa0fd0668280a4109cbb295f Mon Sep 17 00:00:00 2001 From: Dean Brophy Date: Sat, 28 Oct 2017 13:23:38 -0600 Subject: [PATCH 2/9] Linters and prettier run --- .../src/ReactFiberClassComponent.js | 84 +++++++++---------- .../ReactFiberClassComponent-test.js | 20 ++--- 2 files changed, 52 insertions(+), 52 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.js b/packages/react-reconciler/src/ReactFiberClassComponent.js index 77c789009f2db..d41e815ec06eb 100644 --- a/packages/react-reconciler/src/ReactFiberClassComponent.js +++ b/packages/react-reconciler/src/ReactFiberClassComponent.js @@ -9,19 +9,19 @@ 'use strict'; -import type {Fiber } from './ReactFiber'; -import type {ExpirationTime } from './ReactFiberExpirationTime'; +import type {Fiber} from './ReactFiber'; +import type {ExpirationTime} from './ReactFiberExpirationTime'; -var { Update } = require('shared/ReactTypeOfSideEffect'); +var {Update} = require('shared/ReactTypeOfSideEffect'); var ReactFeatureFlags = require('shared/ReactFeatureFlags'); -var { isMounted } = require('shared/ReactFiberTreeReflection'); +var {isMounted} = require('shared/ReactFiberTreeReflection'); var ReactInstanceMap = require('shared/ReactInstanceMap'); var emptyObject = require('fbjs/lib/emptyObject'); var getComponentName = require('shared/getComponentName'); var shallowEqual = require('fbjs/lib/shallowEqual'); var invariant = require('fbjs/lib/invariant'); -var { AsyncUpdates } = require('./ReactTypeOfInternalContext'); +var {AsyncUpdates} = require('./ReactTypeOfInternalContext'); var { cacheContext, getMaskedContext, @@ -32,7 +32,7 @@ var { insertUpdateIntoFiber, processUpdateQueue, } = require('./ReactFiberUpdateQueue'); -var { hasContextChanged } = require('./ReactFiberContext'); +var {hasContextChanged} = require('./ReactFiberContext'); const fakeInternalInstance = {}; const isArray = Array.isArray; @@ -40,13 +40,13 @@ const isArray = Array.isArray; if (__DEV__) { var warning = require('fbjs/lib/warning'); - var { startPhaseTimer, stopPhaseTimer } = require('./ReactDebugFiberPerf'); + var {startPhaseTimer, stopPhaseTimer} = require('./ReactDebugFiberPerf'); - var warnOnInvalidCallback = function (callback: mixed, callerName: string) { + var warnOnInvalidCallback = function(callback: mixed, callerName: string) { warning( callback === null || typeof callback === 'function', '%s(...): Expected the last optional `callback` argument to be a ' + - 'function. Instead received: %s.', + 'function. Instead received: %s.', callerName, callback, ); @@ -59,22 +59,22 @@ if (__DEV__) { // exception. Object.defineProperty(fakeInternalInstance, '_processChildContext', { enumerable: false, - value: function () { + value: function() { invariant( false, '_processChildContext is not available in React 16+. This likely ' + - 'means you have multiple copies of React and are attempting to nest ' + - 'a React 15 tree inside a React 16 tree using ' + - "unstable_renderSubtreeIntoContainer, which isn't supported. Try " + - 'to make sure you have only one copy of React (and ideally, switch ' + - 'to ReactDOM.createPortal).', + 'means you have multiple copies of React and are attempting to nest ' + + 'a React 15 tree inside a React 16 tree using ' + + "unstable_renderSubtreeIntoContainer, which isn't supported. Try " + + 'to make sure you have only one copy of React (and ideally, switch ' + + 'to ReactDOM.createPortal).', ); }, }); Object.freeze(fakeInternalInstance); } -module.exports = function ( +module.exports = function( scheduleWork: (fiber: Fiber, expirationTime: ExpirationTime) => void, computeExpirationForFiber: (fiber: Fiber) => ExpirationTime, memoizeProps: (workInProgress: Fiber, props: any) => void, @@ -178,7 +178,7 @@ module.exports = function ( warning( shouldUpdate !== undefined, '%s.shouldComponentUpdate(): Returned undefined instead of a ' + - 'boolean value. Make sure to return true or false.', + 'boolean value. Make sure to return true or false.', getComponentName(workInProgress) || 'Unknown', ); } @@ -206,14 +206,14 @@ module.exports = function ( warning( renderPresent, '%s(...): No `render` method found on the returned component ' + - 'instance: is the `constructor` defined well?', + 'instance: is the `constructor` defined well?', name, ); } else { warning( renderPresent, '%s(...): No `render` method found on the returned component ' + - 'instance: you may have forgotten to define `render`.', + 'instance: you may have forgotten to define `render`.', name, ); } @@ -224,8 +224,8 @@ module.exports = function ( warning( noGetInitialStateOnES6, 'getInitialState was defined on %s, a plain JavaScript class. ' + - 'This is only supported for classes created using React.createClass. ' + - 'Did you mean to define a state property instead?', + 'This is only supported for classes created using React.createClass. ' + + 'Did you mean to define a state property instead?', name, ); const noGetDefaultPropsOnES6 = @@ -234,22 +234,22 @@ module.exports = function ( warning( noGetDefaultPropsOnES6, 'getDefaultProps was defined on %s, a plain JavaScript class. ' + - 'This is only supported for classes created using React.createClass. ' + - 'Use a static property to define defaultProps instead.', + 'This is only supported for classes created using React.createClass. ' + + 'Use a static property to define defaultProps instead.', name, ); const noInstancePropTypes = !instance.propTypes; warning( noInstancePropTypes, 'propTypes was defined as an instance property on %s. Use a static ' + - 'property to define propTypes instead.', + 'property to define propTypes instead.', name, ); const noInstanceContextTypes = !instance.contextTypes; warning( noInstanceContextTypes, 'contextTypes was defined as an instance property on %s. Use a static ' + - 'property to define contextTypes instead.', + 'property to define contextTypes instead.', name, ); const noComponentShouldUpdate = @@ -257,9 +257,9 @@ module.exports = function ( warning( noComponentShouldUpdate, '%s has a method called ' + - 'componentShouldUpdate(). Did you mean shouldComponentUpdate()? ' + - 'The name is phrased as a question because the function is ' + - 'expected to return a value.', + 'componentShouldUpdate(). Did you mean shouldComponentUpdate()? ' + + 'The name is phrased as a question because the function is ' + + 'expected to return a value.', name, ); if ( @@ -270,8 +270,8 @@ module.exports = function ( warning( false, '%s has a method called shouldComponentUpdate(). ' + - 'shouldComponentUpdate should not be used when extending React.PureComponent. ' + - 'Please extend React.Component if shouldComponentUpdate is used.', + 'shouldComponentUpdate should not be used when extending React.PureComponent. ' + + 'Please extend React.Component if shouldComponentUpdate is used.', getComponentName(workInProgress) || 'A pure component', ); } @@ -280,8 +280,8 @@ module.exports = function ( warning( noComponentDidUnmount, '%s has a method called ' + - 'componentDidUnmount(). But there is no such lifecycle method. ' + - 'Did you mean componentWillUnmount()?', + 'componentDidUnmount(). But there is no such lifecycle method. ' + + 'Did you mean componentWillUnmount()?', name, ); const noComponentWillRecieveProps = @@ -289,14 +289,14 @@ module.exports = function ( warning( noComponentWillRecieveProps, '%s has a method called ' + - 'componentWillRecieveProps(). Did you mean componentWillReceiveProps()?', + 'componentWillRecieveProps(). Did you mean componentWillReceiveProps()?', name, ); const hasMutatedProps = instance.props !== workInProgress.pendingProps; warning( instance.props === undefined || !hasMutatedProps, '%s(...): When calling super() in `%s`, make sure to pass ' + - "up the same props that your component's constructor was passed.", + "up the same props that your component's constructor was passed.", name, name, ); @@ -304,7 +304,7 @@ module.exports = function ( warning( noInstanceDefaultProps, 'Setting defaultProps as an instance property on %s is not supported and will be ignored.' + - ' Instead, define defaultProps as a static property on %s.', + ' Instead, define defaultProps as a static property on %s.', name, name, ); @@ -322,7 +322,7 @@ module.exports = function ( invariant( typeof workInProgress.type.childContextTypes === 'object', '%s.getChildContext(): childContextTypes must be defined in order to ' + - 'use getChildContext().', + 'use getChildContext().', getComponentName(workInProgress), ); } @@ -377,8 +377,8 @@ module.exports = function ( warning( false, '%s.componentWillMount(): Assigning directly to this.state is ' + - "deprecated (except inside a component's " + - 'constructor). Use setState instead.', + "deprecated (except inside a component's " + + 'constructor). Use setState instead.', getComponentName(workInProgress), ); } @@ -406,8 +406,8 @@ module.exports = function ( warning( false, '%s.componentWillReceiveProps(): Assigning directly to ' + - "this.state is deprecated (except inside a component's " + - 'constructor). Use setState instead.', + "this.state is deprecated (except inside a component's " + + 'constructor). Use setState instead.', getComponentName(workInProgress), ); } @@ -433,7 +433,7 @@ module.exports = function ( invariant( props, 'There must be pending props for an initial mount. This error is ' + - 'likely caused by a bug in React. Please file an issue.', + 'likely caused by a bug in React. Please file an issue.', ); const unmaskedContext = getUnmaskedContext(workInProgress); @@ -596,7 +596,7 @@ module.exports = function ( invariant( newProps != null, 'There should always be pending or memoized props. This error is ' + - 'likely caused by a bug in React. Please file an issue.', + 'likely caused by a bug in React. Please file an issue.', ); } const oldContext = instance.context; diff --git a/packages/react-reconciler/src/__tests__/ReactFiberClassComponent-test.js b/packages/react-reconciler/src/__tests__/ReactFiberClassComponent-test.js index c8c06eeb8f6f0..6091e08c67ef3 100644 --- a/packages/react-reconciler/src/__tests__/ReactFiberClassComponent-test.js +++ b/packages/react-reconciler/src/__tests__/ReactFiberClassComponent-test.js @@ -1,5 +1,3 @@ - - describe('ReactFiberClassComponent', () => { let React; let ReactNoop; @@ -26,7 +24,7 @@ describe('ReactFiberClassComponent', () => { class RenderTextInvalidConstructor extends React.Component { constructor(props) { super(props); - return { something: false }; + return {something: false}; } render() { @@ -39,22 +37,24 @@ describe('ReactFiberClassComponent', () => { const error = console.error.calls.mostRecent().args[0]; - expectDev(error).toBe('Warning: RenderTextInvalidConstructor(...): No `render` method found on the returned ' + - 'component instance: is the `constructor` defined well?'); - + expectDev(error).toBe( + 'Warning: RenderTextInvalidConstructor(...): No `render` method found on the returned ' + + 'component instance: is the `constructor` defined well?', + ); }); it('should return error if render is not defined', () => { spyOn(console, 'error'); - class RenderTestUndefinedRender extends React.Component { - } + class RenderTestUndefinedRender extends React.Component {} ReactNoop.render(); ReactNoop.flushUnitsOfWork(2); const error = console.error.calls.mostRecent().args[0]; - expectDev(error).toBe('Warning: RenderTestUndefinedRender(...): No `render` method found on the returned ' + - 'component instance: you may have forgotten to define `render`.'); + expectDev(error).toBe( + 'Warning: RenderTestUndefinedRender(...): No `render` method found on the returned ' + + 'component instance: you may have forgotten to define `render`.', + ); }); }); From 03dc1635304c9509f1a7079fec0dab00cb05227f Mon Sep 17 00:00:00 2001 From: Dean Brophy Date: Sat, 28 Oct 2017 18:34:21 -0600 Subject: [PATCH 3/9] Remove unnecessary checks --- .../src/ReactFiberClassComponent.js | 31 ++++++++++--------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.js b/packages/react-reconciler/src/ReactFiberClassComponent.js index d41e815ec06eb..0b901ff874742 100644 --- a/packages/react-reconciler/src/ReactFiberClassComponent.js +++ b/packages/react-reconciler/src/ReactFiberClassComponent.js @@ -202,21 +202,24 @@ module.exports = function( const name = getComponentName(workInProgress); const renderPresent = instance.render; - if (type.prototype && type.prototype.render !== undefined) { - warning( - renderPresent, - '%s(...): No `render` method found on the returned component ' + - 'instance: is the `constructor` defined well?', - name, - ); - } else { - warning( - renderPresent, - '%s(...): No `render` method found on the returned component ' + - 'instance: you may have forgotten to define `render`.', - name, - ); + if (!renderPresent) { + if (type.prototype && type.prototype.render !== undefined) { + warning( + false, + '%s(...): No `render` method found on the returned component ' + + 'instance: is the `constructor` defined well?', + name, + ); + } else { + warning( + false, + '%s(...): No `render` method found on the returned component ' + + 'instance: you may have forgotten to define `render`.', + name, + ); + } } + const noGetInitialStateOnES6 = !instance.getInitialState || instance.getInitialState.isReactClassApproved || From e9a0574125647f6a154e90f0e67297a4e8ead372 Mon Sep 17 00:00:00 2001 From: Dean Brophy Date: Sat, 28 Oct 2017 18:38:08 -0600 Subject: [PATCH 4/9] Update error message --- packages/react-reconciler/src/ReactFiberClassComponent.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.js b/packages/react-reconciler/src/ReactFiberClassComponent.js index 0b901ff874742..77e531fffff92 100644 --- a/packages/react-reconciler/src/ReactFiberClassComponent.js +++ b/packages/react-reconciler/src/ReactFiberClassComponent.js @@ -207,7 +207,7 @@ module.exports = function( warning( false, '%s(...): No `render` method found on the returned component ' + - 'instance: is the `constructor` defined well?', + 'instance: did you accidentally return an object from the constructor?', name, ); } else { From 51c5c71d16d20a9c531ecdfd14166624c00f5f31 Mon Sep 17 00:00:00 2001 From: Dean Brophy Date: Sat, 28 Oct 2017 18:42:28 -0600 Subject: [PATCH 5/9] Updat unit test --- .../src/__tests__/ReactFiberClassComponent-test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/react-reconciler/src/__tests__/ReactFiberClassComponent-test.js b/packages/react-reconciler/src/__tests__/ReactFiberClassComponent-test.js index 6091e08c67ef3..c43cccab3ae72 100644 --- a/packages/react-reconciler/src/__tests__/ReactFiberClassComponent-test.js +++ b/packages/react-reconciler/src/__tests__/ReactFiberClassComponent-test.js @@ -38,8 +38,8 @@ describe('ReactFiberClassComponent', () => { const error = console.error.calls.mostRecent().args[0]; expectDev(error).toBe( - 'Warning: RenderTextInvalidConstructor(...): No `render` method found on the returned ' + - 'component instance: is the `constructor` defined well?', + 'Warning: RenderTextInvalidConstructor(...): No `render` method found on the returned component instance: ' + + 'did you accidentally return an object from the constructor?' ); }); From 186062f96b543f8df7b470cc3e5f1d538bc0b0c7 Mon Sep 17 00:00:00 2001 From: Dean Brophy Date: Sat, 28 Oct 2017 18:42:51 -0600 Subject: [PATCH 6/9] prettier --- .../src/__tests__/ReactFiberClassComponent-test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-reconciler/src/__tests__/ReactFiberClassComponent-test.js b/packages/react-reconciler/src/__tests__/ReactFiberClassComponent-test.js index c43cccab3ae72..d77fd43a887a0 100644 --- a/packages/react-reconciler/src/__tests__/ReactFiberClassComponent-test.js +++ b/packages/react-reconciler/src/__tests__/ReactFiberClassComponent-test.js @@ -39,7 +39,7 @@ describe('ReactFiberClassComponent', () => { expectDev(error).toBe( 'Warning: RenderTextInvalidConstructor(...): No `render` method found on the returned component instance: ' + - 'did you accidentally return an object from the constructor?' + 'did you accidentally return an object from the constructor?', ); }); From d739f863b394e3de970ee25e8d0c4d3839274625 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 31 Oct 2017 13:05:01 +0000 Subject: [PATCH 7/9] Tweak the check to be more specific --- packages/react-reconciler/src/ReactFiberClassComponent.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.js b/packages/react-reconciler/src/ReactFiberClassComponent.js index 77e531fffff92..bd0363f822b97 100644 --- a/packages/react-reconciler/src/ReactFiberClassComponent.js +++ b/packages/react-reconciler/src/ReactFiberClassComponent.js @@ -203,7 +203,7 @@ module.exports = function( const renderPresent = instance.render; if (!renderPresent) { - if (type.prototype && type.prototype.render !== undefined) { + if (type.prototype && typeof type.prototype.render === 'function') { warning( false, '%s(...): No `render` method found on the returned component ' + From 0593cce9b3f923cde541342877d005af18bd6d0a Mon Sep 17 00:00:00 2001 From: Dean Brophy Date: Wed, 1 Nov 2017 11:47:22 -0600 Subject: [PATCH 8/9] Move tests to ReactCompositeComponent-test --- .../__tests__/ReactCompositeComponent-test.js | 41 +++++++++++++ .../ReactFiberClassComponent-test.js | 60 ------------------- 2 files changed, 41 insertions(+), 60 deletions(-) delete mode 100644 packages/react-reconciler/src/__tests__/ReactFiberClassComponent-test.js diff --git a/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js b/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js index 75f3e8060f384..31f9f4d724420 100644 --- a/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js +++ b/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js @@ -1500,4 +1500,45 @@ describe('ReactCompositeComponent', () => { ReactTestUtils.renderIntoDocument(); expect(mockArgs.length).toEqual(0); }); + + it('should return a meaningful warning when constructor is returned', () => { + spyOn(console, 'error'); + class RenderTextInvalidConstructor extends React.Component { + constructor(props) { + super(props); + return {something: false}; + } + + render() { + return
; + } + } + + expect(function() { + ReactTestUtils.renderIntoDocument(); + }).toThrow(); + + const error = console.error.calls.mostRecent().args[0]; + + expectDev(error).toBe( + 'Warning: RenderTextInvalidConstructor(...): No `render` method found on the returned component instance: ' + + 'did you accidentally return an object from the constructor?', + ); + }); + + it('should return error if render is not defined', () => { + spyOn(console, 'error'); + class RenderTestUndefinedRender extends React.Component {} + + expect(function() { + ReactTestUtils.renderIntoDocument(); + }).toThrow(); + + const error = console.error.calls.mostRecent().args[0]; + + expectDev(error).toBe( + 'Warning: RenderTestUndefinedRender(...): No `render` method found on the returned ' + + 'component instance: you may have forgotten to define `render`.', + ); + }); }); diff --git a/packages/react-reconciler/src/__tests__/ReactFiberClassComponent-test.js b/packages/react-reconciler/src/__tests__/ReactFiberClassComponent-test.js deleted file mode 100644 index d77fd43a887a0..0000000000000 --- a/packages/react-reconciler/src/__tests__/ReactFiberClassComponent-test.js +++ /dev/null @@ -1,60 +0,0 @@ -describe('ReactFiberClassComponent', () => { - let React; - let ReactNoop; - - beforeEach(() => { - jest.resetModules(); - React = require('react'); - ReactNoop = require('react-noop-renderer'); - }); - - it('should render a class if nominal', () => { - class NominalComponent extends React.Component { - render() { - return
; - } - } - - ReactNoop.render(); - ReactNoop.flush(); - }); - - it('should return a meaningful warning when constructor is returned', () => { - spyOn(console, 'error'); - class RenderTextInvalidConstructor extends React.Component { - constructor(props) { - super(props); - return {something: false}; - } - - render() { - return
; - } - } - - ReactNoop.render(); - ReactNoop.flushUnitsOfWork(2); - - const error = console.error.calls.mostRecent().args[0]; - - expectDev(error).toBe( - 'Warning: RenderTextInvalidConstructor(...): No `render` method found on the returned component instance: ' + - 'did you accidentally return an object from the constructor?', - ); - }); - - it('should return error if render is not defined', () => { - spyOn(console, 'error'); - class RenderTestUndefinedRender extends React.Component {} - - ReactNoop.render(); - ReactNoop.flushUnitsOfWork(2); - - const error = console.error.calls.mostRecent().args[0]; - - expectDev(error).toBe( - 'Warning: RenderTestUndefinedRender(...): No `render` method found on the returned ' + - 'component instance: you may have forgotten to define `render`.', - ); - }); -}); From 884e1b485a611e65ad7b3d2a796b53d6e44d6fb9 Mon Sep 17 00:00:00 2001 From: Dean Brophy Date: Wed, 1 Nov 2017 13:43:22 -0600 Subject: [PATCH 9/9] add error call count and remove line --- .../src/__tests__/ReactCompositeComponent-test.js | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js b/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js index 31f9f4d724420..94826f96559f9 100644 --- a/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js +++ b/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js @@ -1518,9 +1518,8 @@ describe('ReactCompositeComponent', () => { ReactTestUtils.renderIntoDocument(); }).toThrow(); - const error = console.error.calls.mostRecent().args[0]; - - expectDev(error).toBe( + expectDev(console.error.calls.count()).toBe(1); + expectDev(console.error.calls.mostRecent().args[0]).toBe( 'Warning: RenderTextInvalidConstructor(...): No `render` method found on the returned component instance: ' + 'did you accidentally return an object from the constructor?', ); @@ -1534,9 +1533,8 @@ describe('ReactCompositeComponent', () => { ReactTestUtils.renderIntoDocument(); }).toThrow(); - const error = console.error.calls.mostRecent().args[0]; - - expectDev(error).toBe( + expectDev(console.error.calls.count()).toBe(1); + expectDev(console.error.calls.mostRecent().args[0]).toBe( 'Warning: RenderTestUndefinedRender(...): No `render` method found on the returned ' + 'component instance: you may have forgotten to define `render`.', );