-
-
Notifications
You must be signed in to change notification settings - Fork 204
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
refactor: avoid using file name to determine class identity. #603
base: master
Are you sure you want to change the base?
refactor: avoid using file name to determine class identity. #603
Conversation
The original logic for determining if a Identifier was of a specific Ember class used the file name to make this decision, rather than resolving the import of the Identifier back to the Ember module it was imported from. This ended up causing a number of issues in lint rule in cases where people places a class in a location that was unexpected based on Ember’s convention. Such issues include ember-cli#601 and ember-cli#430. Making this change avoids the need for something like ember-cli#213, which adds additional logic based on file names to prevent some of these issues. Closes ember-cli#590
Adding tests to validate ember-cli#601
12a3cd8
to
ba714c5
Compare
The commit |
We should move existing import utils to |
Yeah, I'm fighting to get the code coverage back to 99% -- it's currently at 98.87 🙄 let me take another stab at that I'd be happy to do some more work to organize things and clean up, but this PR is already pretty huge! |
@@ -29,7 +29,9 @@ module.exports = { | |||
}; | |||
|
|||
return { | |||
CallExpression(node) { | |||
'CallExpression > MemberExpression[property.name="extend"]'(memberExpressionNode) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not push this complexity (checking for extends and parents and such) into each rule. Each rule should be able to make one simple util function call to check if the node is an Ember controller/component/etc, like this:
CallExpression(node) {
// classic class
if (ember.isEmberController(context, node)) {
...
}
},
ClassDeclaration(node) {
// native class
if (ember.isEmberController(context, node)) {
...
}
},
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I agree -- it seems like we have a specific CallExpression
pattern we're looking for, which looks like
Foo.extend()
We have the ability to tell ESLint those details -- why would we want to check every call expression if we don't have to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To my knowledge, there's no actual difference between using the more-specific selector vs. doing it ourselves: either eslint does the checks or we do the checks. In general, I would agree with you that it's nice to take advantage of using more-specific selectors, but I don't think that's appropriate here, because:
- Using both the selector matching and util function unnecessarily splits the logic for detecting components/controllers/etc into two separate places instead of consolidating the logic fully inside a single util function (and maybe this util function could even come from a third-party library someday).
- Using the selector matching duplicates part of the detection logic across countless rules, which increases the chance for typos and inconsistencies between rules.
- Encapsulating all of the logic in a util function makes it easy to unit test it in its entirety.
- The rule code is simpler when the detection details are delegated to the util function. We should optimize for simplicity and readability, and make it as easy to write rules as possible (which includes providing extremely simple util function APIs).
bb09420
to
ba714c5
Compare
code: ` | ||
import CustomController from '@ember/controller'; | ||
export default CustomController.extend({}); | ||
`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about export default Ember.Controller.extend(...)
?
Since a LOT of files changed here, to add the import to all of the test cases that now actually resolve identifiers rather than make an assumption based on its name, I would suggest looking at the commits one-at-a-time to review. The updated tests pass before and after the change to how
isEmberCoreModule
works. If this is just too much to review at once, I'm happy to break it up.Todo
Identifier
source back to an Ember module across file boundaries (see Add import validation on existing rules #590 (comment))isDSModel