From 9fccdf9020e3bf5db0869ab35e8b4cb5135b2780 Mon Sep 17 00:00:00 2001 From: Valentin Agachi Date: Fri, 5 Aug 2016 13:47:24 +0100 Subject: [PATCH] no-did-update|mount-set-state: disallow-via-methods option --- docs/rules/no-did-mount-set-state.md | 37 +++++- docs/rules/no-did-update-set-state.md | 37 +++++- lib/rules/no-did-mount-set-state.js | 26 +++- lib/rules/no-did-update-set-state.js | 27 ++-- lib/util/no-set-state-via-methods.js | 119 +++++++++++++++++ tests/lib/rules/no-did-mount-set-state.js | 140 ++++++++++++++++++++ tests/lib/rules/no-did-update-set-state.js | 142 ++++++++++++++++++++- 7 files changed, 509 insertions(+), 19 deletions(-) create mode 100644 lib/util/no-set-state-via-methods.js diff --git a/docs/rules/no-did-mount-set-state.md b/docs/rules/no-did-mount-set-state.md index 7af9e6320e..f80891d03a 100644 --- a/docs/rules/no-did-mount-set-state.md +++ b/docs/rules/no-did-mount-set-state.md @@ -53,7 +53,7 @@ var Hello = React.createClass({ ```js ... -"no-did-mount-set-state": [, ] +"no-did-mount-set-state": [, , ] ... ``` @@ -90,3 +90,38 @@ var Hello = React.createClass({ } }); ``` + +### `disallow-via-methods` mode + +By default this rule ignores any call to `this.setState` via methods called in `componentDidMount`. The `disallow-via-methods` mode makes this rule more strict by disallowing calls to `this.setState` even within methods. + +The following patterns are considered warnings: + +```js +var Hello = React.createClass({ + componentDidMount: function() { + this.setState({ + name: this.props.name.toUpperCase() + }); + }, + render: function() { + return
Hello {this.state.name}
; + } +}); +``` + +```js +var Hello = React.createClass({ + componentDidMount: function() { + this.doSomethingToState(); + }, + doSomethingToState: function() { + this.setState({ + name: newName + }); + }, + render: function() { + return
Hello {this.state.name}
; + } +}); +``` diff --git a/docs/rules/no-did-update-set-state.md b/docs/rules/no-did-update-set-state.md index 60d398e818..5cf3c108e9 100644 --- a/docs/rules/no-did-update-set-state.md +++ b/docs/rules/no-did-update-set-state.md @@ -51,7 +51,7 @@ var Hello = React.createClass({ ```js ... -"no-did-update-set-state": [, ] +"no-did-update-set-state": [, , ] ... ``` @@ -88,3 +88,38 @@ var Hello = React.createClass({ } }); ``` + +### `disallow-via-methods` mode + +By default this rule ignores any call to `this.setState` via methods called in `componentDidUpdate`. The `disallow-via-methods` mode makes this rule more strict by disallowing calls to `this.setState` even within methods. + +The following patterns are considered warnings: + +```js +var Hello = React.createClass({ + componentDidUpdate: function() { + this.setState({ + name: this.props.name.toUpperCase() + }); + }, + render: function() { + return
Hello {this.state.name}
; + } +}); +``` + +```js +var Hello = React.createClass({ + componentDidUpdate: function() { + this.doSomethingToState(); + }, + doSomethingToState: function() { + this.setState({ + name: newName + }); + }, + render: function() { + return
Hello {this.state.name}
; + } +}); +``` diff --git a/lib/rules/no-did-mount-set-state.js b/lib/rules/no-did-mount-set-state.js index 648f9ec32e..9121cb5532 100644 --- a/lib/rules/no-did-mount-set-state.js +++ b/lib/rules/no-did-mount-set-state.js @@ -4,6 +4,14 @@ */ 'use strict'; +var Components = require('../util/Components'); +var noSetStateViaMethods = require('../util/no-set-state-via-methods'); + +var DISALLOW_IN_FUNCTIONS = 'disallow-in-func'; +var ALLOW_IN_FUNCTIONS = 'allow-in-func'; +var DISALLOW_VIA_METHODS = 'disallow-via-methods'; +var ALLOW_VIA_METHODS = 'allow-via-methods'; + // ------------------------------------------------------------------------------ // Rule Definition // ------------------------------------------------------------------------------ @@ -17,20 +25,21 @@ module.exports = { }, schema: [{ - enum: ['disallow-in-func'] + enum: [DISALLOW_IN_FUNCTIONS, ALLOW_IN_FUNCTIONS] + }, { + enum: [DISALLOW_VIA_METHODS, ALLOW_VIA_METHODS] }] }, - create: function(context) { - - var mode = context.options[0] || 'allow-in-func'; + create: Components.detect(function(context, components, utils) { + var modeInFunctions = context.options[0] || ALLOW_IN_FUNCTIONS; + var modeViaMethods = context.options[1] || ALLOW_VIA_METHODS; // -------------------------------------------------------------------------- // Public // -------------------------------------------------------------------------- return { - CallExpression: function(node) { var callee = node.callee; if ( @@ -49,7 +58,7 @@ module.exports = { if ( (ancestors[i].type !== 'Property' && ancestors[i].type !== 'MethodDefinition') || ancestors[i].key.name !== 'componentDidMount' || - (mode !== 'disallow-in-func' && depth > 1) + (modeInFunctions !== DISALLOW_IN_FUNCTIONS && depth > 1) ) { continue; } @@ -59,8 +68,11 @@ module.exports = { }); break; } + if (modeViaMethods === DISALLOW_VIA_METHODS) { + noSetStateViaMethods.run('componentDidMount', callee, ancestors, context, components, utils); + } } }; - } + }) }; diff --git a/lib/rules/no-did-update-set-state.js b/lib/rules/no-did-update-set-state.js index 53f096e3f1..7e5f0e95fb 100644 --- a/lib/rules/no-did-update-set-state.js +++ b/lib/rules/no-did-update-set-state.js @@ -4,6 +4,14 @@ */ 'use strict'; +var Components = require('../util/Components'); +var noSetStateViaMethods = require('../util/no-set-state-via-methods'); + +var DISALLOW_IN_FUNCTIONS = 'disallow-in-func'; +var ALLOW_IN_FUNCTIONS = 'allow-in-func'; +var DISALLOW_VIA_METHODS = 'disallow-via-methods'; +var ALLOW_VIA_METHODS = 'allow-via-methods'; + // ------------------------------------------------------------------------------ // Rule Definition // ------------------------------------------------------------------------------ @@ -17,20 +25,21 @@ module.exports = { }, schema: [{ - enum: ['disallow-in-func'] + enum: [DISALLOW_IN_FUNCTIONS, ALLOW_IN_FUNCTIONS] + }, { + enum: [DISALLOW_VIA_METHODS, ALLOW_VIA_METHODS] }] }, - create: function(context) { - - var mode = context.options[0] || 'allow-in-func'; + create: Components.detect(function(context, components, utils) { + var modeInFunctions = context.options[0] || ALLOW_IN_FUNCTIONS; + var modeViaMethods = context.options[1] || ALLOW_VIA_METHODS; // -------------------------------------------------------------------------- // Public // -------------------------------------------------------------------------- return { - CallExpression: function(node) { var callee = node.callee; if ( @@ -49,7 +58,7 @@ module.exports = { if ( (ancestors[i].type !== 'Property' && ancestors[i].type !== 'MethodDefinition') || ancestors[i].key.name !== 'componentDidUpdate' || - (mode !== 'disallow-in-func' && depth > 1) + (modeInFunctions !== DISALLOW_IN_FUNCTIONS && depth > 1) ) { continue; } @@ -59,8 +68,10 @@ module.exports = { }); break; } + if (modeViaMethods === DISALLOW_VIA_METHODS) { + noSetStateViaMethods.run('componentDidUpdate', callee, ancestors, context, components, utils); + } } }; - - } + }) }; diff --git a/lib/util/no-set-state-via-methods.js b/lib/util/no-set-state-via-methods.js new file mode 100644 index 0000000000..63620ebb97 --- /dev/null +++ b/lib/util/no-set-state-via-methods.js @@ -0,0 +1,119 @@ +/** + * @fileoverview Prevent usage of findDOMNode + * @author Valentin Agachi + */ + +'use strict'; + +function getCalleeName(node) { + if (!node.callee) { + return null; + } + var callee = node.callee; + if ( + callee.type !== 'MemberExpression' || + ( + callee.object.type !== 'ThisExpression' && + callee.object.type !== 'Identifier' + ) || + callee.property.type !== 'Identifier' + ) { + return null; + } + + var obj = callee.object.type === 'ThisExpression' + ? 'this' + : callee.object.name; + var prop = callee.property.name; + + return obj + '.' + prop; +} + +function isMethodDefinition(node) { + return ( + node.type === 'MethodDefinition' && + node.kind === 'method' && + !node.static + ); +} +function isObjectPropertyMethod(node) { + return ( + node.type === 'Property' && + node.method + ); +} + +function functionExpressionCallsExpression(node, methodName) { + var verifyBodyNode = function(bodyNode) { + switch (bodyNode.type) { + case 'ExpressionStatement': + if (bodyNode.expression.type === 'CallExpression') { + return getCalleeName(bodyNode.expression) === 'this.' + methodName; + } + break; + case 'IfStatement': + return ( + bodyNode.consequent.body.some(verifyBodyNode) || + (bodyNode.alternate && bodyNode.alternate.body.some(verifyBodyNode)) + ); + default: + return false; + } + return false; + }; + + return node.value.body.body.some(verifyBodyNode); +} + +function lifecycleCallsMethod(lifecycleMethod, methodName) { + return function(node) { + var isLifecycleMethod = ( + ( + isMethodDefinition(node) || + isObjectPropertyMethod(node) + ) && + node.key.name === lifecycleMethod + ); + return isLifecycleMethod && + functionExpressionCallsExpression(node, methodName); + }; +} + +module.exports = { + run: function(lifecycleMethod, callee, ancestors, context, components, utils) { + var method; + var methodName; + + var es6Class = utils.getParentES6Component(); + if (es6Class) { + method = ancestors.find(isMethodDefinition); + methodName = method.key.name; + var classBody = es6Class.body.body; + // Loop through all the component's methods to find componentDidUpdate + // and search for call expressions of `methodName` + if (classBody.some(lifecycleCallsMethod(lifecycleMethod, methodName))) { + context.report({ + node: callee, + message: 'Do not use `setState` in other methods called in `' + lifecycleMethod + '`' + }); + } + return; + } + + var es5Class = utils.getParentES5Component(); + if (es5Class) { + method = ancestors.find(isObjectPropertyMethod); + methodName = method.key.name; + // Loop through all the component's methods to find componentDidUpdate + // and search for call expressions of `methodName` + var classProperties = es5Class.properties; + if (classProperties.some(lifecycleCallsMethod(lifecycleMethod, methodName))) { + context.report({ + node: callee, + message: 'Do not use `setState` in other methods called in `' + lifecycleMethod + '`' + }); + } + return; + } + } +}; diff --git a/tests/lib/rules/no-did-mount-set-state.js b/tests/lib/rules/no-did-mount-set-state.js index 0011171aff..f760a6ed7e 100644 --- a/tests/lib/rules/no-did-mount-set-state.js +++ b/tests/lib/rules/no-did-mount-set-state.js @@ -81,6 +81,77 @@ ruleTester.run('no-did-mount-set-state', rule, { ].join('\n'), parser: 'babel-eslint', parserOptions: parserOptions + }, { + code: [ + 'class Foo extends React.Component {', + ' componentDidMount() {', + ' this._doNothing();', + ' }', + ' _doNothing() {}', + '}' + ].join('\n'), + parserOptions: parserOptions + }, { + code: [ + 'class Foo extends React.Component {', + ' componentDidMount() {', + ' if (false) {}', + ' }', + ' _resetState() {', + ' this.setState({foo: 123});', + ' }', + '}' + ].join('\n'), + parserOptions: parserOptions + }, { + code: [ + 'class Foo extends React.Component {', + ' componentDidMount() {', + ' this._resetState();', + ' }', + ' _resetState() {', + ' this.setState({foo: 123});', + ' }', + '}' + ].join('\n'), + options: ['allow-in-func', 'allow-via-methods'], + parserOptions: parserOptions + }, { + code: [ + 'var Foo = React.createClass({', + ' componentDidMount() {', + ' this._doNothing();', + ' },', + ' _doNothing() {}', + '});' + ].join('\n'), + parserOptions: parserOptions + }, + { + code: [ + 'var Foo = React.createClass({', + ' componentDidMount() {', + ' if (false) {}', + ' },', + ' _resetState() {', + ' this.setState({foo: 123});', + ' }', + '});' + ].join('\n'), + parserOptions: parserOptions + }, { + code: [ + 'var Foo = React.createClass({', + ' componentDidMount() {', + ' this._resetState();', + ' },', + ' _resetState() {', + ' this.setState({foo: 123});', + ' }', + '});' + ].join('\n'), + options: ['allow-in-func', 'allow-via-methods'], + parserOptions: parserOptions }], invalid: [{ @@ -234,5 +305,74 @@ ruleTester.run('no-did-mount-set-state', rule, { errors: [{ message: 'Do not use setState in componentDidMount' }] + }, { + code: [ + 'class Foo extends React.Component {', + ' componentDidMount() {', + ' this._resetState();', + ' }', + ' _resetState() {', + ' this.setState({foo: 123});', + ' }', + '}' + ].join('\n'), + options: ['allow-in-func', 'disallow-via-methods'], + parserOptions: parserOptions, + errors: [{ + message: 'Do not use `setState` in other methods called in `componentDidMount`' + }] + }, { + code: [ + 'class Foo extends React.Component {', + ' componentDidMount() {', + ' if (true) {', + ' this._resetState();', + ' } else {', + ' }', + ' }', + ' _resetState() {', + ' this.setState({foo: 123});', + ' }', + '}' + ].join('\n'), + options: ['allow-in-func', 'disallow-via-methods'], + parserOptions: parserOptions, + errors: [{ + message: 'Do not use `setState` in other methods called in `componentDidMount`' + }] + }, { + code: [ + 'var Foo = React.createClass({', + ' componentDidMount() {', + ' this._resetState();', + ' },', + ' _resetState() {', + ' this.setState({foo: 123});', + ' }', + '});' + ].join('\n'), + options: ['allow-in-func', 'disallow-via-methods'], + parserOptions: parserOptions, + errors: [{ + message: 'Do not use `setState` in other methods called in `componentDidMount`' + }] + }, { + code: [ + 'var Foo = React.createClass({', + ' componentDidMount() {', + ' if (true) {', + ' this._resetState();', + ' }', + ' },', + ' _resetState() {', + ' this.setState({foo: 123});', + ' }', + '});' + ].join('\n'), + options: ['allow-in-func', 'disallow-via-methods'], + parserOptions: parserOptions, + errors: [{ + message: 'Do not use `setState` in other methods called in `componentDidMount`' + }] }] }); diff --git a/tests/lib/rules/no-did-update-set-state.js b/tests/lib/rules/no-did-update-set-state.js index 4345bd54f5..dde4fc6d5a 100644 --- a/tests/lib/rules/no-did-update-set-state.js +++ b/tests/lib/rules/no-did-update-set-state.js @@ -79,7 +79,77 @@ ruleTester.run('no-did-update-set-state', rule, { ' }', '});' ].join('\n'), - parser: 'babel-eslint', + parserOptions: parserOptions + }, { + code: [ + 'class Foo extends React.Component {', + ' componentDidUpdate() {', + ' this._doNothing();', + ' }', + ' _doNothing() {}', + '}' + ].join('\n'), + parserOptions: parserOptions + }, { + code: [ + 'class Foo extends React.Component {', + ' componentDidUpdate() {', + ' if (false) {}', + ' }', + ' _resetState() {', + ' this.setState({foo: 123});', + ' }', + '}' + ].join('\n'), + parserOptions: parserOptions + }, { + code: [ + 'class Foo extends React.Component {', + ' componentDidUpdate() {', + ' this._resetState();', + ' }', + ' _resetState() {', + ' this.setState({foo: 123});', + ' }', + '}' + ].join('\n'), + options: ['allow-in-func', 'allow-via-methods'], + parserOptions: parserOptions + }, { + code: [ + 'var Foo = React.createClass({', + ' componentDidUpdate() {', + ' this._doNothing();', + ' },', + ' _doNothing() {}', + '});' + ].join('\n'), + parserOptions: parserOptions + }, + { + code: [ + 'var Foo = React.createClass({', + ' componentDidUpdate() {', + ' if (false) {}', + ' },', + ' _resetState() {', + ' this.setState({foo: 123});', + ' }', + '});' + ].join('\n'), + parserOptions: parserOptions + }, { + code: [ + 'var Foo = React.createClass({', + ' componentDidUpdate() {', + ' this._resetState();', + ' },', + ' _resetState() {', + ' this.setState({foo: 123});', + ' }', + '});' + ].join('\n'), + options: ['allow-in-func', 'allow-via-methods'], parserOptions: parserOptions }], @@ -215,7 +285,6 @@ ruleTester.run('no-did-update-set-state', rule, { ' }', '});' ].join('\n'), - parser: 'babel-eslint', parserOptions: parserOptions, options: ['disallow-in-func'], errors: [{ @@ -234,5 +303,74 @@ ruleTester.run('no-did-update-set-state', rule, { errors: [{ message: 'Do not use setState in componentDidUpdate' }] + }, { + code: [ + 'class Foo extends React.Component {', + ' componentDidUpdate() {', + ' this._resetState();', + ' }', + ' _resetState() {', + ' this.setState({foo: 123});', + ' }', + '}' + ].join('\n'), + options: ['allow-in-func', 'disallow-via-methods'], + parserOptions: parserOptions, + errors: [{ + message: 'Do not use `setState` in other methods called in `componentDidUpdate`' + }] + }, { + code: [ + 'class Foo extends React.Component {', + ' componentDidUpdate() {', + ' if (true) {', + ' this._resetState();', + ' } else {', + ' }', + ' }', + ' _resetState() {', + ' this.setState({foo: 123});', + ' }', + '}' + ].join('\n'), + options: ['allow-in-func', 'disallow-via-methods'], + parserOptions: parserOptions, + errors: [{ + message: 'Do not use `setState` in other methods called in `componentDidUpdate`' + }] + }, { + code: [ + 'var Foo = React.createClass({', + ' componentDidUpdate() {', + ' this._resetState();', + ' },', + ' _resetState() {', + ' this.setState({foo: 123});', + ' }', + '});' + ].join('\n'), + options: ['allow-in-func', 'disallow-via-methods'], + parserOptions: parserOptions, + errors: [{ + message: 'Do not use `setState` in other methods called in `componentDidUpdate`' + }] + }, { + code: [ + 'var Foo = React.createClass({', + ' componentDidUpdate() {', + ' if (true) {', + ' this._resetState();', + ' }', + ' },', + ' _resetState() {', + ' this.setState({foo: 123});', + ' }', + '});' + ].join('\n'), + options: ['allow-in-func', 'disallow-via-methods'], + parserOptions: parserOptions, + errors: [{ + message: 'Do not use `setState` in other methods called in `componentDidUpdate`' + }] }] });