From 854daa90d93db62be262322f6804ac6edf835cb8 Mon Sep 17 00:00:00 2001 From: sw-yx Date: Sat, 28 Oct 2017 21:44:12 -0400 Subject: [PATCH 1/3] added forgotExtendClass warning on new master --- .../__tests__/ReactCompositeComponent-test.js | 26 +++++++++++++++++++ .../src/ReactFiberBeginWork.js | 12 +++++++++ 2 files changed, 38 insertions(+) diff --git a/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js b/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js index 75f3e8060f384..cca936c731aff 100644 --- a/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js +++ b/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js @@ -326,6 +326,32 @@ describe('ReactCompositeComponent', () => { expect(cbCalled).toBe(false); }); + it( + 'should throw an Error with a warning when rendering' + + "a class with a render method that doesn't extend React.Component", + () => { + spyOn(console, 'error'); + var container = document.createElement('div'); + class ClassWithRenderNotExtended { + render() { + return
; + } + } + expectDev(console.error.calls.count()).toBe(0); + try { + ReactDOM.render(, container); + } catch (e) { + expect(e).toEqual(new TypeError('Cannot call a class as a function')); + expectDev(console.error.calls.count()).toBe(1); + expectDev(console.error.calls.argsFor(0)[0]).toContain( + 'Warning: The component appears to have a render method, ' + + "but doesn't extend React.Component. This is likely to cause errors. " + + 'Change ClassWithRenderNotExtended to extend React.Component instead.', + ); + } + }, + ); + it('should warn about `setState` in render', () => { spyOn(console, 'error'); diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index efeb2a29f2801..af4dfa207a1a9 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -39,6 +39,7 @@ var { } = require('shared/ReactTypeOfSideEffect'); var {ReactCurrentOwner} = require('shared/ReactGlobalSharedState'); var invariant = require('fbjs/lib/invariant'); +var getComponentName = require('shared/getComponentName'); var ReactFiberClassComponent = require('./ReactFiberClassComponent'); var { @@ -471,6 +472,17 @@ module.exports = function( var value; if (__DEV__) { + const classWithRenderButNotExtendedFromReactComponent = + workInProgress.type.prototype && workInProgress.type.prototype.render; + if (classWithRenderButNotExtendedFromReactComponent) { + warning( + false, + "The <%s /> component appears to have a render method, but doesn't extend React.Component. " + + 'This is likely to cause errors. Change %s to extend React.Component instead.', + getComponentName(workInProgress), + getComponentName(workInProgress), + ); + } ReactCurrentOwner.current = workInProgress; value = fn(props, context); } else { From e8d38510cf1828c786d722b0d57890e16158ee4d Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 31 Oct 2017 11:56:51 +0000 Subject: [PATCH 2/3] Tweak the test to assert in all cases --- .../__tests__/ReactCompositeComponent-test.js | 42 ++++++++----------- 1 file changed, 18 insertions(+), 24 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js b/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js index cca936c731aff..56bd6c63aba24 100644 --- a/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js +++ b/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js @@ -326,31 +326,25 @@ describe('ReactCompositeComponent', () => { expect(cbCalled).toBe(false); }); - it( - 'should throw an Error with a warning when rendering' + - "a class with a render method that doesn't extend React.Component", - () => { - spyOn(console, 'error'); - var container = document.createElement('div'); - class ClassWithRenderNotExtended { - render() { - return
; - } - } - expectDev(console.error.calls.count()).toBe(0); - try { - ReactDOM.render(, container); - } catch (e) { - expect(e).toEqual(new TypeError('Cannot call a class as a function')); - expectDev(console.error.calls.count()).toBe(1); - expectDev(console.error.calls.argsFor(0)[0]).toContain( - 'Warning: The component appears to have a render method, ' + - "but doesn't extend React.Component. This is likely to cause errors. " + - 'Change ClassWithRenderNotExtended to extend React.Component instead.', - ); + it('should warn when rendering a class with a render method that does not extend React.Component', () => { + spyOn(console, 'error'); + var container = document.createElement('div'); + class ClassWithRenderNotExtended { + render() { + return
; } - }, - ); + } + expectDev(console.error.calls.count()).toBe(0); + expect(() => { + ReactDOM.render(, container); + }).toThrow(TypeError); + expectDev(console.error.calls.count()).toBe(1); + expectDev(console.error.calls.argsFor(0)[0]).toContain( + 'Warning: The component appears to have a render method, ' + + "but doesn't extend React.Component. This is likely to cause errors. " + + 'Change ClassWithRenderNotExtended to extend React.Component instead.', + ); + }); it('should warn about `setState` in render', () => { spyOn(console, 'error'); From 97e3a77aa43c3745e9eaffce8941d106b23e1478 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 31 Oct 2017 12:00:56 +0000 Subject: [PATCH 3/3] Simplify the condition --- packages/react-reconciler/src/ReactFiberBeginWork.js | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index af4dfa207a1a9..868b7e244ab0c 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -472,15 +472,14 @@ module.exports = function( var value; if (__DEV__) { - const classWithRenderButNotExtendedFromReactComponent = - workInProgress.type.prototype && workInProgress.type.prototype.render; - if (classWithRenderButNotExtendedFromReactComponent) { + if (fn.prototype && typeof fn.prototype.render === 'function') { + const componentName = getComponentName(workInProgress); warning( false, "The <%s /> component appears to have a render method, but doesn't extend React.Component. " + 'This is likely to cause errors. Change %s to extend React.Component instead.', - getComponentName(workInProgress), - getComponentName(workInProgress), + componentName, + componentName, ); } ReactCurrentOwner.current = workInProgress;