From 6499aba257c9046ca5632d8b94ddbf4d1a3b9779 Mon Sep 17 00:00:00 2001 From: Neil Kistner Date: Mon, 8 Jan 2018 11:23:09 -0600 Subject: [PATCH 1/4] Add warning in server renderer if class doesn't extend React.Component In dev mode, while server rendering, a warning will be thrown if there is a class that doesn't extend React.Component. --- .../__tests__/ReactServerRendering-test.js | 21 +++++++++++++++++++ .../src/server/ReactPartialRenderer.js | 15 +++++++++++++ 2 files changed, 36 insertions(+) diff --git a/packages/react-dom/src/__tests__/ReactServerRendering-test.js b/packages/react-dom/src/__tests__/ReactServerRendering-test.js index cfba21e0523ef..fd5930c87f55d 100644 --- a/packages/react-dom/src/__tests__/ReactServerRendering-test.js +++ b/packages/react-dom/src/__tests__/ReactServerRendering-test.js @@ -554,4 +554,25 @@ describe('ReactDOMServer', () => { 'The experimental Call and Return types are not currently supported by the server renderer.', ); }); + + it('should warn when server rendering a class with a render method that does not extend React.Component', () => { + spyOnDevAndProd(console, 'error'); + class ClassWithRenderNotExtended { + render() { + return
; + } + } + expect(console.error.calls.count()).toBe(0); + expect(() => { + ReactDOMServer.renderToString(); + }).toThrow(TypeError); + if (__DEV__) { + expect(console.error.calls.count()).toBe(1); + expect(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.', + ); + } + }); }); diff --git a/packages/react-dom/src/server/ReactPartialRenderer.js b/packages/react-dom/src/server/ReactPartialRenderer.js index cc939b691b073..58cc808ffaec6 100644 --- a/packages/react-dom/src/server/ReactPartialRenderer.js +++ b/packages/react-dom/src/server/ReactPartialRenderer.js @@ -421,6 +421,21 @@ function resolve( if (shouldConstruct(Component)) { inst = new Component(element.props, publicContext, updater); } else { + if (__DEV__) { + if ( + Component.prototype && + typeof Component.prototype.render === 'function' + ) { + const componentName = getComponentName(Component); + 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.', + componentName, + componentName, + ); + } + } inst = Component(element.props, publicContext, updater); if (inst == null || inst.render == null) { child = inst; From 0ed0dae8a4681e5a65e39e669a8b9367717aa011 Mon Sep 17 00:00:00 2001 From: Neil Kistner Date: Mon, 8 Jan 2018 13:46:31 -0600 Subject: [PATCH 2/4] Use `.toWarnDev` matcher and deduplicate warnings --- .../__tests__/ReactServerRendering-test.js | 18 ++++++++++-------- .../src/server/ReactPartialRenderer.js | 19 ++++++++++++------- 2 files changed, 22 insertions(+), 15 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactServerRendering-test.js b/packages/react-dom/src/__tests__/ReactServerRendering-test.js index fd5930c87f55d..cfc17f7e28aab 100644 --- a/packages/react-dom/src/__tests__/ReactServerRendering-test.js +++ b/packages/react-dom/src/__tests__/ReactServerRendering-test.js @@ -556,23 +556,25 @@ describe('ReactDOMServer', () => { }); it('should warn when server rendering a class with a render method that does not extend React.Component', () => { - spyOnDevAndProd(console, 'error'); class ClassWithRenderNotExtended { render() { return
; } } - expect(console.error.calls.count()).toBe(0); + expect(() => { - ReactDOMServer.renderToString(); - }).toThrow(TypeError); - if (__DEV__) { - expect(console.error.calls.count()).toBe(1); - expect(console.error.calls.argsFor(0)[0]).toContain( + expect(() => + ReactDOMServer.renderToString(), + ).toWarnDev( '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.', ); - } + }).toThrow(TypeError); + + // Test deduplication + expect(() => { + ReactDOMServer.renderToString(); + }).toThrow(TypeError); }); }); diff --git a/packages/react-dom/src/server/ReactPartialRenderer.js b/packages/react-dom/src/server/ReactPartialRenderer.js index 58cc808ffaec6..7704403d36c5a 100644 --- a/packages/react-dom/src/server/ReactPartialRenderer.js +++ b/packages/react-dom/src/server/ReactPartialRenderer.js @@ -122,6 +122,7 @@ let didWarnDefaultSelectValue = false; let didWarnDefaultTextareaValue = false; let didWarnInvalidOptionChildren = false; const didWarnAboutNoopUpdateForComponent = {}; +const didWarnAboutBadClass = {}; const valuePropNames = ['value', 'defaultValue']; const newlineEatingTags = { listing: true, @@ -427,13 +428,17 @@ function resolve( typeof Component.prototype.render === 'function' ) { const componentName = getComponentName(Component); - 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.', - componentName, - componentName, - ); + + if (componentName !== null && !didWarnAboutBadClass[componentName]) { + 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.', + componentName, + componentName, + ); + didWarnAboutBadClass[componentName] = true; + } } } inst = Component(element.props, publicContext, updater); From 2aaabea10a246d9436e6341cf0df4bdcd8a3ed21 Mon Sep 17 00:00:00 2001 From: Neil Kistner Date: Tue, 9 Jan 2018 08:19:51 -0600 Subject: [PATCH 3/4] Deduplicate client-side warning if class doesn't extend React.Component --- .../__tests__/ReactCompositeComponent-test.js | 5 +++++ .../src/ReactFiberBeginWork.js | 20 ++++++++++++------- 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js b/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js index a707a8df6cd1f..330b613644908 100644 --- a/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js +++ b/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js @@ -338,6 +338,11 @@ describe('ReactCompositeComponent', () => { 'Change ClassWithRenderNotExtended to extend React.Component instead.', ); }).toThrow(TypeError); + + // Test deduplication + expect(() => { + ReactDOM.render(, container); + }).toThrow(TypeError); }); it('should warn about `setState` in render', () => { diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index 6c962576e02ed..4bf60b2dd9593 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -60,9 +60,11 @@ import { import {NoWork, Never} from './ReactFiberExpirationTime'; let warnedAboutStatelessRefs; +let didWarnAboutBadClass; if (__DEV__) { warnedAboutStatelessRefs = {}; + didWarnAboutBadClass = {}; } export default function( @@ -459,13 +461,17 @@ export default function( if (__DEV__) { 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.', - componentName, - componentName, - ); + + if (componentName !== null && !didWarnAboutBadClass[componentName]) { + 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.', + componentName, + componentName, + ); + didWarnAboutBadClass[componentName] = true; + } } ReactCurrentOwner.current = workInProgress; value = fn(props, context); From 5a061178b96d64effba975ebf78fea5394436e43 Mon Sep 17 00:00:00 2001 From: Neil Kistner Date: Tue, 9 Jan 2018 09:57:58 -0600 Subject: [PATCH 4/4] Default componentName to Unknown if null --- packages/react-dom/src/server/ReactPartialRenderer.js | 4 ++-- packages/react-reconciler/src/ReactFiberBeginWork.js | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/react-dom/src/server/ReactPartialRenderer.js b/packages/react-dom/src/server/ReactPartialRenderer.js index 7704403d36c5a..526771f49ca5e 100644 --- a/packages/react-dom/src/server/ReactPartialRenderer.js +++ b/packages/react-dom/src/server/ReactPartialRenderer.js @@ -427,9 +427,9 @@ function resolve( Component.prototype && typeof Component.prototype.render === 'function' ) { - const componentName = getComponentName(Component); + const componentName = getComponentName(Component) || 'Unknown'; - if (componentName !== null && !didWarnAboutBadClass[componentName]) { + if (!didWarnAboutBadClass[componentName]) { warning( false, "The <%s /> component appears to have a render method, but doesn't extend React.Component. " + diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index 4bf60b2dd9593..415e2697147fa 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -460,9 +460,9 @@ export default function( if (__DEV__) { if (fn.prototype && typeof fn.prototype.render === 'function') { - const componentName = getComponentName(workInProgress); + const componentName = getComponentName(workInProgress) || 'Unknown'; - if (componentName !== null && !didWarnAboutBadClass[componentName]) { + if (!didWarnAboutBadClass[componentName]) { warning( false, "The <%s /> component appears to have a render method, but doesn't extend React.Component. " +