Merge pull request #1374 from jaaberg/no-access-state-in-setstate

Rule proposal: no-access-state-in-setstate

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

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})
+```

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'),

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;
+          }
+        }
+      }
+    };
+  }
+};

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.'
+    }]
+  }]
+});