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

no-new-mixins being flagged incorrectly #430

Open
amk221 opened this issue Jun 14, 2019 · 8 comments
Open

no-new-mixins being flagged incorrectly #430

amk221 opened this issue Jun 14, 2019 · 8 comments
Labels

Comments

@amk221
Copy link
Contributor

amk221 commented Jun 14, 2019

...I tried to write a failing test, but the above code passed :/

@bmish
Copy link
Member

bmish commented Jun 14, 2019

What's the file path?

@amk221
Copy link
Contributor Author

amk221 commented Jun 14, 2019

Ohh, is that how you're figuring it out? :(

tests/lib/common/unit/mixins/model/de-dupe.js

@bmish
Copy link
Member

bmish commented Jun 14, 2019

Yeah the detection is based on having mixin in the file path and extending an Ember object. Not great, but it also seems a bit confusing that your path has /model/ under the /mixins/ path.

@amk221
Copy link
Contributor Author

amk221 commented Jun 15, 2019

Ok.

I store my mixins in directories like so:

lib/mixins/model
lib/mixins/route
lib/mixins/controller
lib/mixins/component

because I have a lot, I group them by the type of thing they are mixed-in to.

...hence my need for this rule. I do not want any more new mixins :)

Do you have any suggestions for me other than renaming the directories (which I don't want to do).

@bmish
Copy link
Member

bmish commented Jun 15, 2019

You can disable the rule in .eslintrc.js for the entire tests directory. Would that help?

@amk221
Copy link
Contributor Author

amk221 commented Jun 16, 2019

I suppose I could do that for the time being 🙂 But I'd prefer to keep this issue open so that a better solution is found.

(It seems to be a common theme, see here: #235)

Cheers

@alexlafroscia
Copy link
Contributor

A number of the ESLint rules we have use the path on the filesystem to determine what type of class is being defined -- maybe that's something worth avoiding?

We could pretty easily determine if a new Mixin is being defined by tracking the imported identifier back to the @ember/object/mixin module -- maybe that's a change worth making?

@bmish
Copy link
Member

bmish commented Nov 6, 2019

I noticed that #213 would help fix this.

alexlafroscia added a commit to alexlafroscia/eslint-plugin-ember that referenced this issue Nov 21, 2019
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 added a commit to alexlafroscia/eslint-plugin-ember that referenced this issue Nov 21, 2019
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants