feat: new rule - mapStateToProps-prefer-hoisted (#21)

* feat: new rule - mapStateToProps-prefer-hoisted

* fix: corner case with returning an empty object

* fix: fixing the logic

Author: DianaSuvorova

README.md

@@ -51,5 +51,6 @@ To configure individual rules:
 * [react-redux/mapDispatchToProps-prefer-object](docs/rules/mapDispatchToProps-prefer-object.md) Enforces that mapDispatchToProps returns an object.
 * [react-redux/mapDispatchToProps-prefer-parameters-names](docs/rules/mapDispatchToProps-prefer-parameters-names.md)  Enforces that all mapDispatchToProps parameters have specific names.
 * [react-redux/mapStateToProps-no-store](docs/rules/mapStateToProps-no-store.md) Prohibits binding a whole store object to a component.
+* [react-redux/mapStateToProps-prefer-hoisted](docs/rules/mapStateToProps-prefer-hoisted.md) Flags generation of copies of same-by-value but different-by-reference props.
 * [react-redux/mapStateToProps-prefer-parameters-names](docs/rules/mapStateToProps-prefer-parameters-names.md) Enforces that all mapStateToProps parameters have specific names.
 * [react-redux/prefer-separate-component-file](docs/rules/prefer-separate-component-file.md) Enforces that all connected components are defined in a separate file.

docs/rules/mapStateToProps-prefer-hoisted.md

@@ -0,0 +1,72 @@
+# Flags generation of copies of same-by-value but different-by-reference props (react-redux/mapStateToProps-prefer-hoisted)
+
+Primitives props like strings and numbers are compared by their value, while objects like arrays, dates, and plain objects are compared by their reference.
+
+In case when mapStateToProps creates a new "constant" (i.e. independent of `state` and `ownProps`) object inside of it, React will trigger a re-render of connected component even if actual prop value didn't change.
+
+
+## Rule details
+
+The following patterns are considered incorrect:
+
+```js
+const mapStateToProps = state => {
+  return {
+    foo: [1, 2, 3] // this array should be defined outside of mapStateToProps
+  };
+};
+```
+
+
+```js
+const mapStateToProps = state => {
+  return {
+    foo: {  // this object should be defined outside of mapStateToProps
+      a: 1
+    }
+  };
+};
+```
+
+
+The following patterns are correct
+
+```js
+const mapStateToProps = state => {
+  return {
+    a: 1
+  };
+};
+```
+
+```js
+const mapStateToProps = state => {
+  const a = state.a;
+  return {
+    a
+  };
+};
+```
+
+```js
+const mapStateToProps = state => ({
+  user: state.user,
+  // this is still a bad design because the list prop will be considered
+  // updated on every store change but the rule will not flag this.
+  list: [1, 2, state.count] 
+});
+```
+
+
+## Limitations
+
+Below case wouldn't be flagged by the rule:
+
+```js
+const mapStateToProps = state => {
+  const foo = [];
+  return {
+    foo
+  };
+};
+```

index.js

@@ -5,6 +5,7 @@ const rules = {
   'mapDispatchToProps-returns-object': require('./lib/rules/mapDispatchToProps-returns-object'),
   'mapDispatchToProps-prefer-parameters-names': require('./lib/rules/mapDispatchToProps-prefer-parameters-names'),
   'mapStateToProps-no-store': require('./lib/rules/mapStateToProps-no-store'),
+  'mapStateToProps-prefer-hoisted': require('./lib/rules/mapStateToProps-prefer-hoisted'),
   'mapStateToProps-prefer-parameters-names': require('./lib/rules/mapStateToProps-prefer-parameters-names'),
   'prefer-separate-component-file': require('./lib/rules/prefer-separate-component-file'),
 };
@@ -31,6 +32,7 @@ module.exports = {
         'react-redux/mapDispatchToProps-prefer-shorthand': 2,
         'react-redux/mapDispatchToProps-returns-object': 2,
         'react-redux/mapStateToProps-no-store': 2,
+        'react-redux/mapStateToProps-prefer-hoisted': 2,
         'react-redux/mapStateToProps-prefer-parameters-names': 2,
         'react-redux/prefer-separate-component-file': 1,
       },

lib/rules/mapStateToProps-prefer-hoisted.js

@@ -0,0 +1,64 @@
+const utils = require('../utils');
+const isReactReduxConnect = require('../isReactReduxConnect');
+
+const report = function (context, node) {
+  context.report({
+    message: 'constant arrays and objects should be initialized outside of mapStateToProps',
+    node,
+  });
+};
+
+const isConstArrayOrObj = (node, nested) => {
+  if (node && node.type === 'ObjectExpression') {
+    return node.properties.reduce((acc, prop) =>
+      (acc && isConstArrayOrObj(prop.value, (nested + 1))), true);
+  }
+  if (node && node.type === 'ArrayExpression') {
+    return node.elements.reduce((acc, el) =>
+      (acc && isConstArrayOrObj(el, (nested + 1))), true);
+  }
+  if (node && node.type === 'Literal' && nested > 0) {
+    return true;
+  }
+  return false;
+};
+
+const checkProp = (node, context) => {
+  if (isConstArrayOrObj(node, 0)) {
+    report(context, node);
+  }
+};
+
+
+const checkFunction = function (context, body) {
+  const returnNode = utils.getReturnNode(body);
+  if (returnNode && returnNode.type === 'ObjectExpression') {
+    returnNode.properties.forEach(prop => checkProp(prop.value, context));
+  }
+};
+
+module.exports = function (context) {
+  return {
+    VariableDeclaration(node) {
+      node.declarations.forEach((decl) => {
+        if (decl.id && decl.id.name === 'mapStateToProps') {
+          const body = decl.init.body;
+          checkFunction(context, body);
+        }
+      });
+    },
+    FunctionDeclaration(node) {
+      if (node.id && node.id.name === 'mapStateToProps') {
+        checkFunction(context, node.body);
+      }
+    },
+    CallExpression(node) {
+      if (isReactReduxConnect(node)) {
+        const mapStateToProps = node.arguments && node.arguments[0];
+        if (mapStateToProps && mapStateToProps.body) {
+          checkFunction(context, mapStateToProps.body);
+        }
+      }
+    },
+  };
+};

tests/lib/rules/mapStateToProps-prefer-hoisted.js

@@ -0,0 +1,187 @@
+require('babel-eslint');
+
+const rule = require('../../../lib/rules/mapStateToProps-prefer-hoisted');
+const RuleTester = require('eslint').RuleTester;
+
+const parserOptions = {
+  ecmaVersion: 6,
+  sourceType: 'module',
+  ecmaFeatures: {
+    experimentalObjectRestSpread: true,
+  },
+};
+
+const errorMessage = 'constant arrays and objects should be initialized outside of mapStateToProps';
+
+const ruleTester = new RuleTester({ parserOptions });
+
+ruleTester.run('mapStateToProps-prefer-hoisted', rule, {
+  valid: [
+    `function mapStateToProps(state) {
+      return {};
+    }`,
+    `const mapStateToProps = state => {
+      return {
+        a : 1
+      };
+    };`,
+    `const mapStateToProps = state => {
+      const a = state.a
+      return {
+        a
+      };
+    };`,
+    `const mapStateToProps = state => ({
+      user: state.user,
+      list: [1, 2, state.count]
+    });
+    `,
+    `const mapStateToProps = state => {
+      return {
+        a: 1,
+        b: [state.b, 2]
+      };
+    };
+    `,
+    `const mapStateToProps = state => {
+      const foo = 'hello';
+      return {
+        a: 1,
+        b: [foo, 2]
+      };
+    };
+    `,
+    'export default connect(null, null)(Alert)',
+    'connect((state) => ({isActive: state.isActive}), null)(App)',
+    'connect(null, null)(App)',
+    `connect(
+          (state) => {
+              return {
+                  isActive: state.isActive
+              }
+          },
+          null
+        )(App)
+    `,
+    `connect(function(state){
+              return {
+                  isActive: state.isActive
+              }
+          },
+          null
+        )(App)
+    `,
+    `const mapStateToProps = function(state) {
+      return {
+        a: x
+      };
+    }`,
+    'const mapStateToProps = (state, ownProps) => {}',
+    'const mapStateToProps = (state) => {set: [1, 2, 3, state.a]}',
+    `const mapStateToProps = (state, ownProps) => {};
+      connect(mapStateToProps, null)(Alert);`,
+    `const mapStateToProps = ({ header }) => ({
+      isLoggedIn: header.user && header.user.isLoggedIn,
+    }); `,
+    'const mapStateToProps = ({header}, ownProps) => {header};',
+    'connect(({header}, ownProps) => {header})(App);',
+    'connect(({header}, {ownProp1}) => {header, ownProp1})(App);',
+    `const mapStateToProps = ({header}, ownProps) => {
+      return {
+        props: {
+          header,
+        }
+      }
+    };`,
+  ],
+  invalid: [{
+    code: `const mapStateToProps = (state) => {
+      return {
+        foo: {
+          a: 1
+        }
+      }
+    }`,
+    errors: [
+      {
+        message: errorMessage,
+      },
+    ],
+  }, {
+    code: `const mapStateToProps = state => {
+            return {
+              foo: [1, 2, 3]
+            }
+          }`,
+    errors: [
+      {
+        message: errorMessage,
+      },
+    ],
+  }, {
+    code: `function mapStateToProps(state) {
+      return {
+        a: []
+      };
+    }`,
+    errors: [
+      {
+        message: errorMessage,
+      },
+    ],
+  }, {
+    code: `export default connect(
+        (state) => {
+            return {
+                a: {
+                  z: 1
+                }
+            }
+        },
+        (dispatch) => {
+            return {
+                actions: bindActionCreators(actions, dispatch)
+            }
+        }
+    )(App)`,
+    errors: [
+      {
+        message: errorMessage,
+      },
+    ],
+  }, {
+    code: `const mapStateToProps = state => {
+      return {
+        a: [1, 2, 3],
+      };
+    };
+    `,
+    errors: [
+      {
+        message: errorMessage,
+      },
+    ],
+  }, {
+    code: `function mapStateToProps(state) {
+      return {a : {}};
+    }`,
+    errors: [
+      {
+        message: errorMessage,
+      },
+    ],
+  }, {
+    code: `function mapStateToProps(state) {
+      return {
+        aProp: state.aProp,
+        aConstProp: [1, 2, 3]
+      };
+    }`,
+    errors: [
+      {
+        message: errorMessage,
+      },
+    ],
+  },
+  ],
+});