Rule proposition: no-side-effects-in-computed-properties

[michalsnik]

This rule would check if there are no side effects inside computed properties.

So this example would throw an error:

export default {
  computed: {
    hello() {
      this.lastName = 'Dolor' // <- error "Unexpected side effect in computed property 'hello'"
      return `Hello ${this.firstName} ${this.lastName}`
    }
  },
  data() {
    return {
      firstName: 'Lorem',
      lastName: 'Ipsum'
    }
  }
}

What do you think @mysticatea @chrisvfritz ?

[chrisvfritz]

I like it!

[LinusBorg]

Would this rule also check for things like:

export default {
  computed: {
    hello() {
      this.sideEffect() // <- error "Unexpected side effect in computed property 'hello'"
      const formattedValue = this.format(value) // <- no error
      return `Hello ${this.firstName} ${this.lastName}`
    }
  },
methods: {
  sideEffect() {
    // some side effect.
  },
  format(value) {
    return // ...
  }
}

[michalsnik]

We could @LinusBorg, so the assumptions would be to throw an error when:

• this.xxx <=|+=|++|-=|-->
• this.xxx.yyy... <=|+=|++|-=|-->
• this.xxx.<push|pop|shift|unshift|reverse|splice|sort|copyWithin|fill>
• this.xxx.yyy...<push|pop|shift|unshift|reverse|splice|sort|copyWithin|fill>
• any of the above is present in method that is being called inside computed property

Am I missing something?

// Update
Damn we actually could simplify this and consider that even simple this.xxx() is a side effect. That makes it even simpler. But we still need to remember that it should be possible to use map or other non-mutable methods on properties.

[LinusBorg]

But we still need to remember that it should be possible to use map or other non-mutable methods on properties.

When using map etc., we would use the return value, right?

this.something.map(/* ... */) // doesn't make sense
const result = this.something.map(/* ... */) // makes sense

The same is true for method calls - if we don't do something with the return value, it's safe to assume that the method we call has a side-effect.

this.myMethod(value) // this must have a side-effect, otherwise there's no reason in calling it, right?
const result =  this.myMethod(value) // this *might* have a side-effect, but could just be a helper.

[michalsnik]

When using map etc., we would use the return value, right?

I would check all CallExpressions, in terms of non-mutable methods like map I don't have to care about assignments, as it would not proudce any side effect in any case. And in terms of mutable methods like splice we also don't need to check for any assignment because it will mutate the initial value anyway, so presence of any this.something.<any mutable method> should trigger error.

The same is true for method calls - if we don't do something with the return value, it's safe to assume that the method we call has a side-effect.

This case has more sense, I do however think that methods you want to call in computed properties should be pure, and perhaps using standalone functions instead of component's methods should be considered better practice? Not sure entirely though... E.g:

function add2(num) {
  return num + 2;
}

export default {
  data() {
    return {
      num: 10
    }
  },
  computed: {
    result() {
      return add2(this.num);
    }
  }
}

over this:

export default {
  data() {
    return {
      num: 10
    }
  },
  computed: {
    result() {
      return this.add2(this.num);
    }
  },
  methods: {
    add2(num) {
      return num + 2;
    }
  }
}

The best would be to actually check all methods bodies for any side effect presences and then if this method is called trigger error, but that complicates things. For example one method doesn't introduce side-effect but only calls other method which has side effect and we end up with nice complex tree of side-or-not-effects hard to properly detect.

[Akryum]

A pure function can call other pure functions and still be pure I think. 😸

[Akryum]

@michalsnik What about method that transform your data with other props?

export default {
  props: {
    repeat: {
      default: 1,
    },
  },
  data() {
    return {
      num: 10
    }
  },
  computed: {
    result() {
      return this.add(this.num);
    }
  },
  methods: {
    add(num) {
      for (let i = 0; i < this.repeat; i++) {
        num += i
      }
      return num
    }
  }
}

(Sorry for the boring example.)

[LinusBorg]

@Akryum that example is not a pure functions, but it doesn't have any side-effects either.

[michalsnik]

Hah, I don't think we can catch everything. I'll just start with the simplest implementation as the initial version of this rule and we'll see what's next then.

In your example @Akryum I'd opt for moving this function up, so that there is no method called in computed property that uses other props or data. But it's maybe the case for another rule 🤔 (Being explicit in computed properties seems like a good practice to me).

function add(num, repeatTimes) {
  for (let i = 0; i < repeatTimes; i++) {
    num += i
  }
  return num
}

export default {
  props: {
    repeat: {
      default: 1,
    },
  },
  data() {
    return {
      num: 10
    }
  },
  computed: {
    result() {
      return add(this.num, this.repeat);
    }
  },
  methods: {}
}

[michalsnik]

My PR is almost ready #69
I think we can stick with those simple checks at this moment. These will probably catch most cases. Overthinking can lead to problems hard to solve. @Akryum @LinusBorg

[michalsnik]

Okay, there is maybe one more thing @LinusBorg mentioned we can add to this implementation:

this.sideEffect() // <- error "Unexpected side effect in computed property 'hello'"

I'll consider this too in initial implementation.

[armano2]

It will be nice if we we have support for plugins provided by vuejs team

Vue-router

export default {
  computed: {
    foo () {
      this.$router.go(-1)
      this.$router.push({})
      this.$router.replace({})
      return true
    }
  }
}

Vuex

export default {
  computed: {
    foo () {
      this.$store.commit({}) // Mutations
      this.$store.dispatch('', {}) // Actions
      return true
    }
  }
}

[LinusBorg]

@armano2 The proposed rule should reject all of these examples.

[armano2]

@LinusBorg you are right i missed last message 🐰

[michalsnik]

Interesting idea @armano2, however I'd prefer to add this in a separate PR after initial rule is merged.

[armano2]

There is also one case with prototype function witch is not supported

return Array.prototype.unshift.apply(this.object, data) // i know this is invalid

return Object.assign(this.object, data);

[michalsnik]

Ready in v3.6.0 🚀 Let's leave such edge cases for further consideration, we need to be agile and constantly improve those rules starting with simple solutions. I'm going to move those ideas to separate issue and closing this one. Thanks for a great discussion here guys!