From 9bec9e90d7a4dcf25fa20e369b00bca00e04145b Mon Sep 17 00:00:00 2001 From: Rolf Erik Lekang Date: Tue, 20 Sep 2016 21:19:08 +0200 Subject: [PATCH 1/4] wip: Add new rule 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 is 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. 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}) --- index.js | 1 + lib/rules/no-access-state-in-setstate.js | 48 ++++++++ .../lib/rules/no-access-state-in-setstate.js | 106 ++++++++++++++++++ 3 files changed, 155 insertions(+) create mode 100644 lib/rules/no-access-state-in-setstate.js create mode 100644 tests/lib/rules/no-access-state-in-setstate.js diff --git a/index.js b/index.js index 4a1e118c9b..bb13066e34 100644 --- a/index.js +++ b/index.js @@ -35,6 +35,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..bc8b6342df --- /dev/null +++ b/lib/rules/no-access-state-in-setstate.js @@ -0,0 +1,48 @@ +/** + * @fileoverview Prevent usage of this.state within setState + * @author Rolf Erik Lekang + */ + +'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 && + node.callee.property && + node.callee.property.name === 'setState'; + } + + return { + ThisExpression: function(node) { + var memberExpression = node.parent; + if (memberExpression.property.name === 'state') { + var current = memberExpression; + while (current.type !== 'Program') { + if (isSetStateCall(current)) { + context.report({ + node: memberExpression, + message: 'Use callback in setState when referencing the previous state.' + }); + break; + } + 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..531c90e703 --- /dev/null +++ b/tests/lib/rules/no-access-state-in-setstate.js @@ -0,0 +1,106 @@ +/** + * @fileoverview Prevent usage of this.state within setState + * @author Rolf Erik Lekang + */ +'use strict'; + +// ------------------------------------------------------------------------------ +// Requirements +// ------------------------------------------------------------------------------ + +var rule = require('../../../lib/rules/no-access-state-in-setstate'); +var RuleTester = require('eslint').RuleTester; + +var parserOptions = { + ecmaVersion: 6, + ecmaFeatures: { + jsx: true + } +}; + +// ------------------------------------------------------------------------------ +// Tests +// ------------------------------------------------------------------------------ + +var 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 + }], + + 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.' + }] + }] +}); From 2d53aa75e8b338e43d45d962b8231b09b4610899 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B8rgen=20Aaberg?= Date: Wed, 9 Aug 2017 16:06:02 +0200 Subject: [PATCH 2/4] Complete rule no-access-state-in-setstate --- lib/rules/no-access-state-in-setstate.js | 114 ++++++++++++++++-- .../lib/rules/no-access-state-in-setstate.js | 23 +++- 2 files changed, 122 insertions(+), 15 deletions(-) diff --git a/lib/rules/no-access-state-in-setstate.js b/lib/rules/no-access-state-in-setstate.js index bc8b6342df..f87f393f6f 100644 --- a/lib/rules/no-access-state-in-setstate.js +++ b/lib/rules/no-access-state-in-setstate.js @@ -1,6 +1,6 @@ /** * @fileoverview Prevent usage of this.state within setState - * @author Rolf Erik Lekang + * @author Rolf Erik Lekang, Jørgen Aaberg */ 'use strict'; @@ -21,23 +21,117 @@ module.exports = { create: function(context) { function isSetStateCall(node) { return node.type === 'CallExpression' && - node.callee && node.callee.property && - node.callee.property.name === 'setState'; + 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 { - ThisExpression: function(node) { - var memberExpression = node.parent; - if (memberExpression.property.name === 'state') { - var current = memberExpression; + 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: memberExpression, - message: 'Use callback in setState when referencing the previous state.' + 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 index 531c90e703..70f56d2386 100644 --- a/tests/lib/rules/no-access-state-in-setstate.js +++ b/tests/lib/rules/no-access-state-in-setstate.js @@ -1,6 +1,6 @@ /** * @fileoverview Prevent usage of this.state within setState - * @author Rolf Erik Lekang + * @author Rolf Erik Lekang, Jørgen Aaberg */ 'use strict'; @@ -8,10 +8,10 @@ // Requirements // ------------------------------------------------------------------------------ -var rule = require('../../../lib/rules/no-access-state-in-setstate'); -var RuleTester = require('eslint').RuleTester; +const rule = require('../../../lib/rules/no-access-state-in-setstate'); +const RuleTester = require('eslint').RuleTester; -var parserOptions = { +const parserOptions = { ecmaVersion: 6, ecmaFeatures: { jsx: true @@ -22,7 +22,7 @@ var parserOptions = { // Tests // ------------------------------------------------------------------------------ -var ruleTester = new RuleTester(); +const ruleTester = new RuleTester(); ruleTester.run('no-access-state-in-setstate', rule, { valid: [{ code: [ @@ -33,6 +33,19 @@ ruleTester.run('no-access-state-in-setstate', rule, { '});' ].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: [{ From 70fa3c8436e4b76f19f462b22adce545355bb272 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B8rgen=20Aaberg?= Date: Tue, 15 Aug 2017 16:50:35 +0200 Subject: [PATCH 3/4] Add docs for no-access-state-in-setstate --- README.md | 1 + docs/rules/no-access-state-in-setstate.md | 39 +++++++++++++++++++++++ 2 files changed, 40 insertions(+) create mode 100644 docs/rules/no-access-state-in-setstate.md diff --git a/README.md b/README.md index 6bb0836761..92be7dbf6c 100644 --- a/README.md +++ b/README.md @@ -88,6 +88,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..5a5d75d217 --- /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 is +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}) +``` From df92a494b6068fba5cd4bd9c79d192a0a2de881c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B8rgen=20Aaberg?= Date: Wed, 23 Aug 2017 08:36:25 +0200 Subject: [PATCH 4/4] Use code blocks in docs for code references --- docs/rules/no-access-state-in-setstate.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/rules/no-access-state-in-setstate.md b/docs/rules/no-access-state-in-setstate.md index 5a5d75d217..03af0a2b1d 100644 --- a/docs/rules/no-access-state-in-setstate.md +++ b/docs/rules/no-access-state-in-setstate.md @@ -1,7 +1,7 @@ # 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 is +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: @@ -11,7 +11,7 @@ function increment() { } ``` -If these two setState operations is grouped together in a batch it will +If these two `setState` operations is grouped together in a batch it will look be something like the following, given that value is 1: ```