eslint-plugin-ember
eslint-plugin-ember copied to clipboard
ember-cli
New rule proposal - "no-unused-dependent-properties"
Proposed rule should check if there are any unused dependent properties in computed properties.
Correct:
test: computed('a', 'b', function() {
return get(this, 'a') + get(this, 'b');
}),
Incorrect:
test: computed('a', 'b', function() {
return get(this, 'a') + 'asd';
}),
Hey, I'd love to help out with this plugin somehow. Want me to try putting together an implementation for this this weekend?
Sure, that would be really cool @alexlafroscia Great to see your interest in this! :)
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
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?
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.
Ah, ok. It's a good idea to change it for all rules. I'll create separate issue for that :)
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.
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.
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.getthis.getPropertiesget(x, y)orgetProperties(...) - 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.
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.
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 😐
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.
Any progress on this? I wouldn't mind working on this as it would save me some time
This could either be created as a new rule or as an option on the related rule require-computed-property-dependencies.
