-
-
Notifications
You must be signed in to change notification settings - Fork 385
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
Add prefer-event-target
rule
#1792
Add prefer-event-target
rule
#1792
Conversation
rules/prefer-event-target.js
Outdated
const eventEmitterClassSelector = [ | ||
':matches(ClassDeclaration, ClassExpression)', | ||
'[superClass.name="EventEmitter"]', | ||
'[body.type="ClassBody"]', |
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.
You added this body type check, but not tests covers this, is it possible to have different body here? Maybe in TypeScript? Please test it.
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.
You added this body type check, but not tests covers this, is it possible to have different body here? Maybe in TypeScript? Please test it.
I'm sorry, but I think I actually don't understand this feedback.
Is it possible to handle typescript with this lib?
I tried to insert a simple typescript statement (let a: number = 3;
) and tested, but it seems like parsing error occurs.
I think class declaration's with other body type seems not possible, according to this ECMA document in Javascript,
In case of Typescript, this specification's 117 page
And FYI, when I added this selector, I referenced to /rules/no-static-only-class.js
's selector.
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 don't know if it's possible to have different body in TS, I guess it's fine to keep this just in case.
If you find anything want test, you can use
test.typescript();
// OR
test.snapshot({
valid: [
{
code: '',
parser: parsers.typescript,
}
],
});
There are examples in other rules.
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.
Two comments, otherwise looks good to me. Thanks!
@jopemachine Can you fix the snapshot? |
Woho! thank you for this! 🎉 |
Thanks for this awesome lib!
Added
prefer-event-target
rule, Fixes #1740.