False positive in vue/no-side-effects-in-computed-properties

[doublemarked]

Tell us about your environment

Node v9.3.0
eslint-plugin-vue v

• ESLint Version: 4.18.0
• eslint-plugin-vue Version: 4.3.0
• Node Version: 9.3.0

Please show your full configuration:

module.exports = {
  root: true,
  env: {
    browser: true,
    node: true
  },
  parserOptions: {
    parser: 'babel-eslint'
  },
  extends: [
    'plugin:vue/essential'
  ],
  plugins: [
    'vue'
  ],
}

What did you do? Please include the actual source code causing the issue.

computed: {
   categorized() {
      const categories = {}

      this.types.forEach(c => {
        categories[c.category] = categories[c.category] || []
        categories[c.category].push(c)
      })

      return categories
    },
},

What did you expect to happen?

This function takes an array of types, each of which has a category property, and returns an array of categories, each of which has an array of types within that category.

This function is appropriate as a computed function as it does not modify this.types.

What actually happened? Please include the actual, raw output from ESLint.

47:7 error Unexpected side effect in "categorized" computed property vue/no-side-effects-in-computed-properties
✖ 1 problem (1 error, 0 warnings)

The issue is with line 7 from the snippet: categories[c.category].push(c). The regex for no-side-effects-in-computed-properties catches ANY use of push, even if it's on unrelated arrays. It even catches push calls within comments, for instance:

   unacceptable() {
      return this.types.map(t => {
        // [].push('xxx')
        return t
   })

I'm not certain what an appropriate fix is for this. Perhaps it should try to verify that the array being modified is not within the scope of the computed function?

[michalsnik]

Since we check all CallExpressions, I think the most simple and not too strict solution would be to just strip content of any call expression arguments in chain and perform regex check on the reduced code.

[ankhzet]

Also triggers on copied arrays (ES6 spread operator):

computedProp() {
  const sortedOther = [...this.otherProp].sort(); // .sort() here considered to mutate otherProp
  ...
},

[csicky]

It should not warn if I loop over an array like this.something.forEach and take something from each item to build the computed value. forEach is not mutating.

[csicky]

I am not sure if I should open an new issue, or maybe I don't understand how it should work, but I think there is another false positive for this rule. I want to emit a computed value containing an array of objects and each object will have a method on it, which yes, mutates the local state. But the method is not executed in the computed evaluation, it is just another property on the object.

    dateRangeShortcuts() {
      return [
        {
          text: this.translations.txtPrevious7Days,
          onClick() {
            this.selectedRange = [new Date(Date.now() - 3600 * 1000 * 24 * 7), new Date()]
          }
        },
        {
          text: this.translations.txtPrevious30Days,
          onClick() {
            this.selectedRange = [new Date(Date.now() - 3600 * 1000 * 24 * 30), new Date()]
          }
        }
      ]
    }