1218409 - ESLint rule that checks for balanced addEventListener/removeEventListener and on/off

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Patrick Brosset <:pbro>]

It's hard to check by hand when there are many, and I know we do have some added listeners that aren't removed. So at least I would find this rule useful.

We can probably start very simple by just making sure all addEventListener/on have a removeEventListener/off with the same event name without checking objects, callbacks, etc...
We could go further than this, but I believe a simple rule would show a warning/error too, and that's all we need to know something is wrong somewhere.

I'll attach a simple first patch and see what people think.

Comment 1

[Patrick Brosset <:pbro>]

Attachment: Bug_1218409_-_Eslint_rule_that_checks_for_balanced.diff

Mike, what do you think about this?
As the comment at the top of the rule file says, it's simple, but it did already find problems (in style-inspector.js for instance).

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8678877 [details] [diff] [review]
Bug_1218409_-_Eslint_rule_that_checks_for_balanced.diff

Review of attachment 8678877 [details] [diff] [review]:
-----------------------------------------------------------------

It would be great if we could add useCapture but I will leave that in your hands.

::: testing/eslint-plugin-mozilla/docs/index.rst
@@ +34,3 @@
>       "mozilla/components-imports": 1,
>       "mozilla/import-headjs-globals": 1,
>       "mozilla/mark-test-function-used": 1,

Nit: I would add this to this block:
"mozilla/balanced-listeners": 2,

::: testing/eslint-plugin-mozilla/lib/rules/balanced-listeners.js
@@ +3,5 @@
> + * addEventListener and an off for each on.
> + * Note that for now, this rule is rather simple in that it only checks that
> + * for each event name there is both an add and remove listener. It doesn't
> + * check that these are called on the right objects or with the same callback
> + * and useCapture arguments.

The function name should be enough but it would be great if we can include useCapture... I guess you know you can use:
var useCapture = node.arguments[2] ? node.arguments[2].value : "";

Comment 3

[Patrick Brosset <:pbro>]

Attachment: Bug_1218409_-_Eslint_rule_that_checks_for_balanced.diff

Thanks Mike.
This new patch also checks the useCapture flag, and cleans up the warning string (uses {{}} template variables instead of concatenating strings).

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/604e693761bd

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/604e693761bd