Add no-side-effects-in-computed-properties rule (#69)

michalsnik · web-flow · commit 455e9b93fac9 · 2017-07-19T16:12:34.000+02:00
* Add no-side-effects-in-computed-properties rule

* Prepare example tests for no-side-effects-in-computed-properties rule,
Move component detection logic to utils

* Add getComputedProperties logic

* Finalize initial logic for no-side-effects-in-computed-properties rule

* Fix test

* Add more methods in regex

* Add no-side-effects-in-computed-properties rule

* Prepare example tests for no-side-effects-in-computed-properties rule,
Move component detection logic to utils

* Add getComputedProperties logic

* Add more test, move functions to utils

* Make it work with Node 4
diff --git a/docs/rules/no-side-effects-in-computed-properties.md b/docs/rules/no-side-effects-in-computed-properties.md
@@ -0,0 +1,41 @@
+# Do not introduce side effects in computed properties (no-side-effects-in-computed-properties)
+
+It is considered a very bad practice to introduce side effects inside computed properties. It makes the code not predictable and hard to understand.
+
+
+## Rule Details
+
+Examples of **incorrect** code for this rule:
+
+```js
+
+export default {
+  computed: {
+    fullName() {
+      this.firstName = 'lorem' // <- side effect
+      return `${this.firstName} ${this.lastName}`
+    },
+    somethingReversed() {
+      return this.something.reverse() // <- side effect
+    }
+  }
+}
+
+```
+
+Examples of **correct** code for this rule:
+
+```js
+
+export default {
+  computed: {
+    fullName() {
+      return `${this.firstName} ${this.lastName}`
+    },
+    somethingReversed() {
+      return this.something.slice(0).reverse()
+    }
+  }
+}
+
+```
diff --git a/lib/rules/no-side-effects-in-computed-properties.js b/lib/rules/no-side-effects-in-computed-properties.js
@@ -0,0 +1,71 @@
+/**
+ * @fileoverview Don't introduce side effects in computed properties
+ * @author Michał Sajnóg
+ */
+'use strict'
+
+const utils = require('../utils')
+
+function create (context) {
+  const forbiddenNodes = []
+
+  return Object.assign({},
+    {
+      // this.xxx <=|+=|-=>
+      'AssignmentExpression > MemberExpression' (node) {
+        if (utils.parseMemberExpression(node)[0] === 'this') {
+          forbiddenNodes.push(node)
+        }
+      },
+      // this.xxx <++|-->
+      'UpdateExpression > MemberExpression' (node) {
+        if (utils.parseMemberExpression(node)[0] === 'this') {
+          forbiddenNodes.push(node)
+        }
+      },
+      // this.xxx.func()
+      'CallExpression' (node) {
+        const code = context.getSourceCode().getText(node)
+        const MUTATION_REGEX = /(this.)((?!(concat|slice|map|filter)\().)*((push|pop|shift|unshift|reverse|splice|sort|copyWithin|fill)\()/g
+
+        if (MUTATION_REGEX.test(code)) {
+          forbiddenNodes.push(node)
+        }
+      }
+    },
+    utils.executeOnVueComponent(context, (obj) => {
+      const computedProperties = utils.getComputedProperties(obj)
+
+      computedProperties.forEach(cp => {
+        forbiddenNodes.forEach(node => {
+          if (
+            node.loc.start.line >= cp.value.loc.start.line &&
+            node.loc.end.line <= cp.value.loc.end.line
+          ) {
+            context.report({
+              node: node,
+              message: `Unexpected side effect in "${cp.key}" computed property.`
+            })
+          }
+        })
+      })
+    })
+  )
+}
+
+// ------------------------------------------------------------------------------
+// Rule Definition
+// ------------------------------------------------------------------------------
+
+module.exports = {
+  create,
+  meta: {
+    docs: {
+      description: 'Don\'t introduce side effects in computed properties',
+      category: 'Best Practices',
+      recommended: false
+    },
+    fixable: null,
+    schema: []
+  }
+}
diff --git a/lib/rules/order-in-components.js b/lib/rules/order-in-components.js
@@ -4,6 +4,8 @@
  */
 'use strict'
 
+const utils = require('../utils')
+
 const defaultOrder = [
   ['name', 'delimiters', 'functional', 'model'],
   ['components', 'directives', 'filters'],
@@ -36,38 +38,6 @@ const groups = {
   ]
 }
 
-function isComponentFile (node, path) {
-  const isVueFile = path.endsWith('.vue') || path.endsWith('.jsx')
-  return isVueFile && node.declaration.type === 'ObjectExpression'
-}
-
-function isVueComponent (node) {
-  const callee = node.callee
-
-  const isFullVueComponent = node.type === 'CallExpression' &&
-    callee.type === 'MemberExpression' &&
-    callee.object.type === 'Identifier' &&
-    callee.object.name === 'Vue' &&
-    callee.property.type === 'Identifier' &&
-    callee.property.name === 'component' &&
-    node.arguments.length &&
-    node.arguments.slice(-1)[0].type === 'ObjectExpression'
-
-  const isDestructedVueComponent = callee.type === 'Identifier' &&
-    callee.name === 'component'
-
-  return isFullVueComponent || isDestructedVueComponent
-}
-
-function isVueInstance (node) {
-  const callee = node.callee
-  return node.type === 'NewExpression' &&
-    callee.type === 'Identifier' &&
-    callee.name === 'Vue' &&
-    node.arguments.length &&
-    node.arguments[0].type === 'ObjectExpression'
-}
-
 function getOrderMap (order) {
   const orderMap = new Map()
 
@@ -106,28 +76,13 @@ function checkOrder (propertiesNodes, orderMap, context) {
 function create (context) {
   const options = context.options[0] || {}
   const order = options.order || defaultOrder
-  const filePath = context.getFilename()
 
   const extendedOrder = order.map(property => groups[property] || property)
   const orderMap = getOrderMap(extendedOrder)
 
-  return {
-    ExportDefaultDeclaration (node) {
-      // export default {} in .vue || .jsx
-      if (!isComponentFile(node, filePath)) return
-      checkOrder(node.declaration.properties, orderMap, context)
-    },
-    CallExpression (node) {
-      // Vue.component('xxx', {}) || component('xxx', {})
-      if (!isVueComponent(node)) return
-      checkOrder(node.arguments.slice(-1)[0].properties, orderMap, context)
-    },
-    NewExpression (node) {
-      // new Vue({})
-      if (!isVueInstance(node)) return
-      checkOrder(node.arguments[0].properties, orderMap, context)
-    }
-  }
+  return utils.executeOnVueComponent(context, (obj) => {
+    checkOrder(obj.properties, orderMap, context)
+  })
 }
 
 // ------------------------------------------------------------------------------
diff --git a/lib/utils/index.js b/lib/utils/index.js
@@ -241,5 +241,148 @@ module.exports = {
     assert(typeof name === 'string')
 
     return VOID_ELEMENT_NAMES.has(name.toLowerCase())
+  },
+
+  /**
+   * Parse member expression node to get array with all of it's parts
+   * @param {ASTNode} MemberExpression
+   * @returns {Array}
+   */
+  parseMemberExpression (node) {
+    const members = []
+    let memberExpression
+
+    if (node.type === 'MemberExpression') {
+      memberExpression = node
+
+      while (memberExpression.type === 'MemberExpression') {
+        if (memberExpression.property.type === 'Identifier') {
+          members.push(memberExpression.property.name)
+        }
+        memberExpression = memberExpression.object
+      }
+
+      if (memberExpression.type === 'ThisExpression') {
+        members.push('this')
+      } else if (memberExpression.type === 'Identifier') {
+        members.push(memberExpression.name)
+      }
+    }
+
+    return members.reverse()
+  },
+
+  /**
+   * Get all computed properties by looking at all component's properties
+   * @param {ObjectExpression} Object with component definition
+   * @return {Array} Array of computed properties in format: [{key: String, value: ASTNode}]
+   */
+  getComputedProperties (componentObject) {
+    const computedPropertiesNode = componentObject.properties
+      .filter(p =>
+        p.key.type === 'Identifier' &&
+        p.key.name === 'computed' &&
+        p.value.type === 'ObjectExpression'
+      )[0]
+
+    if (!computedPropertiesNode) { return [] }
+
+    return computedPropertiesNode.value.properties
+      .filter(cp => cp.type === 'Property')
+      .map(cp => {
+        const key = cp.key.name
+        let value
+
+        if (cp.value.type === 'FunctionExpression') {
+          value = cp.value.body
+        } else if (cp.value.type === 'ObjectExpression') {
+          value = cp.value.properties
+            .filter(p =>
+              p.key.type === 'Identifier' &&
+              p.key.name === 'get' &&
+              p.value.type === 'FunctionExpression'
+            )
+            .map(p => p.value.body)[0]
+        }
+
+        return { key, value }
+      })
+  },
+
+    /**
+     * Check whether the given node is a Vue component based
+     * on the filename and default export type
+     * export default {} in .vue || .jsx
+     * @param {ASTNode} node Node to check
+     * @param {string} path File name with extension
+     * @returns {boolean}
+     */
+  isVueComponentFile (node, path) {
+    const isVueFile = path.endsWith('.vue') || path.endsWith('.jsx')
+    return isVueFile &&
+      node.type === 'ExportDefaultDeclaration' &&
+      node.declaration.type === 'ObjectExpression'
+  },
+
+    /**
+     * Check whether given node is Vue component
+     * Vue.component('xxx', {}) || component('xxx', {})
+     * @param {ASTNode} node Node to check
+     * @returns {boolean}
+     */
+  isVueComponent (node) {
+    const callee = node.callee
+
+    const isFullVueComponent = node.type === 'CallExpression' &&
+      callee.type === 'MemberExpression' &&
+      callee.object.type === 'Identifier' &&
+      callee.object.name === 'Vue' &&
+      callee.property.type === 'Identifier' &&
+      callee.property.name === 'component' &&
+      node.arguments.length &&
+      node.arguments.slice(-1)[0].type === 'ObjectExpression'
+
+    const isDestructedVueComponent = callee.type === 'Identifier' &&
+      callee.name === 'component'
+
+    return isFullVueComponent || isDestructedVueComponent
+  },
+
+    /**
+     * Check whether given node is new Vue instance
+     * new Vue({})
+     * @param {ASTNode} node Node to check
+     * @returns {boolean}
+     */
+  isVueInstance (node) {
+    const callee = node.callee
+    return node.type === 'NewExpression' &&
+      callee.type === 'Identifier' &&
+      callee.name === 'Vue' &&
+      node.arguments.length &&
+      node.arguments[0].type === 'ObjectExpression'
+  },
+
+  executeOnVueComponent (context, cb) {
+    const filePath = context.getFilename()
+    const _this = this
+
+    return {
+      'ExportDefaultDeclaration:exit' (node) {
+        // export default {} in .vue || .jsx
+        if (!_this.isVueComponentFile(node, filePath)) return
+        cb(node.declaration)
+      },
+      'CallExpression:exit' (node) {
+        // Vue.component('xxx', {}) || component('xxx', {})
+        if (!_this.isVueComponent(node)) return
+        cb(node.arguments.slice(-1)[0])
+      },
+      'NewExpression:exit' (node) {
+        // new Vue({})
+        if (!_this.isVueInstance(node)) return
+        cb(node.arguments[0])
+      }
+    }
   }
 }
diff --git a/package.json b/package.json
@@ -49,6 +49,8 @@
   },
   "devDependencies": {
     "@types/node": "^4.2.6",
+    "babel-eslint": "^7.2.3",
+    "chai": "^4.1.0",
     "eslint": "^3.18.0",
     "eslint-plugin-eslint-plugin": "^0.7.1",
     "eslint-plugin-vue-libs": "^1.2.0",
diff --git a/tests/lib/rules/no-side-effects-in-computed-properties.js b/tests/lib/rules/no-side-effects-in-computed-properties.js
diff --git a/tests/lib/utils/index.js b/tests/lib/utils/index.js
