From ec527cc8342c9c80dfd3e396c4280346a7603450 Mon Sep 17 00:00:00 2001 From: Soo Jae Hwang Date: Fri, 21 Apr 2017 16:55:16 +0200 Subject: [PATCH] [WIP] Warn in dev if shouldComponentUpdate is defined on PureComponent (#9240) * Add test for React.PureComponent * Add warning when shouldComponentUpdate is declared in a PureComponent * Add actionable warning * Add warning in Fiber * Format added code by running yarn prettier * Move pure sCU check to checkClassInstance That way it warns before the component updates --- scripts/fiber/tests-passing.txt | 1 + .../__tests__/ReactPureComponent-test.js | 31 +++++++++++++++++++ .../shared/fiber/ReactFiberClassComponent.js | 16 +++++++++- .../reconciler/ReactCompositeComponent.js | 12 +++++++ 4 files changed, 59 insertions(+), 1 deletion(-) diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 80f88c729f..d46c793861 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -336,6 +336,7 @@ src/isomorphic/modern/class/__tests__/ReactPureComponent-test.js * should render * can override shouldComponentUpdate * extends React.Component +* should warn when shouldComponentUpdate is defined on React.PureComponent src/isomorphic/modern/class/__tests__/ReactTypeScriptClass-test.ts * preserves the name of the class for use in error messages diff --git a/src/isomorphic/modern/class/__tests__/ReactPureComponent-test.js b/src/isomorphic/modern/class/__tests__/ReactPureComponent-test.js index e7c03af27b..83c327870f 100644 --- a/src/isomorphic/modern/class/__tests__/ReactPureComponent-test.js +++ b/src/isomorphic/modern/class/__tests__/ReactPureComponent-test.js @@ -64,6 +64,7 @@ describe('ReactPureComponent', () => { }); it('can override shouldComponentUpdate', () => { + spyOn(console, 'error'); var renders = 0; class Component extends React.PureComponent { render() { @@ -77,6 +78,13 @@ describe('ReactPureComponent', () => { var container = document.createElement('div'); ReactDOM.render(, container); ReactDOM.render(, container); + expect(console.error.calls.count()).toBe(1); + expect(console.error.calls.argsFor(0)[0]).toBe( + 'Warning: ' + + 'Component has a method called shouldComponentUpdate(). ' + + 'shouldComponentUpdate should not be used when extending React.PureComponent. ' + + 'Please extend React.Component if shouldComponentUpdate is used.', + ); expect(renders).toBe(2); }); @@ -93,4 +101,27 @@ describe('ReactPureComponent', () => { ReactDOM.render(, document.createElement('div')); expect(renders).toBe(1); }); + + it('should warn when shouldComponentUpdate is defined on React.PureComponent', () => { + spyOn(console, 'error'); + + class PureComponent extends React.PureComponent { + shouldComponentUpdate() { + return true; + } + render() { + return
; + } + } + var container = document.createElement('div'); + ReactDOM.render(, container); + + expect(console.error.calls.count()).toBe(1); + expect(console.error.calls.argsFor(0)[0]).toBe( + 'Warning: ' + + 'PureComponent has a method called shouldComponentUpdate(). ' + + 'shouldComponentUpdate should not be used when extending React.PureComponent. ' + + 'Please extend React.Component if shouldComponentUpdate is used.', + ); + }); }); diff --git a/src/renderers/shared/fiber/ReactFiberClassComponent.js b/src/renderers/shared/fiber/ReactFiberClassComponent.js index 24a44e94c3..f5969f3621 100644 --- a/src/renderers/shared/fiber/ReactFiberClassComponent.js +++ b/src/renderers/shared/fiber/ReactFiberClassComponent.js @@ -112,6 +112,7 @@ module.exports = function( } const instance = workInProgress.stateNode; + const type = workInProgress.type; if (typeof instance.shouldComponentUpdate === 'function') { if (__DEV__) { startPhaseTimer(workInProgress, 'shouldComponentUpdate'); @@ -137,7 +138,6 @@ module.exports = function( return shouldUpdate; } - const type = workInProgress.type; if (type.prototype && type.prototype.isPureReactComponent) { return ( !shallowEqual(oldProps, newProps) || !shallowEqual(oldState, newState) @@ -149,6 +149,7 @@ module.exports = function( function checkClassInstance(workInProgress: Fiber) { const instance = workInProgress.stateNode; + const type = workInProgress.type; if (__DEV__) { const name = getComponentName(workInProgress); const renderPresent = instance.render; @@ -203,6 +204,19 @@ module.exports = function( 'expected to return a value.', name, ); + if ( + type.prototype && + type.prototype.isPureReactComponent && + typeof instance.shouldComponentUpdate !== 'undefined' + ) { + 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.', + getComponentName(workInProgress) || 'A pure component', + ); + } const noComponentDidUnmount = typeof instance.componentDidUnmount !== 'function'; warning( diff --git a/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js b/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js index c58a8fcedb..fe7ff49ba5 100644 --- a/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js +++ b/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js @@ -299,6 +299,18 @@ var ReactCompositeComponent = { 'componentWillRecieveProps(). Did you mean componentWillReceiveProps()?', this.getName() || 'A component', ); + if ( + isPureComponent(Component) && + typeof inst.shouldComponentUpdate !== 'undefined' + ) { + 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.', + this.getName() || 'A pure component', + ); + } warning( !inst.defaultProps, 'defaultProps was defined as an instance property on %s. Use a static ' +