eslint-plugin-react icon indicating copy to clipboard operation
eslint-plugin-react copied to clipboard

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

Open AndreaPontrandolfo opened this issue 2 years ago • 6 comments

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

AndreaPontrandolfo avatar Sep 26 '22 18:09 AndreaPontrandolfo

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.

AndreaPontrandolfo avatar Sep 26 '22 22:09 AndreaPontrandolfo

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 :-)

ljharb avatar Sep 26 '22 22:09 ljharb

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 avatar Sep 26 '22 22:09 AndreaPontrandolfo

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

AndreaPontrandolfo avatar Oct 11 '22 09:10 AndreaPontrandolfo

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

ljharb avatar Oct 11 '22 23:10 ljharb

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.

codecov[bot] avatar Nov 05 '22 23:11 codecov[bot]