ondestroy in custom elements

[Rich-Harris]

Trying to wrap my head round this while looking at #1117. I don't think ondestroy should be called for a custom element when it's detached (with disconnectedCallback) because it means something slightly different in custom element land — it can be a temporary thing (the element can be reinserted), whereas destroy is permanent. In other words, destroy should still be called manually for custom elements inserted with e.g. document.body.innerHTML = '<x-app/>'.

But when we have a situation like this...

{{#if foo}}
  <x-thing/>
{{/if}}

...and x-thing is a Svelte custom element, we probably do want to call ondestroy, since there's basically no sensible way to do that programmatically.

Which means we need a reliable way to distinguish between Svelte custom elements and non-Svelte custom elements (where destroy could potentially mean something different, meaning duck typing isn't necessarily reliable).

[Swizz]

A like in the Context API tutorial, I used slotted custom element childs to set the main object. Also like a video element with child sources.

But while a child is removed from the dom, I would love to unset the property related to the child presence. Removing a Marker from the map for example.

But without the disconnectedCallback or a MutationObserver, I have no clue about how to achieve this.

[dunqan]

Is there any workaround to get the info if a svelte custom element is disconnected?

In case, where a custom element is dynamically created via some third party orchestrating app (e.g. as a reusable component on a cms driven view) it could easily lead to memory leaks.

For example, in vuejs/vue-web-component-wrapper they also decided to not use destroy hook from the exact same reason, but you can hook into deactivated and easily call destroy from that if you need (making possible to do the proper clean up for both internal logic and the wrapper stuff).

[hontas]

Can the reinsertion issue be manifested in a test? When does it occur?
And could it be a solution to simply call the $$.on_destroy callbacks directly from disconnectedCallback instead of $destroy?
Without this it is hard to clean up after components and we have problems with memory leaks because of this.

[nolanlawson]

For anyone looking for a temporary solution to this problem, this is what I'm using:

class SubElement extends SvelteElement { // extend the Svelte custom element here
  disconnectedCallback() {
    this.$destroy()
  }
}

This definitely seems like a tricky issue. Intuitively, as a web developer, I would expect that:

document.body.appendChild(element)
document.body.removeChild(element)

... should result in no memory leaks. There are no more references to the element anywhere. Unless... you have a Svelte action, e.g. which uses ResizeObserver/IntersectionObserver/etc. Then the destroy() never gets called, so the component leaks.

My preference is to have the entire Svelte component created in connectedCallback() and destroyed in disconnectedCallback(). I.e. if you remove and re-insert a custom element, then the entire Svelte lifecycle is invoked all over again. But I admit I haven't thought too deeply about this.

[benkeil]

@nolanlawson do you have a working example? I don't get your code...

[zhaoyao91]

@nolanlawson hi, may I know how to apply this workaround?

[yannkost]

@zhaoyao91

You have to add

 disconnectedCallback() {
    this.$destroy()
  }

in the SvelteElement class.
In the index.js internal file of svelte. (node_modules/svelte/internal/index.js), search for exports.SvelteElement = class extends HTMLElement { and you will be in the right place.

A related thread here

[arackaf]

Update to @yannkost's comment. You don't actually have to modify Svelte internals. Doing this will be good enough

import { SvelteElement } from "svelte/internal";

SvelteElement.prototype.disconnectedCallback = function () {
  this.$destroy();
};

SO LONG AS that code runs before your custom elements are define'd. That code MUST run first, before any Svelte (web component) components are imported / parsed / defined. More context here: https://twitter.com/justinfagnani/status/1359213920272044034

So you may need to tuck all of your web component imports behind a single require() call, which is run after the code above executes. webpack will put things together properly - dunno about Rollup.

/cc @nolanlawson

[yannkost]

@nolanlawson
Ah why didn't I think about something so simple. I will test this out and check if rollup handles this good as well.

[antony]

Marking as fixed by #4522