Skip to content

Commit 7ebcd48

Browse files
ljharbljharb
authored
Merge pull request #1084 from jomasti/no-useless-scu
Add react/no-redundant-should-component-update
2 parents a80cf0c + e01a771 commit 7ebcd48

File tree

5 files changed

+326
-1
lines changed

5 files changed

+326
-1
lines changed

‎README.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,7 @@ Finally, enable all of the rules that you would like to use. Use [our preset](#
9696
* [react/no-find-dom-node](docs/rules/no-find-dom-node.md): Prevent usage of `findDOMNode`
9797
* [react/no-is-mounted](docs/rules/no-is-mounted.md): Prevent usage of `isMounted`
9898
* [react/no-multi-comp](docs/rules/no-multi-comp.md): Prevent multiple component definition per file
99+
* [react/no-redundant-should-component-update](docs/rules/no-redundant-should-component-update.md): Prevent usage of `shouldComponentUpdate` when extending React.PureComponent
99100
* [react/no-render-return-value](docs/rules/no-render-return-value.md): Prevent usage of the return value of `React.render`
100101
* [react/no-set-state](docs/rules/no-set-state.md): Prevent usage of `setState`
101102
* [react/no-string-refs](docs/rules/no-string-refs.md): Prevent using string references in `ref` attribute.

‎docs/rules/no-redundant-should-component-update.md‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
# Prevent usage of shouldComponentUpdate when extending React.PureComponent (react/no-redundant-should-component-update)
2+
3+
Warns if you have `shouldComponentUpdate` defined when defining a component that extends React.PureComponent.
4+
While having `shouldComponentUpdate` will still work, it becomes pointless to extend PureComponent.
5+
6+
## Rule Details
7+
8+
The following patterns are considered warnings:
9+
10+
```jsx
11+
class Foo extends React.PureComponent {
12+
shouldComponentUpdate() {
13+
// do check
14+
}
15+
16+
render() {
17+
return <div>Radical!</div>
18+
}
19+
}
20+
21+
function Bar() {
22+
return class Baz extends React.PureComponent {
23+
shouldComponentUpdate() {
24+
// do check
25+
}
26+
27+
render() {
28+
return <div>Groovy!</div>
29+
}
30+
}
31+
}
32+
```
33+
34+
The following patterns are not considered warnings:
35+
36+
```jsx
37+
class Foo extends React.Component {
38+
shouldComponentUpdate() {
39+
// do check
40+
}
41+
42+
render() {
43+
return <div>Radical!</div>
44+
}
45+
}
46+
47+
function Bar() {
48+
return class Baz extends React.Component {
49+
shouldComponentUpdate() {
50+
// do check
51+
}
52+
53+
render() {
54+
return <div>Groovy!</div>
55+
}
56+
}
57+
}
58+
59+
class Qux extends React.PureComponent {
60+
render() {
61+
return <div>Tubular!</div>
62+
}
63+
}
64+
```

‎index.js‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,8 @@ var allRules = {
6262
'no-unused-prop-types': require('./lib/rules/no-unused-prop-types'),
6363
'no-children-prop': require('./lib/rules/no-children-prop'),
6464
'void-dom-elements-no-children': require('./lib/rules/void-dom-elements-no-children'),
65-
'jsx-tag-spacing': require('./lib/rules/jsx-tag-spacing')
65+
'jsx-tag-spacing': require('./lib/rules/jsx-tag-spacing'),
66+
'no-redundant-should-component-update': require('./lib/rules/no-redundant-should-component-update')
6667
};
6768

6869
function filterRules(rules, predicate) {

‎lib/rules/no-redundant-should-component-update.js‎

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,109 @@
1+
/**
2+
* @fileoverview Flag shouldComponentUpdate when extending PureComponent
3+
*/
4+
'use strict';
5+
6+
var Components = require('../util/Components');
7+
8+
function errorMessage(node) {
9+
return `${node} does not need shouldComponentUpdate when extending React.PureComponent.`;
10+
}
11+
12+
// ------------------------------------------------------------------------------
13+
// Rule Definition
14+
// ------------------------------------------------------------------------------
15+
16+
module.exports = {
17+
meta: {
18+
docs: {
19+
description: 'Flag shouldComponentUpdate when extending PureComponent',
20+
category: 'Possible Errors',
21+
recommended: false
22+
},
23+
schema: []
24+
},
25+
26+
create: Components.detect(function(context, components, utils) {
27+
28+
/**
29+
* Get properties name
30+
* @param {Object} node - Property.
31+
* @returns {String} Property name.
32+
*/
33+
function getPropertyName(node) {
34+
if (node.key) {
35+
return node.key.name;
36+
} else if (node.type === 'ClassProperty') {
37+
// Special case for class properties
38+
// (babel-eslint does not expose property name so we have to rely on tokens)
39+
var tokens = context.getFirstTokens(node, 2);
40+
return tokens[1] && tokens[1].type === 'Identifier' ? tokens[1].value : tokens[0].value;
41+
}
42+
return '';
43+
}
44+
45+
/**
46+
* Get properties for a given AST node
47+
* @param {ASTNode} node The AST node being checked.
48+
* @returns {Array} Properties array.
49+
*/
50+
function getComponentProperties(node) {
51+
switch (node.type) {
52+
case 'ClassExpression':
53+
case 'ClassDeclaration':
54+
return node.body.body;
55+
default:
56+
return [];
57+
}
58+
}
59+
60+
/**
61+
* Checks for shouldComponentUpdate property
62+
* @param {ASTNode} node The AST node being checked.
63+
* @returns {Boolean} Whether or not the property exists.
64+
*/
65+
function hasShouldComponentUpdate(node) {
66+
var properties = getComponentProperties(node);
67+
return properties.some(function(property) {
68+
var name = getPropertyName(property);
69+
return name === 'shouldComponentUpdate';
70+
});
71+
}
72+
73+
/**
74+
* Get name of node if available
75+
* @param {ASTNode} node The AST node being checked.
76+
* @return {String} The name of the node
77+
*/
78+
function getNodeName(node) {
79+
if (node.id) {
80+
return node.id.name;
81+
} else if (node.parent && node.parent.id) {
82+
return node.parent.id.name;
83+
}
84+
return '';
85+
}
86+
87+
/**
88+
* Checks for violation of rule
89+
* @param {ASTNode} node The AST node being checked.
90+
*/
91+
function checkForViolation(node) {
92+
if (utils.isPureComponent(node)) {
93+
var hasScu = hasShouldComponentUpdate(node);
94+
if (hasScu) {
95+
var className = getNodeName(node);
96+
context.report({
97+
node: node,
98+
message: errorMessage(className)
99+
});
100+
}
101+
}
102+
}
103+
104+
return {
105+
ClassDeclaration: checkForViolation,
106+
ClassExpression: checkForViolation
107+
};
108+
})
109+
};

‎tests/lib/rules/no-redundant-should-component-update.js‎

Lines changed: 150 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,150 @@
1+
/**
2+
* @fileoverview Tests for no-redundant-should-component-update
3+
*/
4+
5+
'use strict';
6+
7+
// -----------------------------------------------------------------------------
8+
// Requirements
9+
// -----------------------------------------------------------------------------
10+
11+
var rule = require('../../../lib/rules/no-redundant-should-component-update');
12+
var RuleTester = require('eslint').RuleTester;
13+
14+
var parserOptions = {
15+
ecmaVersion: 6,
16+
ecmaFeatures: {
17+
experimentalObjectRestSpread: true,
18+
jsx: true
19+
}
20+
};
21+
22+
function errorMessage(node) {
23+
return `${node} does not need shouldComponentUpdate when extending React.PureComponent.`;
24+
}
25+
26+
// -----------------------------------------------------------------------------
27+
// Tests
28+
// -----------------------------------------------------------------------------
29+
30+
var ruleTester = new RuleTester();
31+
ruleTester.run('no-redundant-should-component-update', rule, {
32+
valid: [
33+
{
34+
code: [
35+
'class Foo extends React.Component {',
36+
' shouldComponentUpdate() {',
37+
' return true;',
38+
' }',
39+
'}'
40+
].join('\n'),
41+
parserOptions: parserOptions
42+
},
43+
{
44+
code: [
45+
'class Foo extends React.Component {',
46+
' shouldComponentUpdate = () => {',
47+
' return true;',
48+
' }',
49+
'}'
50+
].join('\n'),
51+
parser: 'babel-eslint',
52+
parserOptions: parserOptions
53+
},
54+
{
55+
code: [
56+
'class Foo extends React.Component {',
57+
' shouldComponentUpdate() {',
58+
' return true;',
59+
' }',
60+
'}'
61+
].join('\n'),
62+
parserOptions: parserOptions
63+
},
64+
{
65+
code: [
66+
'function Foo() {',
67+
' return class Bar extends React.Component {',
68+
' shouldComponentUpdate() {',
69+
' return true;',
70+
' }',
71+
' };',
72+
'}'
73+
].join('\n'),
74+
parserOptions: parserOptions
75+
}
76+
],
77+
invalid: [
78+
{
79+
code: [
80+
'class Foo extends React.PureComponent {',
81+
' shouldComponentUpdate() {',
82+
' return true;',
83+
' }',
84+
'}'
85+
].join('\n'),
86+
errors: [{message: errorMessage('Foo')}],
87+
parserOptions: parserOptions
88+
},
89+
{
90+
code: [
91+
'class Foo extends PureComponent {',
92+
' shouldComponentUpdate() {',
93+
' return true;',
94+
' }',
95+
'}'
96+
].join('\n'),
97+
errors: [{message: errorMessage('Foo')}],
98+
parserOptions: parserOptions
99+
},
100+
{
101+
code: [
102+
'class Foo extends React.PureComponent {',
103+
' shouldComponentUpdate = () => {',
104+
' return true;',
105+
' }',
106+
'}'
107+
].join('\n'),
108+
errors: [{message: errorMessage('Foo')}],
109+
parser: 'babel-eslint',
110+
parserOptions: parserOptions
111+
},
112+
{
113+
code: [
114+
'function Foo() {',
115+
' return class Bar extends React.PureComponent {',
116+
' shouldComponentUpdate() {',
117+
' return true;',
118+
' }',
119+
' };',
120+
'}'
121+
].join('\n'),
122+
errors: [{message: errorMessage('Bar')}],
123+
parserOptions: parserOptions
124+
},
125+
{
126+
code: [
127+
'function Foo() {',
128+
' return class Bar extends PureComponent {',
129+
' shouldComponentUpdate() {',
130+
' return true;',
131+
' }',
132+
' };',
133+
'}'
134+
].join('\n'),
135+
errors: [{message: errorMessage('Bar')}],
136+
parserOptions: parserOptions
137+
},
138+
{
139+
code: [
140+
'var Foo = class extends PureComponent {',
141+
' shouldComponentUpdate() {',
142+
' return true;',
143+
' }',
144+
'}'
145+
].join('\n'),
146+
errors: [{message: errorMessage('Foo')}],
147+
parserOptions: parserOptions
148+
}
149+
]
150+
});

0 commit comments

Comments
 (0)