Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rule proposal: simple-computed-properties #254

Open
michalsnik opened this issue Nov 26, 2017 · 4 comments
Open

Rule proposal: simple-computed-properties #254

michalsnik opened this issue Nov 26, 2017 · 4 comments

Comments

@michalsnik
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member Author

michalsnik commented Jan 3, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants