Closed Bug 1218409 Opened 4 years ago Closed 4 years ago

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

Categories

(DevTools :: General, defect)

defect
Not set

Tracking

(firefox44 fixed)

RESOLVED FIXED
Firefox 44
Tracking Status
firefox44 --- fixed

People

(Reporter: pbro, Assigned: pbro)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
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).
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
Attachment #8678877 - Flags: review?(mratcliffe)
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 : "";
Attachment #8678877 - Flags: review?(mratcliffe) → review+
Thanks Mike.
This new patch also checks the useCapture flag, and cleans up the warning string (uses {{}} template variables instead of concatenating strings).
Attachment #8678877 - Attachment is obsolete: true
Attachment #8679386 - Flags: review+
Keywords: checkin-needed
Blocks: 1218425
https://hg.mozilla.org/mozilla-central/rev/604e693761bd
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.