diff --git a/README.md b/README.md index bd7b856be9..caa1ac900c 100644 --- a/README.md +++ b/README.md @@ -96,6 +96,7 @@ Finally, enable all of the rules that you would like to use. Use [our preset](# * [react/no-find-dom-node](docs/rules/no-find-dom-node.md): Prevent usage of `findDOMNode` * [react/no-is-mounted](docs/rules/no-is-mounted.md): Prevent usage of `isMounted` * [react/no-multi-comp](docs/rules/no-multi-comp.md): Prevent multiple component definition per file +* [react/no-redundant-should-component-update](docs/rules/no-redundant-should-component-update.md): Prevent usage of `shouldComponentUpdate` when extending React.PureComponent * [react/no-render-return-value](docs/rules/no-render-return-value.md): Prevent usage of the return value of `React.render` * [react/no-set-state](docs/rules/no-set-state.md): Prevent usage of `setState` * [react/no-string-refs](docs/rules/no-string-refs.md): Prevent using string references in `ref` attribute. diff --git a/docs/rules/no-redundant-should-component-update.md b/docs/rules/no-redundant-should-component-update.md new file mode 100644 index 0000000000..a57b6f7eba --- /dev/null +++ b/docs/rules/no-redundant-should-component-update.md @@ -0,0 +1,64 @@ +# Prevent usage of shouldComponentUpdate when extending React.PureComponent (react/no-redundant-should-component-update) + +Warns if you have `shouldComponentUpdate` defined when defining a component that extends React.PureComponent. +While having `shouldComponentUpdate` will still work, it becomes pointless to extend PureComponent. + +## Rule Details + +The following patterns are considered warnings: + +```jsx +class Foo extends React.PureComponent { + shouldComponentUpdate() { + // do check + } + + render() { + return
Radical!
+ } +} + +function Bar() { + return class Baz extends React.PureComponent { + shouldComponentUpdate() { + // do check + } + + render() { + return
Groovy!
+ } + } +} +``` + +The following patterns are not considered warnings: + +```jsx +class Foo extends React.Component { + shouldComponentUpdate() { + // do check + } + + render() { + return
Radical!
+ } +} + +function Bar() { + return class Baz extends React.Component { + shouldComponentUpdate() { + // do check + } + + render() { + return
Groovy!
+ } + } +} + +class Qux extends React.PureComponent { + render() { + return
Tubular!
+ } +} +``` diff --git a/index.js b/index.js index 85385275df..da3089ec8d 100644 --- a/index.js +++ b/index.js @@ -62,7 +62,8 @@ var allRules = { 'no-unused-prop-types': require('./lib/rules/no-unused-prop-types'), 'no-children-prop': require('./lib/rules/no-children-prop'), 'void-dom-elements-no-children': require('./lib/rules/void-dom-elements-no-children'), - 'jsx-tag-spacing': require('./lib/rules/jsx-tag-spacing') + 'jsx-tag-spacing': require('./lib/rules/jsx-tag-spacing'), + 'no-redundant-should-component-update': require('./lib/rules/no-redundant-should-component-update') }; function filterRules(rules, predicate) { diff --git a/lib/rules/no-redundant-should-component-update.js b/lib/rules/no-redundant-should-component-update.js new file mode 100644 index 0000000000..dfafb05f72 --- /dev/null +++ b/lib/rules/no-redundant-should-component-update.js @@ -0,0 +1,109 @@ +/** + * @fileoverview Flag shouldComponentUpdate when extending PureComponent + */ +'use strict'; + +var Components = require('../util/Components'); + +function errorMessage(node) { + return `${node} does not need shouldComponentUpdate when extending React.PureComponent.`; +} + +// ------------------------------------------------------------------------------ +// Rule Definition +// ------------------------------------------------------------------------------ + +module.exports = { + meta: { + docs: { + description: 'Flag shouldComponentUpdate when extending PureComponent', + category: 'Possible Errors', + recommended: false + }, + schema: [] + }, + + create: Components.detect(function(context, components, utils) { + + /** + * Get properties name + * @param {Object} node - Property. + * @returns {String} Property name. + */ + function getPropertyName(node) { + if (node.key) { + return node.key.name; + } else if (node.type === 'ClassProperty') { + // Special case for class properties + // (babel-eslint does not expose property name so we have to rely on tokens) + var tokens = context.getFirstTokens(node, 2); + return tokens[1] && tokens[1].type === 'Identifier' ? tokens[1].value : tokens[0].value; + } + return ''; + } + + /** + * Get properties for a given AST node + * @param {ASTNode} node The AST node being checked. + * @returns {Array} Properties array. + */ + function getComponentProperties(node) { + switch (node.type) { + case 'ClassExpression': + case 'ClassDeclaration': + return node.body.body; + default: + return []; + } + } + + /** + * Checks for shouldComponentUpdate property + * @param {ASTNode} node The AST node being checked. + * @returns {Boolean} Whether or not the property exists. + */ + function hasShouldComponentUpdate(node) { + var properties = getComponentProperties(node); + return properties.some(function(property) { + var name = getPropertyName(property); + return name === 'shouldComponentUpdate'; + }); + } + + /** + * Get name of node if available + * @param {ASTNode} node The AST node being checked. + * @return {String} The name of the node + */ + function getNodeName(node) { + if (node.id) { + return node.id.name; + } else if (node.parent && node.parent.id) { + return node.parent.id.name; + } + return ''; + } + + /** + * Checks for violation of rule + * @param {ASTNode} node The AST node being checked. + */ + function checkForViolation(node) { + if (utils.isPureComponent(node)) { + var hasScu = hasShouldComponentUpdate(node); + if (hasScu) { + var className = getNodeName(node); + context.report({ + node: node, + message: errorMessage(className) + }); + } + } + } + + return { + ClassDeclaration: checkForViolation, + ClassExpression: checkForViolation + }; + }) +}; diff --git a/tests/lib/rules/no-redundant-should-component-update.js b/tests/lib/rules/no-redundant-should-component-update.js new file mode 100644 index 0000000000..64a267998e --- /dev/null +++ b/tests/lib/rules/no-redundant-should-component-update.js @@ -0,0 +1,150 @@ +/** + * @fileoverview Tests for no-redundant-should-component-update + */ + +'use strict'; + +// ----------------------------------------------------------------------------- +// Requirements +// ----------------------------------------------------------------------------- + +var rule = require('../../../lib/rules/no-redundant-should-component-update'); +var RuleTester = require('eslint').RuleTester; + +var parserOptions = { + ecmaVersion: 6, + ecmaFeatures: { + experimentalObjectRestSpread: true, + jsx: true + } +}; + +function errorMessage(node) { + return `${node} does not need shouldComponentUpdate when extending React.PureComponent.`; +} + +// ----------------------------------------------------------------------------- +// Tests +// ----------------------------------------------------------------------------- + +var ruleTester = new RuleTester(); +ruleTester.run('no-redundant-should-component-update', rule, { + valid: [ + { + code: [ + 'class Foo extends React.Component {', + ' shouldComponentUpdate() {', + ' return true;', + ' }', + '}' + ].join('\n'), + parserOptions: parserOptions + }, + { + code: [ + 'class Foo extends React.Component {', + ' shouldComponentUpdate = () => {', + ' return true;', + ' }', + '}' + ].join('\n'), + parser: 'babel-eslint', + parserOptions: parserOptions + }, + { + code: [ + 'class Foo extends React.Component {', + ' shouldComponentUpdate() {', + ' return true;', + ' }', + '}' + ].join('\n'), + parserOptions: parserOptions + }, + { + code: [ + 'function Foo() {', + ' return class Bar extends React.Component {', + ' shouldComponentUpdate() {', + ' return true;', + ' }', + ' };', + '}' + ].join('\n'), + parserOptions: parserOptions + } + ], + invalid: [ + { + code: [ + 'class Foo extends React.PureComponent {', + ' shouldComponentUpdate() {', + ' return true;', + ' }', + '}' + ].join('\n'), + errors: [{message: errorMessage('Foo')}], + parserOptions: parserOptions + }, + { + code: [ + 'class Foo extends PureComponent {', + ' shouldComponentUpdate() {', + ' return true;', + ' }', + '}' + ].join('\n'), + errors: [{message: errorMessage('Foo')}], + parserOptions: parserOptions + }, + { + code: [ + 'class Foo extends React.PureComponent {', + ' shouldComponentUpdate = () => {', + ' return true;', + ' }', + '}' + ].join('\n'), + errors: [{message: errorMessage('Foo')}], + parser: 'babel-eslint', + parserOptions: parserOptions + }, + { + code: [ + 'function Foo() {', + ' return class Bar extends React.PureComponent {', + ' shouldComponentUpdate() {', + ' return true;', + ' }', + ' };', + '}' + ].join('\n'), + errors: [{message: errorMessage('Bar')}], + parserOptions: parserOptions + }, + { + code: [ + 'function Foo() {', + ' return class Bar extends PureComponent {', + ' shouldComponentUpdate() {', + ' return true;', + ' }', + ' };', + '}' + ].join('\n'), + errors: [{message: errorMessage('Bar')}], + parserOptions: parserOptions + }, + { + code: [ + 'var Foo = class extends PureComponent {', + ' shouldComponentUpdate() {', + ' return true;', + ' }', + '}' + ].join('\n'), + errors: [{message: errorMessage('Foo')}], + parserOptions: parserOptions + } + ] +});