New rule proposal - "no-unused-dependent-properties" about eslint-plugin-ember HOT 18 OPEN

Hey, I'd love to help out with this plugin somehow. Want me to try putting together an implementation for this this weekend?

from eslint-plugin-ember.

Sure, that would be really cool @alexlafroscia Great to see your interest in this! :)

from eslint-plugin-ember.

Hmm, does this plugin use the regular ESLint plugin setup or no?

from eslint-plugin-ember.

This is probably gunna take me a little time, since there's a number of edge cases around situations like

{
  foo: computed('array.@each', function() {
    ...
  })
}

and

{
  foo: computed('object.{key,otherKey}', function() {
    ...
  })
}

I'm not going to make a PR just yet, but my progress can be tracked here:

https://github.com/alexlafroscia/eslint-plugin-ember/tree/no-unused-dependent-properties

from eslint-plugin-ember.

Yeah, there are quite a few edge cases. Take your time, and if you'll need me to continue working on this that's fine too.

I believe it's quite regular ESLint plugin setup, is something wrong?

from eslint-plugin-ember.

I use the Yeoman generator to generate a new rule, and it just put the files in a different location. I realized after the question that I just needed to relocate them, not a big deal.

from eslint-plugin-ember.

Ah, ok. It's a good idea to change it for all rules. I'll create separate issue for that :)

from eslint-plugin-ember.

^ #5

from eslint-plugin-ember.

Excellent!

Making good progress. I have a bunch of tests written and a pretty naive implementation that makes them pass. Want to add more test cases tonight and then submit a PR; it might be good enough to start, or we could discuss an alternate approach together.

The main issue that I'm seeing is that it's hard to tell if you're actually accessing the property on the controller/component rather than some other object that happens to use the same keys. Just looking for this is a good step but not technically enough, but just assuming that if you're calling get with the given key might be an OK approach, making the assumption the object is correct. It's something we could iterate on later.

from eslint-plugin-ember.

Right now I'm checking every property definition and looking down into it to see if it's a computed property that uses all the given keys. It might be possible to make a better implementation that starts with all times that get is used and walks upward in the AST to see if it's in a computed property, mark the dependent key is used; and then error on all unused ones later. Not really sure though, might have to try implementing that to find out.

from eslint-plugin-ember.

Great, I'm totally fine with basic approach first in order to iterate over it later with more and more complex scenarios.

Both approaches seems interesting, not sure which is the best though. Personally I was thinking about doing 3 things in this rule:

• parse dependent keys to get a nice array of them
• parse computed block in order to get all properties that were obtained either using this.get this.getProperties get(x, y) or getProperties(...)
• compare obtained properties with dependent keys and see which key is not used

As you pointed it might be damn tricky as both these examples should not cause any errors:

// ok
asd: computed('test.asd', function() {
  return get(this, 'test.asd') + 'asd';
}),

//ok
qwe: computed('test.qwe', function() {
  const t = get(this, 'test');
  return get(t, 'qwe') + 'qwe';
}),

I'd be happy to see your current implementation and then we can think about possible downsides and ideas for improvements.

Maybe as a starting point it would be good to just narrow this rule to simple dependent keys, without @each and nesting support.

from eslint-plugin-ember.

Ahh, I forgot about getProperties. I'll need to update my code to support that (shouldn't be hard).

parse dependent keys to get a nice array of them

this I do, as well as doing things like expanding out foo.{bar,baz} into foo.bar and foo.baz separately and dropping .@each from keys (so array.@each to array). The hard part is then taking that list and checking for it, because you're right; that second case (with qwe) becomes really hard to do in ESLint, considering it doesn't provide all that much information statically to enable tracking an object like that easily.

from eslint-plugin-ember.

Hey @alexlafroscia, any update here? Just checking out ✌️

from eslint-plugin-ember.

Hey sorry, I've been distracted from this by work stuff. I haven't made any progress past the branch I linked to earlier and I'm not sure when I'll have the bandwidth for this again 😐

from eslint-plugin-ember.

Sure, no problem @alexlafroscia But maybe you can just create a PR and set in progress label? it would be a good place to keep further discussion regarding this rule and implementation details.

from eslint-plugin-ember.

Yeah, sure. I'll make the PR but I don't think I can add labels.

from eslint-plugin-ember.

Any progress on this? I wouldn't mind working on this as it would save me some time

from eslint-plugin-ember.

This could either be created as a new rule or as an option on the related rule require-computed-property-dependencies.

from eslint-plugin-ember.