Skip to content
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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

alexlafroscia
Copy link
Contributor

@alexlafroscia alexlafroscia commented Nov 21, 2019

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

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
@alexlafroscia alexlafroscia force-pushed the fix-require-tagless-on-non-components branch from 12a3cd8 to ba714c5 Compare November 21, 2019 06:28
@bmish
Copy link
Member

bmish commented Nov 21, 2019

The commit refactor: remove other unused code doesn't look right, I think that code is still used.

@bmish
Copy link
Member

bmish commented Nov 21, 2019

We should move existing import utils to utils/import, likely in a separate PR. I'd like to have a complete and consistent suite of utils with clear APIs available for checking imports.

@alexlafroscia
Copy link
Contributor Author

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) {
Copy link
Member

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)) {
    ...
  }
},

Copy link
Contributor Author

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?

Copy link
Member

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:

  1. 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).
  2. Using the selector matching duplicates part of the detection logic across countless rules, which increases the chance for typos and inconsistencies between rules.
  3. Encapsulating all of the logic in a util function makes it easy to unit test it in its entirety.
  4. 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).

@alexlafroscia alexlafroscia force-pushed the fix-require-tagless-on-non-components branch from bb09420 to ba714c5 Compare November 21, 2019 16:42
code: `
import CustomController from '@ember/controller';
export default CustomController.extend({});
`,
Copy link
Member

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(...)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants