require-tagless-components rule fails for Service.extend
#601
Comments
|
Can you include the error it fails with? CC: @alexlafroscia |
bmish
changed the title
require-tagless-components rule fails for Service.extend
|
Seems similar to #235, and based on an initial look, looks like the problem is here: https://github.com/ember-cli/eslint-plugin-ember/blob/master/lib/utils/ember.js#L171-L192 |
|
@bmish do you mean the lint failure error message? (I'm not getting an exception thrown, just a false positive on the lint rule) |
|
Ah, I suppose we missed test cases for Ember objects that are not components! I can take a look at fixing this tonight. Feel free to assign this bug to me! |
Adding tests to validate ember-cli#601
|
I added a couple of test cases that look like your example @mehulkar -- however, the tests don't actually report the false positive that you're seeing. Are you running the latest version of |
|
Try specifying a component filename in the unit test. |
|
Oooh yup, that's what it was! Configuring that causes a failure. It really feels like getting away from file names as part of the lint rules would be good, and instead just looking at the actual source of the import 😅 I'll keep following the thread here and figure out what's up! |
|
Yeah, definitely agree that we should be checking imports instead of file paths, tracking that improvement with #590. |
|
Would you be OK with a PR that fixed that? It would be pretty extensive, but I think that's really the right fix in this case. |
|
Yeah, since this lint rule just relies on |
|
Sweet! I'm going to take a stab at this, I have some time I can invest in it over the next few days |
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
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
|
@mehulkar @alexlafroscia is this still an issue or has it been fixed by #603? |
|
That PR was never merged, and at this point I can't see myself coming back to it unfortunately. I just don't have much OSS time these days. |
|
oh, lol, I totally missed that the PR was still open 😅 |
|
I see false positives as well when lodash |
In
tests/integration/components/foo-test.js, if I have:the lint rule fails
The text was updated successfully, but these errors were encountered: