[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
Soo Jae Hwang committed
Apr 21, 2017 at 16:55 UTC
ec527cc8342c9c80dfd3e396c4280346a7603450
4 files changed
+59
-1
scripts/fiber/tests-passing.txt
+1
@@ -336,6 +336,7 @@ src/isomorphic/modern/class/__tests__/ReactPureComponent-test.js
336
* should render
337
* can override shouldComponentUpdate
338
* extends React.Component
339
+* should warn when shouldComponentUpdate is defined on React.PureComponent
340
341
src/isomorphic/modern/class/__tests__/ReactTypeScriptClass-test.ts
342
* preserves the name of the class for use in error messages
src/isomorphic/modern/class/__tests__/ReactPureComponent-test.js
+31
@@ -64,6 +64,7 @@ describe('ReactPureComponent', () => {
64
});
65
66
it('can override shouldComponentUpdate', () => {
67
+ spyOn(console, 'error');
68
var renders = 0;
69
class Component extends React.PureComponent {
70
render() {
@@ -77,6 +78,13 @@ describe('ReactPureComponent', () => {
78
var container = document.createElement('div');
79
ReactDOM.render(<Component />, container);
80
ReactDOM.render(<Component />, container);
81
+ expect(console.error.calls.count()).toBe(1);
82
+ expect(console.error.calls.argsFor(0)[0]).toBe(
83
+ 'Warning: ' +
84
+ 'Component has a method called shouldComponentUpdate(). ' +
85
+ 'shouldComponentUpdate should not be used when extending React.PureComponent. ' +
86
+ 'Please extend React.Component if shouldComponentUpdate is used.',
87
+ );
88
expect(renders).toBe(2);
89
});
90
@@ -93,4 +101,27 @@ describe('ReactPureComponent', () => {
101
ReactDOM.render(<Component />, document.createElement('div'));
102
expect(renders).toBe(1);
103
});
104
+
105
+ it('should warn when shouldComponentUpdate is defined on React.PureComponent', () => {
106
+ spyOn(console, 'error');
107
+
108
+ class PureComponent extends React.PureComponent {
109
+ shouldComponentUpdate() {
110
+ return true;
111
+ }
112
+ render() {
113
+ return <div />;
114
+ }
115
+ }
116
+ var container = document.createElement('div');
117
+ ReactDOM.render(<PureComponent />, container);
118
+
119
+ expect(console.error.calls.count()).toBe(1);
120
+ expect(console.error.calls.argsFor(0)[0]).toBe(
121
+ 'Warning: ' +
122
+ 'PureComponent has a method called shouldComponentUpdate(). ' +
123
+ 'shouldComponentUpdate should not be used when extending React.PureComponent. ' +
124
+ 'Please extend React.Component if shouldComponentUpdate is used.',
125
+ );
126
+ });
127
});
src/renderers/shared/fiber/ReactFiberClassComponent.js
+15
-1
@@ -112,6 +112,7 @@ module.exports = function(
112
}
113
114
const instance = workInProgress.stateNode;
115
+ const type = workInProgress.type;
116
if (typeof instance.shouldComponentUpdate === 'function') {
117
if (__DEV__) {
118
startPhaseTimer(workInProgress, 'shouldComponentUpdate');
@@ -137,7 +138,6 @@ module.exports = function(
138
return shouldUpdate;
139
}
140
140
- const type = workInProgress.type;
141
if (type.prototype && type.prototype.isPureReactComponent) {
142
return (
143
!shallowEqual(oldProps, newProps) || !shallowEqual(oldState, newState)
@@ -149,6 +149,7 @@ module.exports = function(
149
150
function checkClassInstance(workInProgress: Fiber) {
151
const instance = workInProgress.stateNode;
152
+ const type = workInProgress.type;
153
if (__DEV__) {
154
const name = getComponentName(workInProgress);
155
const renderPresent = instance.render;
@@ -203,6 +204,19 @@ module.exports = function(
204
'expected to return a value.',
205
name,
206
);
207
+ if (
208
+ type.prototype &&
209
+ type.prototype.isPureReactComponent &&
210
+ typeof instance.shouldComponentUpdate !== 'undefined'
211
+ ) {
212
+ warning(
213
+ false,
214
+ '%s has a method called shouldComponentUpdate(). ' +
215
+ 'shouldComponentUpdate should not be used when extending React.PureComponent. ' +
216
+ 'Please extend React.Component if shouldComponentUpdate is used.',
217
+ getComponentName(workInProgress) || 'A pure component',
218
+ );
219
+ }
220
const noComponentDidUnmount =
221
typeof instance.componentDidUnmount !== 'function';
222
warning(
src/renderers/shared/stack/reconciler/ReactCompositeComponent.js
+12
@@ -299,6 +299,18 @@ var ReactCompositeComponent = {
299
'componentWillRecieveProps(). Did you mean componentWillReceiveProps()?',
300
this.getName() || 'A component',
301
);
302
+ if (
303
+ isPureComponent(Component) &&
304
+ typeof inst.shouldComponentUpdate !== 'undefined'
305
+ ) {
306
+ warning(
307
+ false,
308
+ '%s has a method called shouldComponentUpdate(). ' +
309
+ 'shouldComponentUpdate should not be used when extending React.PureComponent. ' +
310
+ 'Please extend React.Component if shouldComponentUpdate is used.',
311
+ this.getName() || 'A pure component',
312
+ );
313
+ }
314
warning(
315
!inst.defaultProps,
316
'defaultProps was defined as an instance property on %s. Use a static ' +