1709997 - Add support for AbortController in EventEmitter

Status: RESOLVED FIXED

(DevTools :: Shared Components, task)

Description

[Nicolas Chevobbe [:nchevobbe]]

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.

Comment 1

[Alexandre Poirot [:ochameau]]

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.

Comment 2

[Alexandre Poirot [:ochameau]]

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

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1709997 - [devtools] Add AbortController support to EventEmitter. r=jdescottes.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b51ec9e8d124
[devtools] Add AbortController support to EventEmitter. r=jdescottes.

Comment 5

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/b51ec9e8d124