Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide an AbortSignal that aborts when Nodes become disconnected #1296

Open
keithamus opened this issue Oct 25, 2023 · 20 comments
Open

Provide an AbortSignal that aborts when Nodes become disconnected #1296

keithamus opened this issue Oct 25, 2023 · 20 comments
Labels
addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest topic: aborting AbortController and AbortSignal topic: custom elements Relates to custom elements (as defined in DOM and HTML) topic: nodes

Comments

@keithamus
Copy link

keithamus commented Oct 25, 2023

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:

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

@keithamus keithamus added addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest labels Oct 25, 2023
@annevk annevk added the topic: custom elements Relates to custom elements (as defined in DOM and HTML) label Oct 25, 2023
@jpzwarte
Copy link

jpzwarte commented Oct 26, 2023

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
Copy link
Author

keithamus commented Oct 26, 2023

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
Copy link

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
Copy link
Author

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
Copy link
Contributor

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
Copy link

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
Copy link

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
Copy link
Author

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 WICG/webcomponents#1061 where we can solicit feedback too.

@smaug----
Copy link
Collaborator

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

@keithamus
Copy link
Author

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

@keithamus
Copy link
Author

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 keithamus added the agenda+ To be discussed at a triage meeting label Jul 8, 2024
@keithamus
Copy link
Author

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

  • Attaching to Node seems ergonomically useful for elements that aren't custom elements. It will likely require a fair amount of spec changes as it'll be the first time that a CEReaction is available to something that isn't a Custom Element.
  • There's not a clear use case for getting other signals back; AbortSignal doesn't seem to be the right pattern and the answer the platform currently provides in lieu of a new object is Events, but this gets worryingly close to mutation events which are aiming to be deprecated. The async alternative to mutation events is MutationObserver. More abstraction gets us further away from ergonomics, so perhaps other signals are best dealt with via MutationObserver.
  • There was discussion around whether the API should be a method that gets a fresh signal each time, or a property which resets itself. The property makes things a little more complicated, but both also have a lingering question of:
  • What does the property/method return for an as-yet-to-be-connected element. The prototype returns null which helps avoid problems of using it too early, but if it's on the Node prototype, a common pattern is to create an element, establish event listeners, and then connect the element. In that regard it seems to make sense to have it return a Signal pre-connect.
  • We had some discussion around the exact timing. Largely it is my belief that users of this pattern do what is effectively in the OP: call abort() on disconnectedCallback(), and mint a new abortcontroller every connectedCallback().
  • The discussion around timing also prompted questions about how prevalent this pattern is and whether or not there are resources we can learn from. WebComponents.guide and GitHub's Catalyst framework both describe this pattern (although this comes with the heavy caveat that I am the author of both of these 🙊). Talking with folks in the WCCG, some mentioned they also use this pattern a lot (e.g. @EisenbergEffect). Perhaps @justinfagnani can also provide guidance about the minutiae here.

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
Copy link
Member

domenic commented Jul 11, 2024

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?

@past past removed the agenda+ To be discussed at a triage meeting label Jul 11, 2024
@keithamus
Copy link
Author

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
Copy link
Author

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
Copy link

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:

  • We use abort controllers, but they are tied to the view/state lifetime rather than the DOM.
  • Typically, certain DOM interactions do result in view/state lifetime changes, but not always.
  • The signal is internal and not exposed, because, from a framework perspective, we don't exactly align the abort timing with the custom element lifecycle (for reasons already explained).

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
Copy link

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
Copy link
Author

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 #1255, and I've filed whatwg/html#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
Copy link

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.

@annevk annevk transferred this issue from whatwg/html Jul 18, 2024
@annevk annevk added topic: aborting AbortController and AbortSignal topic: nodes labels Jul 18, 2024
@WebReflection
Copy link

WebReflection commented Jul 18, 2024

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:

  • one does not simply interrupt a microtask ... if the node goes live again right before/oafter that microtask, the fetch would start over again for no benefit
  • tables where limited rows are visible but the sorting is "one click away" are still common ... nodes meant to be kept in that order/view eventually would not benefit from aborting previous operations on connect
  • infinite scroll based logic where visible content is not dozen thousand tweets, just those enough to satisfy the user screen height

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

@keithamus keithamus changed the title Tie in AbortControllers with CustomElements Provide an AbortSignal that aborts when Nodes become disconnected Jul 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest topic: aborting AbortController and AbortSignal topic: custom elements Relates to custom elements (as defined in DOM and HTML) topic: nodes
Development

No branches or pull requests

10 participants