vuejs / eslint-plugin-vue

Official ESLint plugin for Vue.js
https://eslint.vuejs.org/
MIT License
4.47k stars 664 forks source link

Rule proposal: `simple-computed-properties` #254

Open michalsnik opened 6 years ago

michalsnik commented 6 years ago

Style guide: https://vuejs.org/v2/style-guide/#Simple-computed-properties-strongly-recommended

Description: This rule would enforce creating simple computed properties with just a return statement, without too much logic around it.

Bad:

computed: {
  price: function () {
    var basePrice = this.manufactureCost / (1 - this.profitMargin)
    return (
      basePrice -
      basePrice * (this.discountPercent || 0)
    )
  }
}

Good:

computed: {
  basePrice: function () {
    return this.manufactureCost / (1 - this.profitMargin)
  },
  discount: function () {
    return this.basePrice * (this.discountPercent || 0)
  },
  finalPrice: function () {
    return this.basePrice - this.discount
  }
}
chrisvfritz commented 6 years ago

I think this might be one of those unenforceable rules actually, because in my experience, analyzing when it's appropriate to break out computed properties or not requires a lot of human judgment and knowledge of the context.

michalsnik commented 6 years ago

Yes and no :D I think we could treat this rule as max-lines per computed property and warn if there are more than 3 lines for example. I see a clear benefits of having such rule. We might ofc use other default. There is a similar rule for function body size in eslint core rules, but computed properties should be much smaller by definition.

chrisvfritz commented 6 years ago

Hmm, perhaps then. I'd probably want the default to be pretty generous though. If it's too sensitive, people are probably more likely to frustratedly disable it than to tweak it. And maybe counting the number of lines with variable assignments would be a good proxy for how well something can be broken out. If you can define something as a new variable, you can probably return it in a separate computed property.

For the default, I might even want to make it something quite high like 5 lines with variable assignments before we throw. And maybe we shouldn't count variable assignments with destructuring from a simple expression, as they're often more for convenience than saving a new value. Then as we use this in practice, we can experiment with lowering the threshold in individual apps and then maybe lower it with a major version bump.

michalsnik commented 6 years ago

I like the idea of checking not lines but actually assignments like you mentioned. This way we could tell user, that it's probably possible to simplify this computed property, because it most likely would be :D We can set default threshold for 2 assignments in first iteration and wait for the next release to decide final threshold.

Checking lines might not be too relevant, as often we do something like:

const {
  a,
  b,
  c,
  d
} = this.something
return (a + b) * (c - d)

This is 7 lines, but it's not complicated at all.