How to avoid memory leaks (cleanup promises) when detecting custom elements?

[trusktr]

For example,

            if (elm.nodeName.includes('-')) {
                customElements.whenDefined(elm.nodeName.toLowerCase()).then(() => {
                    console.log(' --- element defined:', elm.nodeName)
                })
            }

I don't know if an element will be a custom element, but in order to try to detect this, it seems that I will leak memory due to promises that are never resolved. I don't know the names of the custom elements either, they are arbitrarily defined and can be different for every app.

[rniwa]

There Is no way to avoid leaks here.

[trusktr]

This this is not good then (even if it is just how Promises work). Can we make it cancellable?

ce.whenDefined(...).then(...).cancel() // or something

When a parent component unmounts (disconnectedCallback) it'd be nice to cancel waiting on promises so the component can be cleaned up.

[rniwa]

The problem is that we wouldn't know when a custom element may be defined in the future. Since any definition can be added arbitrarily further in the future, there is no way to determine this.

This is yet another reason we opposed to the async definition of custom elements but since that ship has sailed, there's nothing we can do.

[markcellus]

sorry I'm a little late to the discussion but this doesn't sound good. I just recently subscribed to this repository. @rniwa can you drop a link to the thread that lead up to the adoption of async definition of custom elements? I don't ever remember seeing it.

[rniwa]

You can follow #405 and related topics there; e.g. https://github.com/w3c/webcomponents/wiki/Custom-Elements:-Contentious-Bits and https://lists.w3.org/Archives/Public/public-webapps/2015JulSep/0204.html

Supporting upgrading of custom elements was raised as a show stopper issue from Google, so we all compromised to have it.

In general though, once a custom element is registered with a global object, we can't really remove it retroactively since you can always instantiate the same custom element later, and it should work.

Note that the API I proposed in #671 should be able to determine whether a given element will be upgraded to a custom element or not. But again, we can't really avoid leaking the promise since there is never a guarantee that an element will never be upgraded since a new custom element definition can come in any time in the future.

[trusktr]

The problem is that we wouldn't know when a custom element may be defined in the future.

It doesn't matter because if a component is removed and no longer referenced ("destroyed"), then it should be able to destroy the expectation it had for a future element. It doesn't matter if the element is defined in the future or not, just that we clean the memory that was waiting for that future.

[trusktr]

What I mean, is suppose SomeElement wishes to observe all children, and wants to specifically run logic when one of those children is instanceof OtherElement.

Now, imagine that the library author that ships SomeElement and OtherElement to the end user does not name the element. The library author tells the user

You can name these elements anything you want.

Then the user does so:

customElements.define('any-thing', SomeElement)
customElements.define('an-element', OtherElement)

Then the library author's SomeElement will need to have code like the following (based on the above) in order to detect the children and react only to instances of OtherElement:

        for (const elm of Array.from(this.children)) {
            if (elm.nodeName.includes('-')) {
                customElements.whenDefined(elm.nodeName.toLowerCase()).then(() => {
                    if (elm instanceof OtherElement) {
                       // do something only if the child is an OtherElement
                    }
                })
            }
        }

This is a completely fair thing for a library author to want to do given the API that Custom Elements v1 ships to people (authors and users).

If for some reason, the user's app route changes to some other page, and perhaps the SomeElement is not needed any more, and things weren't done loading, then SomeElement might be disconnected, and it would be fair to prevent SomeElement from running logic for child OtherElements.

There's two things to note:

1. The elm.nodeName might be a name for a future custom element, which will eventually be defined.
2. It might not ever be defined (the web allows us to write elements with any name even if they don't exist, so people will do that sometimes).

In general, we should be able to clean up. I'm sure there's more examples that just this one. f.e., suppose we clean up all the Promises after a few minutes (or seconds) if they are never registered.

Being able to clean up memory is essential for long-running desktop applications. People are making them with Electron...

[trusktr]

Traditional events have ways to clean up (.removeEventListener(), .off(), etc), so it seems fair to need this for Promise-based APIs which are essentially one-time future events, similar to .once() in some event libraries.

Maybe this is a more general problem for JavaScript. Maybe Promises returned from .then() simply need to be cancellable somehow, just like ancient event mechanisms have ways to remove event listeners.

[trusktr]

What I'll do for my un-named custom elements that I want users to name, is to expose specific API on the classes for that, so users do SomeElement.define('any-thing'), OtherElement.define('any-element'), etc. By doing this, I can map constructors to names (rather than other way around with customElements.get()) so I can at least leak Promises only to the specific names for my classes.

In this case, if the elements are never defined, then there's something wrong, and I can throw a warning or error after a timeout.

The downside is that it won't work with customElements.define('any-element', OtherElement) unless I monkey patch that. Hmmm... Maybe I'll just monkey patch. Yeah, I think that's a good idea (unless someone force replaces it after the fact with a forced polyfill).

[rniwa]

The problem is that we wouldn't know when a custom element may be defined in the future.

It doesn't matter because if a component is removed and no longer referenced ("destroyed"), then it should be able to destroy the expectation it had for a future element. It doesn't matter if the element is defined in the future or not, just that we clean the memory that was waiting for that future.

What happens when the node is then get inserted back into the document? It is the case that DOM methods such as appendChild that insert a node momentarily removes the node before inserting it back into the document. This would mean that if you ever move a node from one place to another, it would reject the promise. I don't think that's the behavior you want.

In this case, if the elements are never defined, then there's something wrong, and I can throw a warning or error after a timeout.

The problem is that you'd never know that. You never when the page had stopped loading more scripts. FWIW, a web page can have a code like setTimeout(loadCustomElementDefinition(), 1000000 * Math.random()) and just load the custom element definition at an arbitrary time. Worse yet, this could happen in response to network latency, errors, etc...

[trusktr]

What happens when the node is then get inserted back into the document?

Then the node can allocate new memory again, using whenDefined again.

It is the case that DOM methods such as appendChild that insert a node momentarily removes the node before inserting it back into the document.

This is exactly why in my web components I implemented init and deinit methods in my custom elements. init is fired the first time an element is connected. deinit is fired if and only if the element was removed but not reconnected in the same synchronous tick. If the tick finishes, and the element is not back in the DOM, then deinit is fired in the next microtask (simple implementation here and here).

I don't think that's the behavior you want.

This would mean that if you ever move a node from one place to another, it would reject the promise. I don't think that's the behavior you want.

You're right, hence my init and deinit. When I clean up is not of concern, rather that I should be able to cleanup if I want to.

In this case, if the elements are never defined, then there's something wrong, and I can throw a warning or error after a timeout.
The problem is that you'd never know that.

But if someone is trying to render a WebGL scene with my elements, and the elements are not register after, say, a minute, there may likely be something wrong. I can at least show a warning in this case.

But that's irrelevant to this argument, because if my elements are unmounted (and my deinit methods called) then I'd like to clean up no matter what rather than unexpected logic firing in the future.

Importantly: rather than unexpected logic firing in the future. (and without leaks)

FWIW, a web page can have a code like setTimeout(loadCustomElementDefinition(), 1000000 * Math.random())

This is true, but despite this fact, we all still reserve the right to clean memory up because as a browser dev, you also don't understand every single use case that an HTML/JS dev could have (f.e. mine). Everyone writing C++ has this option. Why can't HTML/JS devs?

Worse yet, this could happen in response to network latency, errors, etc...

When the library is ready to try again, it can easily call whenDefined again. That's no problem.

[tomalec]

@trusktr I'd like to make sure I understand your need correctly. You would like to give the author a way to explicitly reject all promises for whenDefined given custom element? Like,

customElements.define('any-thing', SomeElement);
// SomeElement logic:
for (const elm of Array.from(this.children)) {
    if (elm.nodeName.includes('-')) {
        customElements.whenDefined(elm.nodeName.toLowerCase()).then(() => {
            if (elm instanceof OtherElement) {
               // do something only if the child is an OtherElement
            }
        })
    }
}
// once author knows that `an-element` definition will never come, for example because he/she deliberetly didn't import it
customElements.stopWaitingFor('an-element'); // resolves alls promises for this element name with rejection, letting callback to be collected.

or just for a given call? To reject a promise that this particular element will be upgraded.

customElements.define('any-thing', SomeElement);
// SomeElement logic:
for (const elm of Array.from(this.children)) {
    if (elm.nodeName.includes('-')) {
        elm.whenUpgraded.then(() => {
            if (elm instanceof OtherElement) {
               // do something only if the child is an OtherElement
            }
        })
    }
}
// once author knows that given element will never get upgraded to `an-element`, 
rejectUpgradePromise(document.querySelector('any-thing an-element'), 'reason: nuked'); // something that rejects a promise for that instance

[trusktr]

@tomalec Yes, exactly, something like that. It just seems dirty not to have a way to clean up.

To paint a small picture of the problem:

Suppose a lib has a custom element that needs to detect if certain child elements are instanceof ACertainClass regardless of what name the end user of the library assigned the constructor to.

The custom element would have to call whenDefined(elementName) of every child's name just to detect if only some of them are instances of a constructor of interest.

So, if an application has thousands of nodes, this can lead to many Promises leaked inside of customElements which the HTML engine is waiting to resolve() but will never resolve them.

This problem still applies with @rniwa's whenDefined(elementInstance) idea.

[trusktr]

@tomalec Actually, the problem is that I am letting users define the element names. That way they have controls of the names they use in their DOM incase there's ever a conflict with some other library, or etc. So I myself (as author of my library) am not specifying the element names unless the user calls an API of mine that will use default element names. But the user has the option to skip use of the API and name them anything they want.

F.e.:

import {useDefaultNames} from 'joes-lib'

useDefaultNames() // registers elements with default names defined in the library.

They don't have to use that function, they can import and register the classes themselves.

So because I don't necessarily control the element names, this is the code I have at the moment in order to try and give them a useful Error message in the console:

        async _checkElementIsLibraryElement(element) {
            if ( element.nodeName.includes('-') ) {
                const whenDefined = customElements.whenDefined(element.nodeName.toLowerCase())
                    .then(() => {
                        if (element instanceof Mesh) return true
                        else return false
                    })
                const timeout = new Promise(r => setTimeout(r, 10000))

                const isMesh = await Promise.race([whenDefined, timeout])

                if (!isMesh) throw new Error(`
                    Either the element you're using the mesh behavior on is not
                    a Mesh element, or there was a 10-second timeout waiting for
                    the Mesh element to be defined.
                `)
            }
            else {
                throw new Error(`
                    The element you're using the mesh behavior on is not a Mesh
                    element.
                `)
            }
        },

As you can see, in this case if the timeout wins the race, then the whenDefined promise will be left dangling (though hopefully the error will lead to them fixing the situation).

---

Another case could be a name conflict, someone may want to unregister them (and not be leaking memory).

---

In general, I think Promises just need a way to be cleaned up (cancelled, or at least unlistened to without affecting other listeners).