diff --git a/README.md b/README.md index eb9bc6c216..469ef3ddfa 100644 --- a/README.md +++ b/README.md @@ -90,6 +90,7 @@ Finally, enable all of the rules that you would like to use. Use [our preset](# * [react/forbid-elements](docs/rules/forbid-elements.md): Forbid certain elements * [react/forbid-prop-types](docs/rules/forbid-prop-types.md): Forbid certain propTypes * [react/forbid-foreign-prop-types](docs/rules/forbid-foreign-prop-types.md): Forbid foreign propTypes +* [react/no-access-state-in-setstate](docs/rules/no-access-state-in-setstate.md): Prevent using this.state inside this.setState * [react/no-array-index-key](docs/rules/no-array-index-key.md): Prevent using Array index in `key` props * [react/no-children-prop](docs/rules/no-children-prop.md): Prevent passing children as props * [react/no-danger](docs/rules/no-danger.md): Prevent usage of dangerous JSX properties diff --git a/docs/rules/no-access-state-in-setstate.md b/docs/rules/no-access-state-in-setstate.md new file mode 100644 index 0000000000..03af0a2b1d --- /dev/null +++ b/docs/rules/no-access-state-in-setstate.md @@ -0,0 +1,39 @@ +# Prevent using this.state within a this.setState (no-access-state-in-setstate) + +This rule should prevent usage of `this.state` inside `setState` calls. +Such usage of `this.state` might result in errors when two state calls are +called in batch and thus referencing old state and not the current +state. An example can be an increment function: + +``` +function increment() { + this.setState({value: this.state.value + 1}); +} +``` + +If these two `setState` operations is grouped together in a batch it will +look be something like the following, given that value is 1: + +``` +setState({value: 1 + 1}) +setState({value: 1 + 1}) +``` + +This can be avoided with using callbacks which takes the previous state +as first argument: + +``` +function increment() { + this.setState(prevState => ({value: prevState.value + 1})); +} +``` + +Then react will call the argument with the correct and updated state, +even when things happen in batches. And the example above will be +something like: + + +``` +setState({value: 1 + 1}) +setState({value: 2 + 1}) +``` diff --git a/index.js b/index.js index 2b12eaf88e..a798443eb4 100644 --- a/index.js +++ b/index.js @@ -38,6 +38,7 @@ const allRules = { 'jsx-uses-react': require('./lib/rules/jsx-uses-react'), 'jsx-uses-vars': require('./lib/rules/jsx-uses-vars'), 'jsx-wrap-multilines': require('./lib/rules/jsx-wrap-multilines'), + 'no-access-state-in-setstate': require('./lib/rules/no-access-state-in-setstate'), 'no-array-index-key': require('./lib/rules/no-array-index-key'), 'no-children-prop': require('./lib/rules/no-children-prop'), 'no-danger': require('./lib/rules/no-danger'), diff --git a/lib/rules/no-access-state-in-setstate.js b/lib/rules/no-access-state-in-setstate.js new file mode 100644 index 0000000000..f87f393f6f --- /dev/null +++ b/lib/rules/no-access-state-in-setstate.js @@ -0,0 +1,142 @@ +/** + * @fileoverview Prevent usage of this.state within setState + * @author Rolf Erik Lekang, Jørgen Aaberg + */ + +'use strict'; + +// ------------------------------------------------------------------------------ +// Rule Definition +// ------------------------------------------------------------------------------ + +module.exports = { + meta: { + docs: { + description: 'Reports when this.state is accessed within setState', + category: 'Possible Errors', + recommended: false + } + }, + + create: function(context) { + function isSetStateCall(node) { + return node.type === 'CallExpression' && + node.callee.property && + node.callee.property.name === 'setState' && + node.callee.object.type === 'ThisExpression'; + } + + // The methods array contains all methods or functions that are using this.state + // or that are calling another method or function using this.state + const methods = []; + // The vars array contains all variables that contains this.state + const vars = []; + return { + CallExpression(node) { + // Appends all the methods that are calling another + // method containg this.state to the methods array + methods.map(method => { + if (node.callee.name === method.methodName) { + let current = node.parent; + while (current.type !== 'Program') { + if (current.type === 'MethodDefinition') { + methods.push({ + methodName: current.key.name, + node: method.node + }); + break; + } + current = current.parent; + } + } + }); + + // Finding all CallExpressions that is inside a setState + // to further check if they contains this.state + let current = node.parent; + while (current.type !== 'Program') { + if (isSetStateCall(current)) { + const methodName = node.callee.name; + methods.map(method => { + if (method.methodName === methodName) { + context.report( + method.node, + 'Use callback in setState when referencing the previous state.' + ); + } + }); + + break; + } + current = current.parent; + } + }, + + MemberExpression(node) { + if ( + node.property.name === 'state' && + node.object.type === 'ThisExpression' + ) { + let current = node; + while (current.type !== 'Program') { + // Reporting if this.state is directly within this.setState + if (isSetStateCall(current)) { + context.report( + node, + 'Use callback in setState when referencing the previous state.' + ); + break; + } + + // Storing all functions and methods that contains this.state + if (current.type === 'MethodDefinition') { + methods.push({ + methodName: current.key.name, + node: node + }); + break; + } else if (current.type === 'FunctionExpression') { + methods.push({ + methodName: current.parent.key.name, + node: node + }); + break; + } + + // Storing all variables containg this.state + if (current.type === 'VariableDeclarator') { + vars.push({ + node: node, + scope: context.getScope() + }); + break; + } + + current = current.parent; + } + } + }, + + Identifier(node) { + // Checks if the identifier is a variable within an object + let current = node; + while (current.parent.type === 'BinaryExpression') { + current = current.parent; + } + if (current.parent.value === current) { + while (current.type !== 'Program') { + if (isSetStateCall(current)) { + vars + .filter(v => v.scope === context.getScope()) + .map(v => context.report( + v.node, + 'Use callback in setState when referencing the previous state.' + )); + } + current = current.parent; + } + } + } + }; + } +}; diff --git a/tests/lib/rules/no-access-state-in-setstate.js b/tests/lib/rules/no-access-state-in-setstate.js new file mode 100644 index 0000000000..70f56d2386 --- /dev/null +++ b/tests/lib/rules/no-access-state-in-setstate.js @@ -0,0 +1,119 @@ +/** + * @fileoverview Prevent usage of this.state within setState + * @author Rolf Erik Lekang, Jørgen Aaberg + */ +'use strict'; + +// ------------------------------------------------------------------------------ +// Requirements +// ------------------------------------------------------------------------------ + +const rule = require('../../../lib/rules/no-access-state-in-setstate'); +const RuleTester = require('eslint').RuleTester; + +const parserOptions = { + ecmaVersion: 6, + ecmaFeatures: { + jsx: true + } +}; + +// ------------------------------------------------------------------------------ +// Tests +// ------------------------------------------------------------------------------ + +const ruleTester = new RuleTester(); +ruleTester.run('no-access-state-in-setstate', rule, { + valid: [{ + code: [ + 'var Hello = React.createClass({', + ' onClick: function() {', + ' this.setState(state => ({value: state.value + 1}))', + ' }', + '});' + ].join('\n'), + parserOptions: parserOptions + }, { + code: [ + 'var Hello = React.createClass({', + ' multiplyValue: function(obj) {', + ' return obj.value*2', + ' },', + ' onClick: function() {', + ' var value = this.state.value', + ' this.multiplyValue({ value: value })', + ' }', + '});' + ].join('\n'), + parserOptions: parserOptions + }], + + invalid: [{ + code: [ + 'var Hello = React.createClass({', + ' onClick: function() {', + ' this.setState({value: this.state.value + 1})', + ' }', + '});' + ].join('\n'), + parserOptions: parserOptions, + errors: [{ + message: 'Use callback in setState when referencing the previous state.' + }] + }, { + code: [ + 'var Hello = React.createClass({', + ' onClick: function() {', + ' this.setState(() => ({value: this.state.value + 1}))', + ' }', + '});' + ].join('\n'), + parserOptions: parserOptions, + errors: [{ + message: 'Use callback in setState when referencing the previous state.' + }] + }, { + code: [ + 'var Hello = React.createClass({', + ' onClick: function() {', + ' var nextValue = this.state.value + 1', + ' this.setState({value: nextValue})', + ' }', + '});' + ].join('\n'), + parserOptions: parserOptions, + errors: [{ + message: 'Use callback in setState when referencing the previous state.' + }] + }, { + code: [ + 'function nextState(state) {', + ' return {value: state.value + 1}', + '}', + 'var Hello = React.createClass({', + ' onClick: function() {', + ' this.setState(nextState(this.state))', + ' }', + '});' + ].join('\n'), + parserOptions: parserOptions, + errors: [{ + message: 'Use callback in setState when referencing the previous state.' + }] + }, { + code: [ + 'var Hello = React.createClass({', + ' nextState: function() {', + ' return {value: this.state.value + 1}', + ' },', + ' onClick: function() {', + ' this.setState(nextState())', + ' }', + '});' + ].join('\n'), + parserOptions: parserOptions, + errors: [{ + message: 'Use callback in setState when referencing the previous state.' + }] + }] +});