Hyperapp: "Cleaning" after replaceChild

[mehdi-cit]

Hello all,
I've been reading the code and I'm not sure DOM native listeners are properly removed when replacing a node (not clear if removing listeners is automatically handled by the different browsers).
Moreover, it seems the hooks are not invoked as well (shouldn't onRemove be invoked when replacing a node by another one).

[jorgebucaran]

shouldn't onRemove be invoked when replacing a node by another one

Yes, possibly 👍.

DOM native listeners are properly removed when replacing a node

Can you be more specific? What are native listeners?

[mehdi-cit]

Sorry about that :)
Cleaning/disposing of listeners to native DOM events such as "onclikc".
Let's say we are replacing a node_1 (which has children node_1_1 and node_1_2) with node_x. In case node_1 or any of its children have registered listeners to native dom events or have registered hooks.
Isn't it that we must make sure we dispose of all listeners (in node_1, node_1_1 and node_1_2) as well as call any onRemove on anyone of these nodes before doing the replacelement?

[jorgebucaran]

@mehdi-cit No probs :) Is this related to https://github.com/hyperapp/hyperapp/issues/160 maybe?

@dodekeract Ping.

[mehdi-cit]

Yes, it could be related.
I just read the app.js file. In general I noticed that it can be a good exercise to read code and try to understand the flow ( especially nice & concise code like hyperapp's ).
Simply reading a printed copy of the code on a piece of paper does -in my case at least- help a lot. One can spot potential issues/bugs that weren't found through tests or unit-tests.

About this specific issue. I think we need to re-read the code paying attention to how we dispose of listeners (both to native DOM events and to hooks) when a bunch of nodes ( and their children ) are being discarded (either removed or replaced). We make sure we do not leave any listeners "hanging out". humm.. I'm not pro-efficient with chrome dev-tools but these may help if we are having some memory-leaks.

I'm just thinking out loud here. But one could consider keeping at the parent level (basically on every node) a stack/array to reference any listeners that are defined on the node itself or on its children. This way, if we need to dispose of some node, we need not traverse all its children to make sure we do not leave out listeners/handlers since we can just use the stack to pop-out and removeListener/invoke-onRemove to cleanup on the parent node. Obviously this means any listener would be referenced by the node it's declared on, plus all the parents of this node (it's a trade off, not sure what's best!!!).

It could also be possible to consider disallowing the declaration of DOM events handlers on the nodes but only allowing registering handlers through selectors in a well pre-defined function at the module-level (exp app.registerListeners() => {....}). Meaning, one can only declare such handlers in registerListeners and remove them in another pre-defined app.removeListeners! This would only leave us with the task of making sure onRemove is properly invoked.

Wonder how other libraries deal with this issue (if it's an issue at all).

[mehdi-cit]

Made a quick research and DOM events listeners should be dealt with the garbage collector as long at they are no longer referenced (meaning we need not remove/dispose the listeners when replacing a containing DOM node).
The above should be true for modern browsers (NOT IE8 ...).
In short, and as long as hypperapp doesn't not support older IE versions then we need only make sure to run the onRemove hook when replacing a node_X (any node_Y that is a child of node_X and that defined onRemove must have its onRemove called prior to replacing node_X).
I hope I got it right

[jorgebucaran]

We support IE10 or at least are aiming to support at least IE10.

[jorgebucaran]

Closing as:

DOM events listeners should be dealt with the garbage collector as long at they are no longer referenced (meaning we need not remove/dispose the listeners when replacing a containing DOM node).