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

[michalsnik]

This PR implements rule proposed in #58

This rule checks for existance of any of the following inside all computed properties:

• AssignmentExpression that starts with this. (e.g. this.x = 'asd', this.x += 1)
• UpdateExpression that starts with this. (e.g this.x++)
• CallExpression that starts with this. and calls one of the mutable methods (push, pop, shift, unshift, reverse, splice, sort, copyWithin, fill) not followed by any of those that remove the reference (like: slice, map, filter, concat)

Feel free to comment if I missed anything. At this point I'd like to stick with those simple checks and then we can investigate it further if we can check anything more than that. If believe this is a good starting point.

Left to do:

• Add one more case - simple CallExpression this.anything( that is not assigned anywhere should be considered unexpected side effect
• Add more test scenarios
• Clean current solution (I'm considering using regex for checking AssignmentExpression and UpdateExpression too, instead of parsing those simple chunks of AST)

[armano2]

in case when there is only

computed: {
  foo: {
    set () {}
  }
}

returned value is going to be

return [
  {
    key: '....',
    value: undefined
  }
]

And code when its used has to check for that case or we should filter it here

[michalsnik]

Good point, thanks @armano2

[armano2]

@michalsnik i think its better to check it here cus we are going propably to add rule about checking if computed propery has get and we can do that by just checking it .value is not undefined

[mysticatea]

We should be careful about nested functions if we use ThisExpression to lint.
thiss of nested functions often are bound to different instances.

[michalsnik]

Agree @mysticatea, but given the way vue works and the this bound to a component in default scope I think other thiss inside computed property or any other place in vue component should be considered harmful. Messing different contexts seems like a very bad idea to me and I think users should above else use parameters instead (e.g. when using forEach loop). I'd go with current implementation and see how it works in the wild. Please take a look once again and approve if you agree :)

[armano2]

coverage from unit test never enters this part ObjectExpression

[armano2]

unit test never enters this if

[armano2]

@michalsnik cp.key can be undefined if you are using experimentalObjectRestSpread
Solution: if cp.type === 'Property'

TypeError: Cannot read property 'name' of undefined

example:

export default {
  computed: {
    ...mapGetters({})
  }
}

[michalsnik]

Very good point, thank you @armano2 for pointing that!

[armano2]

unit test never enters here

[armano2]

all tests are for this.

[michalsnik]

Thanks for suggestions @armano2! I moved two helper functions to utils and added separate tests for them. Please take another look.

[armano2]

seems fine right now

[michalsnik]

Okay, let's push this forward. We're still in beta phase and it's not enabled by default so I think it's good to go.