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

[NEW]: ensure-matching-remove-event-listener #3442

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

AndreaPontrandolfo
Copy link

Tentative to add a rule that enforce developers to cleanup eventListeners in the useEffect block.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused how why this needs a full lint rule - manually adding event listeners should be exceedingly rare, as the proper approach is generally to add them as jsx props. Obviously window is a special case, but generally one puts this inside a single component managed by domain experts, and everyone else just uses the component.

docs/rules/ensure-matching-remove-event-listener.md Outdated Show resolved Hide resolved
@AndreaPontrandolfo
Copy link
Author

Hi, appreciate the review!
I wrote this rule (and actually included this in my own Eslint plugin) because this mistake is pretty frequent at my company. I usually happen to work with people of all different skill grades and not everyone knows about every single nuance of React.
Eslint isn't only useful to standardize our codebases, it can also prevent mistakes like this and even help juniors learn something with minimal effort, like in this case.
I think this rule has a pretty solid use-case and could be useful to many people but i'm very eager to listen to other people opinion on this, coming from different angles.

@ljharb
Copy link
Member

ljharb commented Sep 26, 2022

That totally makes sense - but it seems like one way to solve that is to force usage of a sanctioned custom component and ban via eslint any usage of window.addEventListener or addEventListener outside of that component :-)

@AndreaPontrandolfo
Copy link
Author

That totally makes sense - but it seems like one way to solve that is to force usage of a sanctioned custom component and ban via eslint any usage of window.addEventListener or addEventListener outside of that component :-)

That's also an idea, but would be a vastly more complex rule than this, i think. This one is relatively simple and straightforward, and doesn't require any particular setup on part of the developers.

@AndreaPontrandolfo
Copy link
Author

Hi, @ljharb, i'm not really sure why the CI is failing. What should i do?

@ljharb
Copy link
Member

ljharb commented Oct 11, 2022

The documentation checker seems to be erroring; there's a generation script to run.

@codecov
Copy link

codecov bot commented Nov 5, 2022

Codecov Report

Merging #3442 (bd42395) into master (c6d082a) will increase coverage by 0.00%.
The diff coverage is 98.30%.

@@           Coverage Diff           @@
##           master    #3442   +/-   ##
=======================================
  Coverage   97.57%   97.58%           
=======================================
  Files         129      130    +1     
  Lines        9200     9259   +59     
  Branches     3336     3372   +36     
=======================================
+ Hits         8977     9035   +58     
- Misses        223      224    +1     
Impacted Files Coverage Δ
configs/all.js 100.00% <ø> (ø)
lib/rules/ensure-matching-remove-event-listener.js 98.30% <98.30%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@ljharb ljharb force-pushed the master branch 6 times, most recently from 59af733 to 865ed16 Compare November 11, 2022 02:45
@AndreaPontrandolfo AndreaPontrandolfo force-pushed the ensure-matching-remove-event-listener branch from 41a970a to 4115a19 Compare November 12, 2022 23:33
@AndreaPontrandolfo AndreaPontrandolfo marked this pull request as ready for review November 13, 2022 00:13

## When Not To Use It

You don't need this rules if you want to allow developers to not remove eventListeners added in the DOM.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about element.addEventListener, added with a ref?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you mean, can you expand?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean that addEventListener isn't just on window, it's also on every DOM element, and the same reasoning you've used for "window" would apply to element-based listeners as well.

lib/rules/ensure-matching-remove-event-listener.js Outdated Show resolved Hide resolved
lib/rules/ensure-matching-remove-event-listener.js Outdated Show resolved Hide resolved
@ljharb ljharb force-pushed the master branch 4 times, most recently from 069314a to 181c68f Compare November 18, 2022 17:19
author AndreaPontrandolfo <[email protected]> 1664217948 +0200
committer AndreaPontrandolfo <[email protected]> 1668295550 +0100

parent 865ed16
author AndreaPontrandolfo <[email protected]> 1664217948 +0200
committer AndreaPontrandolfo <[email protected]> 1668295511 +0100

parent 865ed16
author AndreaPontrandolfo <[email protected]> 1664217948 +0200
committer AndreaPontrandolfo <[email protected]> 1668295439 +0100

parent 865ed16
author AndreaPontrandolfo <[email protected]> 1664217948 +0200
committer AndreaPontrandolfo <[email protected]> 1668293705 +0100

parent 865ed16
author AndreaPontrandolfo <[email protected]> 1664217948 +0200
committer AndreaPontrandolfo <[email protected]> 1668293608 +0100

parent 865ed16
author AndreaPontrandolfo <[email protected]> 1664217948 +0200
committer AndreaPontrandolfo <[email protected]> 1668293412 +0100

feat: added rule ensure-matching-remove-event-listener

fix: fixed docs whitespaces

fix: removed a whitespace from the docs

fix: added a tab spacing to the docs file

fix: added a single traling newline to the docs

feat(rule): added ensure-remove-event-listener to config-all

docs(readme): updated docs

docs(docs): updated documentation for rule

updated documentation for ensure-matching-remove-event-listener rule

docs(readme): added newline in readme

docs(readme): updated readme

fix(test): fixed rule test

fix: removed a whitespace from the docs

fix: added a single traling newline to the docs

docs(readme): updated docs

docs(docs): updated documentation for rule

updated documentation for ensure-matching-remove-event-listener rule

docs(readme): added newline in readme

fix: removed a whitespace from the docs

fix: added a single traling newline to the docs

docs(readme): updated docs

docs(docs): updated documentation for rule

updated documentation for ensure-matching-remove-event-listener rule

docs(readme): added newline in readme

fix: fixed docs whitespaces

fix: removed a whitespace from the docs

fix: added a tab spacing to the docs file

fix: added a single traling newline to the docs

feat(rule): added ensure-remove-event-listener to config-all

docs(readme): updated docs

docs(docs): updated documentation for rule

updated documentation for ensure-matching-remove-event-listener rule

docs(readme): added newline in readme

docs(readme): updated readme

fix(test): fixed rule test

fix: removed a whitespace from the docs

fix: added a single traling newline to the docs

docs(readme): updated docs

docs(docs): updated documentation for rule

updated documentation for ensure-matching-remove-event-listener rule

docs(readme): added newline in readme

fix: fixed docs whitespaces

fix: removed a whitespace from the docs

fix: added a tab spacing to the docs file

fix: added a single traling newline to the docs

feat(rule): added ensure-remove-event-listener to config-all

docs(readme): updated docs

docs(docs): updated documentation for rule

updated documentation for ensure-matching-remove-event-listener rule

docs(readme): added newline in readme

docs(readme): updated readme

fix(test): fixed rule test

fix: fixed docs whitespaces

fix: removed a whitespace from the docs

fix: added a tab spacing to the docs file

fix: added a single traling newline to the docs

feat(rule): added ensure-remove-event-listener to config-all

docs(readme): updated docs

docs(docs): updated documentation for rule

updated documentation for ensure-matching-remove-event-listener rule

docs(readme): added newline in readme

docs(readme): updated readme

fix(test): fixed rule test

fix: removed a whitespace from the docs

fix: added a single traling newline to the docs

docs(readme): updated docs

docs(docs): updated documentation for rule

updated documentation for ensure-matching-remove-event-listener rule

docs(readme): added newline in readme

fix: removed a whitespace from the docs

fix: added a single traling newline to the docs

docs(readme): updated docs

docs(docs): updated documentation for rule

updated documentation for ensure-matching-remove-event-listener rule

docs(readme): added newline in readme

fix: fixed docs whitespaces

fix: removed a whitespace from the docs

fix: added a tab spacing to the docs file

fix: added a single traling newline to the docs

feat(rule): added ensure-remove-event-listener to config-all

docs(readme): updated docs

docs(docs): updated documentation for rule

updated documentation for ensure-matching-remove-event-listener rule

docs(readme): added newline in readme

docs(readme): updated readme

fix(test): fixed rule test

fix: removed a whitespace from the docs

fix: added a single traling newline to the docs

docs(readme): updated docs

docs(docs): updated documentation for rule

updated documentation for ensure-matching-remove-event-listener rule

docs(readme): added newline in readme

fix: fixed docs whitespaces

fix: removed a whitespace from the docs

fix: added a tab spacing to the docs file

fix: added a single traling newline to the docs

feat(rule): added ensure-remove-event-listener to config-all

docs(readme): updated docs

docs(docs): updated documentation for rule

updated documentation for ensure-matching-remove-event-listener rule

docs(readme): added newline in readme

docs(readme): updated readme

fix(test): fixed rule test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants