whatwg / dom

DOM Standard
https://dom.spec.whatwg.org/
Other
1.56k stars 289 forks source link

Provide an AbortSignal that aborts when Nodes become disconnected #1296

Open keithamus opened 10 months ago

keithamus commented 10 months ago

What problem are you trying to solve?

It's quite common to want to bind to event listeners on global objects, or parent objects to take advantages of delegate event listening. Doing so requires tearing down those listeners, which can be a little cumbersome.

What solutions exist today?

AbortController makes this easier but it can still involve a fair amount of boilerplate; here's the minimum viable code to register an event listener with appropriate AbortController code:

class MyElement extends HTMLElement {
  #controller = null;
  connectedCallback() {
    this.#controller?.abort();
    const signal = this.#controller = new AbortController();
    this.ownerDocument.addEventListener('whatever', e => {}, {signal});
  }
  disconnectedCallback() {
    this.#controller?.abort();
  }
}

How would you solve it?

First proposal I'd like to introduce an AbortSignal _intrinsic_ to the CE lifecycle, one that gets automatically set up _just before_ `connectedCallback()` and gets aborted _just before_ (or maybe _just after_?) `disconnectedCallback()`. In doing so, the above code could become something like: ```js class MyElement extends HTMLElement { #internals = this.elementInternals(); connectedCallback() { const signal = this.#internals.connectedSignal(); this.ownerDocument.addEventListener('whatever', e => {}, {signal}); } } ``` Whenever an element gets connected, it would abort and dispose of its associated connectedSignal, and create a new one. Calling `this.#internals.connectedSignal()` would return the _current_ associated connectedSignal. When an element disconnects from the DOM, it would abort its associated signal. This would alleviate much of the work around creating and managing an `AbortController` just to abstract the work of connectedCallback/disconnectedCallback.

I propose we add the following:

interface Node {
  AbortSignal disconnectedSignal()
}

Calling disconnectedSignal() returns a new AbortSignal. When the Node is disconnected, it aborts all AbortSignals created via this method.

Anything else?

No response

jpzwarte commented 10 months ago

What's the difference between this.#controller?.abort(); and this.ownerDocument.removeEventListener('whatever', ...) other than the event callback has to be a variable so it can be shared between addEventListener and removeEventListener?

(edited) I don't mean to criticise your proposal; I'm just not familiar with the AbortController technique and I'm wondering what the pros/cons are compared to add/removeEventListener.

keithamus commented 10 months ago

One difference is that AbortController signals can be shared with multiple event listeners, which means that it's 1LOC abort, rather than one for each tear-down:

class PointerTrackingElement extends HTMLElement {
  connectedCallback() {
    this.ownerDocument.addEventListener('pointerdown', this);
    this.ownerDocument.addEventListener('pointermove', this);
    this.ownerDocument.addEventListener('pointerup', this);
  }
  disconnectedCallback() {
    // This is a space where bugs can creep in, e.g:
    //   - Add a listener to connected, forget to put it in disconnected
    //   - Make a typo or copy-pasta error
    this.ownerDocument.removeEventListener('pointerdown', this);
    this.ownerDocument.removeEventListener('pointermove', thi);
    this.ownerDocument.removeEventListener('pointerup', this);
  }
}

class PointerTrackingAbortControllerElement extends HTMLElement {
  #controller = null;
  connectedCallback() {
    this.#controller?.abort();
    const signal = this.#controller = new AbortController();
    this.ownerDocument.addEventListener('pointerdown', this, {signal});
    this.ownerDocument.addEventListener('pointermove', this, {signal});
    this.ownerDocument.addEventListener('pointerup', this, {signal});
  }
  disconnectedCallback() {
    this.#controller?.abort();
  }
}

Of course these examples are concise in order to be illustrative but you can imagine a messy complex class with event listeners set up in various places, it can become tricky to collate them all into a disconnectedCallback. but ensuring events always have a signal allows for quick and simple cleanup.

In addition, AbortControllers can be used across other platform features. It can be useful to, for example, cancel fetch requests on disconnected elements:

class FetchAbortControllerElement extends HTMLElement {
  #controller = null;
  connectedCallback() {
    this.#controller?.abort();
    const signal = this.#controller = new AbortController();
    fetch('/results', { signal }).then((data) => this.load(data));
  }
  disconnectedCallback() {
    this.#controller?.abort();
  }
}

I believe this is the only way to cancel a fetch on disconnected elements.

justinfagnani commented 10 months ago

I like this idea, but I think the issue should either be in https://github.com/wicg/webcomponents or https://github.com/whatwg/dom no?

keithamus commented 10 months ago

While AbortController is part of DOM I think ElementInternals and the relevant CE parts are in HTML, so I think the majority of spec work would be in this repo, but I'm happy to take guidance here.

josepharhar commented 2 months ago

I like this idea, but I think the issue should either be in https://github.com/wicg/webcomponents

imo putting the proposal here is fine, but if filing issues there increases visibility to get feedback then yeah we should either file another issue there that points to this one or do something to get their attention.

justinfagnani commented 2 months ago

I like this idea, but I think the issue should either be in https://github.com/wicg/webcomponents

imo putting the proposal here is fine, but if filing issues there increases visibility to get feedback then yeah we should either file another issue there that points to this one or do something to get their attention.

I only meant that I thought DOM API proposals were supposed to be in the dom repo, but it's also true that I don't fully understand the repo organization for all the standards work! 😁

justinfagnani commented 2 months ago

I forgot about this issue when discussing this on Discord. I still like the idea, though I think there's a tricky consideration around component moves and when the signal is aborted.

This is highlighted by the fetch example:

class FetchAbortControllerElement extends HTMLElement {
  #controller = null;
  connectedCallback() {
    this.#controller?.abort();
    const signal = this.#controller = new AbortController();
    fetch('/results', { signal }).then((data) => this.load(data));
  }
  disconnectedCallback() {
    this.#controller?.abort();
  }
}

Let's say you're using this element in a list that gets reordered. You wouldn't want to abort the fetch, just to start it again immediately. The same applies to event listeners, but the downside may be less apparent in many cases. It could show up if you have large lists of components with event listeners that gets reordered - the cost of removing then re-adding listeners could show up.

So I think it'd be great to have a debounce option that only aborts the signal after a microtask. That way any move within the same microtask wouldn't abort. But... the problem with that is that connectedCallback and disconnectedCallback as still called, and you create the hazzard of running multiple setup phases in connection, without multiple cleanup phases. That may imply you want debounced lifecycle callbacks as well.

keithamus commented 2 months ago

imo putting the proposal here is fine, but if filing issues there increases visibility to get feedback then yeah we should either file another issue there that points to this one or do something to get their attention.

Good idea. I've filed https://github.com/WICG/webcomponents/issues/1061 where we can solicit feedback too.

smaug---- commented 2 months ago

I'm not quite sure why we'd want to limit the feature to custom elements.

keithamus commented 2 months ago

I'd be happy to look at an API which works for any element, what do you propose?

keithamus commented 2 months ago

Discussing this with @smaug---- in the Matrix channel, it might be better to have this attached to Node.prototype, and have an API which can return an AbortSignal for various CEReactions; as such something like following API might make sense:

interface Node {
  AbortSignal abortSignalFor(DOMString reaction)
}

I will raise this at the next WHATNOT meeting to get more input before proceeding further with prototypes.

keithamus commented 1 month ago

We discussed various aspects of this at WHATNOT. I'll attempt to summarise the discussion:

Given the above comments, I'd like to refine the proposal to:

interface Node {
  AbortSignal disconnectedSignal()
}

And I think this should return a fresh AbortSignal every time it is called, which gets disconnected on CEReaction timing. I think this offers enough flexibility for authors to finely control when they establish a signal, even outside of Custom Elements, but still keeps tight enough timing to avoid the risk of quirky async behaviours such as overlapping abortsignals with connected CEReactions.

domenic commented 1 month ago

Given the above comments, I'd like to refine the proposal to:

This proposal makes sense to me and seems nice and simple. To be clear, if you call this before connecting, it still works, and the signal will get aborted on the next disconnect?

keithamus commented 1 month ago

To be clear, if you call this before connecting, it still works, and the signal will get aborted on the next disconnect?

Yes exactly, I think that's a useful property for this API.

keithamus commented 1 month ago

On the topic of motivating examples, this GitHub search provides a selection of code examples where web component authors are using the pattern of creating a new AbortController() on connectedCallback and calling .abort() inside the disconnectedCallback. Some examples I glanced at which seem to closely fit the pattern:

Some examples of authors calling new AbortController some time later than connectedCallback, but still calling .abort() inside disconnectedCallback:

EisenbergEffect commented 1 month ago

tl;dr; I use something like this in my framework, but it's not exactly the same. Thus, I probably wouldn't be able to make use of this feature if it were added as is.

Since I was referenced, I'll chime in about what I've been doing. It's a little different than what is described here. Unfortunately, the "framework" I will be talking about is not open source at this time, so I won't be able to link to code or examples.

In said web component framework, each web component can have a template. When the component is first connected, that template is used to create a "view". The view renders into the light/shadow DOM for the component, updating the DOM based on how various data bindings were declared in the template. These bindings can be simple attribute or text bindings, nested template compositions, conditional rendering, loops, etc. Certain of these bindings require "cleanup". For example, loops generate nested child views. When the parent view is cleaned up, each child view must also be cleaned up. Nothing really new with any of this...

So, to get to the point...

The view provides an abort signal to bindings so that if they need to clean up, they can simply add an abort listener. The signal is passed to all event listeners, used to cleanup nested view from loops and conditionals, etc. It's also lazily created, so we don't incur the cost if none of the bindings actually need cleanup behavior.

We use the abort controller as an internal mechanism to coordinate cleanup across nested views. We do not expose it as part of the component API. Another difference is that the abort controller is not specifically a "disconnect" controller. It's an "unbind" controller, because for the purpose of bindings, they don't care about connect/disconnect. They care about whether they are bound to this state or that state or not bound at all. So, when state is unbound, the signal fires to enable teardown.

How does this relate to the custom element lifecycle then? Well, usually when a custom element is disconnected, its view will be unbound. However, we don't immediately unbind the view. We push that out by one microtask. The reason for this is that a web component may be removed and then re-added to the DOM within a single user task. It may just be something like re-ordering items in a list. When that happens, we don't want to tear everything down and then re-create it again...when actually nothing has happened that would change the internal rendering. That would be extremely wasteful.

So, to summarize:

If the feature this issue proposes were added, I'm not sure that we could use it for our purposes. If we did, that would de-optimize our rendering engine on certain operations, such as moving list items around.

There's a different proposal that I think could handle the same use cases as the abort controller: Custom Attributes. That proposal would allow 3rd party behaviors to hook into the connect/disconnect of any element in the DOM. Of course, it enables a lot of other scenarios as well.

justinfagnani commented 1 month ago

The reason for the mictotask is to not call .abort() is the element was only just moved. The isConnected check is the other important bit there:

    disconnectedCallback() {
      // we end the rendering loop only if the component is removed from de DOM. Sometimes it is just moved from one place to another one
      window.queueMicrotask(() => {
        if (this.isConnected === false) {
          this.#abortController.abort();
          this.#loop.return();
        }
      });
    }

This is a pattern I've used (though not yet with AbortController) to prevent cleanup thrash when moving elements, with the exact same timing. And this is my only reservation with this feature - that it would encourage thrashing. But honestly, I feel like debouncing is rare enough today in practice that it will not matter.

I agree with Dominic that this is nice and simple, so my concern may not be enough to change this to be more complicated. Especially because while debouncing is nice for some cases, it's actually a hazard for others.

Consider an element that adds listeners to window and it's parent:

connectedCallback() {
  window.addEventListener('click', this.#onWindowClick, {signal: this.disconnectedSignal()});
  this.parentElement.addEventListener('mouseover', this.#onParentMouseOver, {signal: this.disconnectedSignal()});
}

If disconnectedSignal() were debounced, it would save removing and adding the listener on window, but it would be incorrect for the listener on the parent, since the parent could have changed. So naively always debouncing is unsafe.

What you would need is an opt-in. Something like:

connectedCallback() {
  window.addEventListener('click', this.#onWindowClick, {signal: this.disconnectedSignal({debounce: true})});
  this.parentElement.addEventListener('mouseover', this.#onParentMouseOver, {signal: this.disconnectedSignal()});
}

And that can be added later. So +1 to this API as is.

keithamus commented 1 month ago

Thanks to both of you for your commentary here. I like the idea of adding new opt-in mechanisms via options bags. I also want to call out that there is a broader discussion happening around atomic moves in https://github.com/whatwg/dom/issues/1255, and I've filed https://github.com/whatwg/html/issues/10475 as I think it's important we capture a discussion about how we can solve the "atomic move" problem for Custom Elements.

Given the above discussion, I feel more confident that disconnectedSignal() is the right path to take, and patterns emulating atomic moves (such as debouncing) can be solved later. I'll continue prototyping the disconnectedSignal() API.

EisenbergEffect commented 1 month ago

Just to be clear, I'm definitely not opposed to this feature. Just wanted to share a few nuances/differences in the way I've been using the abort controllers/signals and explain some related scenarios. In general, I think something like this would be quite useful.

WebReflection commented 1 month ago

Let's say you're using this element in a list that gets reordered.

my thoughts in a nutshell ... this feature would make sense only after the one requiring no disconnect/connect handlers are invoked on reordering, so that the node never left the live tree.

On the other side, if this feature use case is to abort fetch operations, dare I say we're missing a destructor primitive where aborting would make more sense, as the element is clearly not reachable by any mean.

Until it's reachable, hence able to rejoin the live DOM state, I think all proposals are off because:

My 2 cents around this matter, it's a cool idea full of unexplored real-world alternative use cases.