Automatic cleanup of nodes and event listeners

[jiayihu]

What happens when a node is replaced by another one?

Let's take the following example:

const routerOutletEl = document.createElement('div');
const router = new HyperHTMLApp();
const renderRoute = hyperHTML.bind(routerOutletEl);

function handler1() {
  // Noop
}

function handler2() {
  // Noop
}

// First render
renderRoute`<my-comp onevent=${handler1} />`

// Update
renderRoute`<another-comp onanotherevent=${handler2} />`

What happens with the instance of <my-comp> ? Is it gargage-collected or is always there in memory ready to come back? The same question arises for the event handler (I'm not using DOM handleEvent on purpose because I prefer this way).

TLDR; I'm wondering if hyperHTML already cleans up the stuff for me when elements are "unmounted" or better disconnected, or I have to do it myself somehow. Should I add a disconnectedCallback in my CE and remove the event listeners?

The question arised because I was trying the Performance monitor in Chrome and going back&forward between home and a card in https://ygo.now.sh/ seems to always increase DOM Nodes and event listeners.

[WebReflection]

hyperHTML uses addEventListener so there's literally nothing you should do unless you hold in memory every node so hyperHTML can't do anything there.

(I'm not using DOM handleEvent on purpose because I prefer this way)

That also might be your issue. If you have every single reference a different listener, i.e. an arrow function, you are trashing N times all listeners and putting back new, causing unnecessary GC.

The handleEvent mechanism solves this because you'll never be able to attach twice the same node so that nothing extra needs to be don in core.

It's a powerful mechanism you should embrace, instead of avoid, IMO.

As example, I'm not too familiar with TS but if this creates a new listener each time then you have your issue: https://github.com/jiayihu/ygo/blob/master/src/components/Navbar/Navbar.ts#L29-L37

Also consider that methods called handleAnything are automatically bound to the instance https://github.com/WebReflection/hyperHTML-Element/blob/master/index.js#L177-L192

That means you don't need to make the prototype method a getter or an invoke, just call it handleClick and use it via onclick=${this.handleClick}.

TL;DR I cannot review your whole app but before blaming hyperHTML or HyperHTMLElement please be sure you are using these tools in the best way 😉

[jiayihu]

I'm not blaming hyperHTML and for the same reason I provided the small snippet, to allow us to discuss without anyone reviewing my whole app. Just trying to dig into hyperHTML.

Also I'm not using an inline arrow function when rendering, so that cannot be the reason if you meant doing <my-com onevent=${() => console.log('event')} />

hyperHTML uses addEventListener so there's literally nothing you should do unless you hold in memory every node so hyperHTML can't do anything there.

I'm not holding in memory the nodes, I'm just doing the following as I added to hyperhtml-app myself. When a route event happens, I render the relative template. And also I declared the event handlers on purpose, to show that I'm not using inline arrow functions.

// First render
renderRoute`<my-comp onevent=${handler1} />`

// Update: what happens to the previous nodes?
renderRoute`<another-comp onanotherevent=${handler2} />`

I'm asking: what happens with the previous nodes, when you call the same binded renderRoute function with a new template? As I was thinking, event listeners are added via addEventListeners but if a new my-comp node is created each time I change route from another-comp to my-comp, that could explain the potential memory leak.

As example, I'm not too familiar with TS but if this creates a new listener each time then you have your issue

This declares an arrow function property for each instance of the class, not each render. Besides it's not TS specific syntax, it's the same in Babel class properties. Also it should not be an issue if previous instances are garbage-collected.
In particular my expectation is that if I render this.html<my-navbar />, then this.html<something-else /> later, the previous instance of MyNavbar should be garbage-collected and its event listeners be detached (since I binded them declaratively within the template and not manually).

[WebReflection]

what happens with the previous nodes, when you call the same binded renderRoute function with a new template?

This happens.

The WeakMemory relation gets updated via bewitched.set(this, { template: template, updates: updates }); and the whole content gets trashed via this.textContent = '' before the new template gets appended.

Nothing is holding previous listeners, or nodes, anywhere.

if a new my-comp node is created each time I change route from another-comp to my-comp, that could explain the potential memory leak.

Nope, nothing is held within the library, listeners speaking, but maybe the GC takes longer than it should to understand it can trash private variables used to remember the previous value of any attribute, including listeners?

https://github.com/WebReflection/hyperHTML/blob/master/index.js#L1087-L1093

The issue is that there's only one holder of that scope, which is here, but we're back to the previous dance, so that when the context reference is updated, the previous updates reference will also go away: https://github.com/WebReflection/hyperHTML/blob/master/index.js#L1269-L1270

This declares an arrow function property for each instance of the class, not each render.

1. it's an anti pattern, specially if you care about memory, because it creates O(n) extra methods per each instance
2. you don't need it. HyperHTMLElement does that for you already but lazily, so you don't have O(n) creation but only when/if you need that listener per each instance

the previous instance of MyNavbar should be garbage-collected and its event listeners be detached

Accordingly to the code, that's exactly what happens. But if you have related (wired) same objects with more templates then you might have in memory more than you think, to have the render of each wire be ready when needed.

[WebReflection]

P.S. how about you try with this instead ? no arrow, no need for it. Maybe it'll improve already your stats.

  handleClick(event: MouseEvent) {
    event.preventDefault();

    const anchorEl = event.target! as HTMLAnchorElement;
    const dispatchedEvent: RouteChangeEvent = new CustomEvent('routeChange', {
      detail: { route: anchorEl.pathname } // `href` property includes the host
    });
    this.dispatchEvent(dispatchedEvent);
  }

[jiayihu]

But if you have related (wired) same objects with more templates then you might have in memory more than you think, to have the render of each wire be ready when needed.

I'm pretty sure I carefully used wire and always "wired" with relative objects.

it's an anti pattern, specially if you care about memory, because it creates O(n) extra methods per each instance. You don't need it. HyperHTMLElement does that for you already but lazily, so you don't have O(n) creation but only when/if you need that listener per each instance

I've also read your article about handleEvent and it was convincing. For the time being, I'm just lazy and biased, so I recognize it's not best practice but "nah" :)

it creates O(n) extra methods per each instance

As far as n is the number of methods and not number of renders I'm fine with it for my simple usage. It will precisely create Θ(m * n) methods, where n = number of methods and m = number of instances, but that's fine for me for now.

Anyway I'm planning to dive into the source code in the next days, so I'll come back if I find something interesting. Thanks for pointing me where to start looking for.

[WebReflection]

I've also read your article about handleEvent and it was convincing. For the time being, I'm just lazy and biased, so I recognize it's not best practice but "nah" :)

You keep not understanding that every method named with handleXXXX automatically does that binding for you so it's not about handleEvent, do you understand? If you have a method called handleClick the HyperHTMLElement class will change it during definition, as linked already before.

It binds it already for you, you don't need instance methods for that, it's done regardless.

I hope this is cleaner now, so ... how about you try this?

  handleClick(event: MouseEvent) {
    event.preventDefault();

    const anchorEl = event.target! as HTMLAnchorElement;
    const dispatchedEvent: RouteChangeEvent = new CustomEvent('routeChange', {
      detail: { route: anchorEl.pathname } // `href` property includes the host
    });
    this.dispatchEvent(dispatchedEvent);
  }

instead of this ?

handleClick = (event: MouseEvent) => {
    // THIS IS NOT NEEDED 'CAUSE IT'S CALLED handleClick
  };

It will precisely create Θ(m * n) methods, where n = number of methods and m = number of instances, but that's fine for me for now.

You can easily go Θ(1 + u) where u is usage, not the number of instances, neither the number of methods, since it's a getter.

All you need to do is already explained but it seems you are not getting what I am saying.

[WebReflection]

Just in case you are misunderstanding the difference, you don't have to pass onclick=${this} if you have handleClick, you can keep passing onclick=${this.handleClick} exactly like you are doing now.

You have nothing to lose, only to gain in terms of operations, memory, and performance using handleClick() {} instead of handleClick = () => {}.

I won't go through this again though, 'cause I am sure it's clear now what handleXXXX does with HyperHTMLElement.

[jiayihu]

Just in case you are misunderstanding the difference, you don't have to pass onclick=${this} if you have handleClick, you can keep passing onclick=${this.handleClick} exactly like you are doing now.

Yup I was misunderstanding buuut: I tried once and this in the handler refers to the binded DOM node, not the Custom Element itself, which is instead what I want when I declare an arrow function property. My understanding is that binding as you suggest, it's exactly as addEventListener (actually it is based on your link) and therefore the value of this is the same as event.currentTarget.

Codepen

[WebReflection]

I honestly don't understand. I've been writing HyperHTMLElement all the time because that's what you use in that card games and all the links were pointing at that one.

Here how is life with HyperHTMLElement components:
https://codepen.io/WebReflection/pen/GXOEVb?editors=0010

hyperHTML itself has nothing to do with custom elements and you filed a bug ab out custom elements and some leak.

[WebReflection]

class HyperWelcome extends HyperHTMLElement {
  handleClick() { console.log(this.tagName); }
  handleInput() { console.log(this.tagName); }
  render() { 
    this.html`
      <button onclick=${this.handleClick}>Logs h-welcome</button>
      <input oninput=${this.handleInput} />
    `;
  }
}

HyperWelcome.define('h-welcome');

This is how things work in HyperHTMLElement: you don't need the arrow function at class definition for handleXXXXX events

[WebReflection]

This was me:

before blaming hyperHTML or HyperHTMLElement please be sure you are using these tools in the best way

you don't need it. HyperHTMLElement does that for you already but lazily, so you don't have O(n) creation but only when/if you need that listener per each instance

If you have a method called handleClick the HyperHTMLElement class will change it during definition, as linked already before.

I won't go through this again though, 'cause I am sure it's clear now what handleXXXX does with HyperHTMLElement.

So please, next time, maybe read a bit more carefully so none of us would waste any time.

Thanks.

[jiayihu]

Clearly these discussions are better to be done in Monday and not Friday, my eyes skipped hyperHTMLElement as soon as they started reading hyper... 😅

Sorry to have made you waste time, have a good week-end.

[WebReflection]

FYI the patch that landed to fix #270 might help your cause too since you are a TypeScript user and turns out TypeScript sucks at transpiling template literals in a standard way.

[jiayihu]

Nice, but I have es2015 as target since CE work best with native classes AFAIK. And about the memory leak...well upon further profiling it was due to forgetting to destroy the chart shown within cards 🤦‍♂️

I was too interested in knowing what happens when nodes are disconnected and I mistakenly immediately thought it was related to hyperHTML. Sorry again for the annoyance 😄