search

ember-cli / eslint-plugin-ember

An ESLint plugin that provides set of rules for Ember applications based on commonly known good practices.
MIT License
257 stars 198 forks source link

`require-tagless-components` rule fails for `Service.extend` #601

Closed mehulkar closed 3 years ago

mehulkar commented 4 years ago

In tests/integration/components/foo-test.js, if I have:

import Service from '@ember/service';
Service.extend({});

the lint rule fails

bmish commented 4 years ago

Can you include the error it fails with?

CC: @alexlafroscia

mehulkar commented 4 years ago

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

mehulkar commented 4 years ago

@bmish do you mean the lint failure error message? (I'm not getting an exception thrown, just a false positive on the lint rule)

113:21  error  Please switch to a tagless component by setting `tagName: ''` or converting to a Glimmer component  ember/require-tagless-components
alexlafroscia commented 4 years ago

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!

alexlafroscia commented 4 years ago

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 eslint-plugin-ember?

bmish commented 4 years ago

Try specifying a component filename in the unit test.

alexlafroscia commented 4 years ago

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!

bmish commented 4 years ago

Yeah, definitely agree that we should be checking imports instead of file paths, tracking that improvement with #590.

alexlafroscia commented 4 years ago

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.

bmish commented 4 years ago

Yeah, since this lint rule just relies on emberUtils.isEmberComponent(), I agree that the right fix would be to check imports instead of filepaths inside that util function. That would affect a lot of rules, but I would support pursuing that fix.

alexlafroscia commented 4 years ago

Sweet! I'm going to take a stab at this, I have some time I can invest in it over the next few days

Turbo87 commented 3 years ago

@mehulkar @alexlafroscia is this still an issue or has it been fixed by #603?

alexlafroscia commented 3 years ago

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.

Turbo87 commented 3 years ago

oh, lol, I totally missed that the PR was still open 😅

ballPointPenguin commented 3 years ago

I see false positives as well when lodash extend is in use within a component, eg

_.extend(notAComponentOrClass(), { foo: 'bar' })