-
-
Notifications
You must be signed in to change notification settings - Fork 364
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 no-invalid-remove-event-listener
rule
#1216
Conversation
@bschlenk Friendly bump :) |
const MESSAGE_ID = 'no-invalid-remove-event-listener'; | ||
const messages = { | ||
[MESSAGE_ID]: | ||
'Store the result of this expression in a variable that is passed to both `removeEventListener` the corresponding `addEventListener`.' |
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.
How about The listener agument should be a function reference.
?
This comment has been minimized.
This comment has been minimized.
no-invalid-event-listener
ruleno-invalid-remove-event-listener
rule
I commonly use |
Looks good, I'll make some adjustment. |
@bschlenk I can't push to your fork, please merge this https://github.com/bschlenk/eslint-plugin-unicorn/pull/1. Next time try not to use the main branch. |
Can you improve the report location a little bit, if it's a function, use |
const isBindCall = node => | ||
node.type === 'CallExpression' && | ||
node.callee.type === 'MemberExpression' && | ||
node.callee.property.name === 'bind' && | ||
!node.callee.computed; |
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.
This function isn't used anymore. Do the below matchers handle when the callee is computed?
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.
Yes, remove it, all tested.
I didn't realize GitHub let you do that. When can you push to someone else's fork? |
Yes, people do this all the time. There is checkbox "allow maintainer edit"(something like that) on right side of this page, it should be checked. I'm not sure if it because you used default branch. |
That's cool! It does say you should be able to push to the main branch
|
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.
Looks great, thanks!
Need fix style |
@bschlenk Can you rebase this and run |
Running |
You probably need to rebase from the main branch first. |
I forgot to npm install, I had an outdated xo 🤦♂️ |
@bschlenk Can you ensure the tests are passing? |
readme.md
Outdated
@@ -167,6 +168,7 @@ Each rule has emojis denoting: | |||
| [no-for-loop](docs/rules/no-for-loop.md) | Do not use a `for` loop that can be replaced with a `for-of` loop. | ✅ | 🔧 | | | |||
| [no-hex-escape](docs/rules/no-hex-escape.md) | Enforce the use of Unicode escapes instead of hexadecimal escapes. | ✅ | 🔧 | | | |||
| [no-instanceof-array](docs/rules/no-instanceof-array.md) | Require `Array.isArray()` instead of `instanceof Array`. | ✅ | 🔧 | | | |||
| [no-invalid-remove-event-listener](docs/rules/no-invalid-remove-event-listener.md) | Prevent calling removeEventListener with the result of an expression | ✅ | | | |
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.
The description here needs to be regenerated.
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.
Is this what you mean? Make this description match the one from docs/rules/no-invalid-remove-event-listener.md
?
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.
Run npm run generate-rules-table
and npm run generate-usage-example
Thanks for helping, it's hard to keep up with all the changes in this repo. |
@sindresorhus I think this rule is ready for review, the code part looks good to me. |
Looks good :) |
Closes #682