require-computed-property-dependencies mustn't handle assignment

[boris-petrov]

After updating eslint-plugin-ember to 8.3.0, I got a require-computed-property-dependencies error on similar code to this:

class extends Component {
  private fooIndex = -1;

  @computed('bar')
  public get foo(): any {
    this.fooIndex = 5;
    return this.bar;
  }
}

On the this.fooIndex = 5; line. This is wrong as that property must not be put in the computed's dependencies as it leads to an assertion failure:

Error: Assertion Failed: You attempted to update <frontend@component:something::ember1529>.fooIndex to "5", but it is being tracked by a tracking context, such as a template, computed property, or observer. In order to make sure the context updates properly, you must invalidate the property when updating it. You can mark the property as `@tracked`, or use `@ember/object#set` to do this.

[bmish]

Hey there!

It looks like you're seeing a new violation reported because decorator support was added to some of our rules recently (specifically require-computed-property-dependencies in your example). Note that the lack of decorator support in many of our rules is a well-documented problem and it has been one of my top priorities to address it so that we can fully support modern Ember Octane syntax.

In your situation, can I ask why a property is being modified inside the getter function of a computed property? Mutating state in computed properties is considered poor practice, and we actually have another rule no-side-effects that specifically disallows that, although it won't flag your code currently because it hasn't been updated to support native classes and decorators yet (hopefully it will be updated soon). So if you can remove the tracked property mutation from inside the computed property, that should solve the problem.

Hope that helps!

[boris-petrov]

@bmish - thanks for the support.

I do know that modifying a property is frowned upon, but I nonetheless think this here is a bug. In our case that is something like a caching property so it's not that bad. 😄

It's great that you're bringing decorator support for these checks - most of the code will be using decorators in the future (perhaps that is already true for many projects)! Thanks!

[bmish]

Interesting! Thanks for explaining your use case. I have seen developers try to do things like caching in computed properties a few times, although I'm pretty sure all of the examples I have seen should have been refactored to avoid computed property side effects like that (perhaps using actions or other functions to perform the caching instead).

Now it is possible that the require-computed-property-dependencies rule could be updated to ignore assignments when determining what the dependent keys should be. Even though I do not want to encourage these assignments, it would still be good for this lint rule to ignore them. I'll see if I can make this change.

[bmish]

Opened a fix in #786.