Closed Bug 1709997 Opened 4 years ago Closed 3 years ago

Add support for AbortController in EventEmitter

Categories

(DevTools :: Shared Components, task)

task

Tracking

(firefox92 fixed)

RESOLVED FIXED
92 Branch
Tracking Status
firefox92 --- fixed

People

(Reporter: nchevobbe, Assigned: nchevobbe)

Details

Attachments

(1 file)

AbortController allows to remove regular event listeners (see signal parameter in https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#parameters)
It's super useful to be able to remove multiple event listeners at once.
As DevTools code is using a mixture of regular event listeners and EventEmitter listeners, it would be great to bring such capability to EventEmitter as well so we'd have only one AbortController signal handle all our event listeners.

So let's say we have this use case:

const RandomDevToolsClass {
  constructor(window, obj) {
    this.window = window;
    this.obj = obj;

    this.window.addEventListener("load", this._onLoad)
    this.obj.on("refresh", this._onRefresh)
  }

  destroy() {
    this.window.removeEventListener("load", this._onLoad)
    this.obj.off("refresh", this._onRefresh)
  }
}

Here, we have to:

  • set window and obj on this, so we can remove the event listener later
  • have _onLoad and _onRefresh as methods (and not inline anonymous function), so we can remove the event listener later
  • in destroy go over all the event listeners we set to remove them

With AbortController, it could be transformed to

const RandomDevToolsClass {
  constructor(window, obj) {
    this.abortController = new AbortController();
    const {signal} = this.abortController;

    window.addEventListener("load", () => { /* … */}, {signal})
    obj.on("refresh",  () => { /* … */}, {signal})
  }

  destroy() {
    this.abortController.abort();
  }
}

of course, we'll still have cases where we need to set window and obj onto this, and other cases where we still want the event listener callbacks to not be inline anonymous functions, but at least with AbortController this is giving us the ability to choose.

I'm wondering if we could make our common base classes inherit from AbortSignal/AbortController, so that we don't have to create an AbortController, just to fetch it signal and avoid having to abort the abort controller...

So that we can do:

const RandomDevToolsClass extends AbortController {
  constructor(window, obj) {
    const signal = this.signal;

    window.addEventListener("load", () => { /* … */}, {signal})
    obj.on("refresh",  () => { /* … */}, {signal})
  }

  destroy() {
    // This will call AbortController.destroy()
    super.destroy()
  }
}

or

const RandomDevToolsClass extends AbortSignal {
  constructor(window, obj) {
    const signal = this;

    window.addEventListener("load", () => { /* … */}, {signal})
    obj.on("refresh",  () => { /* … */}, {signal})
  }

  destroy() {
    // Notify that we should unregister any async listener via AbortSignal interface
    this.dispatchEvent(new Event("abort"));
  }
}

or

const RandomDevToolsClass extends EventTarget {
  constructor(window, obj) {
    this.aborted = fase;
    const signal = this;

    window.addEventListener("load", () => { /* … */}, {signal})
    obj.on("refresh",  () => { /* … */}, {signal})
  }

  destroy() {
    // Implement AbortSignal interface
    this.aborted = true;
    this.dispatchEvent(new Event("abort"));
  }
}

As many classes inherit from EventEmitter, and, inheriting from many classes is complex... may be we should make EventEmitter expose an AbortSignal somehow.

Once we do something in this bug, we can probably revisit this comment and use a signal for this:
https://phabricator.services.mozilla.com/D114367#3720664

Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED
Pushed by nchevobbe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b51ec9e8d124 [devtools] Add AbortController support to EventEmitter. r=jdescottes.
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 92 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: