Save event handler on node or instance itself

[Fuzzyma]

Problem

Issue #805 revealed a memory leak on our library. It can be resolved easily but it made me thinking.
When someone binds an event to an element, the element gets saved in an array so we have a connection from element to handler. However when the element is removed from the dom, the array is not cleaned up.

@kuzmanovicnenad mentioned that it would be good to remove all event handlers when we remove the element from the dom. However, this approach has the problem that dom nodes can be reattached to the dom. A simple front() call deletes the node and reattaches it at the end of the node list.
So this solution is not so great.

Solution

Thats why I propose the solution, that the event handlers are simply not saved in an array anymore but on the element itself. We could have a property _events on any instance where we can save this sort of stuff. But this holds a problem, too. We allow binding of events on normal HTML nodes as well so we would need to save the handlers on the node which means: Altering the native API - baah.

But I dont see a better solution to prevent memory leaks. Because: When all references to a node are dropped and the node is not in the dom anymore, everything saved on the node is cleaned automatically. One thing less to think about.

@saivan @wout @kuzmanovicnenad any thoughts on this?

[saivan]

I'm not so clear as to why what you're proposing (which I think is quite sensible btw), is going to require a modification to the native API, can you explain some more?

[Fuzzyma]

In case someone wants to bind events to html nodes like this:

var div = document.getElementById('SomeDiv')

SVG.on(div, 'eventname', fn)

it would save the handlers on the html node:

div.events['eventname'] = fn

And in my opinion any property change on a native object which is not covered by the spec is a modification of the native api.

This is no modification: node.id = 'newId'
But this is: node.someRandomName = 'foo'

[kuzmanovicnenad]

appendChild() won't remove events from element so front() method is safe (unless I'm missing something in your code..)

You don't have to remove events on remove() if off() is working properly (but I would expect it from methods name), and if lib is used properly.
I think it's developers responsibility to use remove() or clear() methods for removing elements and not to remove it with $('rect').remove() or whatever. But you can't stop them, can you?

I don't know if it's possible but can you just attach native event handlers to elements?
This would fix the problem above and need for a potential mem leak array.

Regarding this:

div.events['eventname'] = fn

It will make a problem if:

SVG.on(div, 'click', fn1)
SVG.on(div, 'click', fn2)

...but could be solved with array of handlers...

[Fuzzyma]

front() was just an example. The point is, that remove is not a good place to delete event handlers because removed elements can be added back to the dom later.

Actually we use native event handlers. However, to make el.off() possible, you have to know which listeners were appended to the element so you need to track all listeners.

And ofc, my code was just pseudocode. It would be more like this:

div.events['eventname']['namespace'] = [fn1, fn2]

Just like at the moment but only directly on the element

[saivan]

Is there anything wrong with binding the new event to the dataset of the element in question? This is covered in the spec and is where you probably should put things like this, maybe an events array to the dataset and just run all of the events on click.

[saivan]

That way when the element gets removed from the dom, its dataset will be flushed too.

[Fuzzyma]

I thought about that, too. But the stuff we save on the element are listeners (functions) and you can only save strings in the dataset

[wout]

The pleasure of maintaining a separate bookkeeping. :) There are two solutions I can think of right now.

1. Store on element instance
We already befouled the native API with our instance reference. So why don't we put it there? (e.g. node.instance.events instead of node.events). We only need to track the events attached using SVG.on. Any other event binding, like vanilla or jQuery, is not our responsibility.

2. Destroy
Make a distinction between removing an element from the DOM (e.g. element.remove()) and marking an element for destruction, where I propose a new method called element.destroy().

With the current API, there is no way for us to know if a user is removing an element to insert it back later or to actually get rid of it. That's why we can't unbind events on remove(). With destroy(), we can be sure of the user's intention.

[Fuzzyma]

Regarding your first point: When we bind an event to an HTML node (with SVG.on(div, ...)) we dont have an instance property where we could save the events. Even the new SVG.HtmlNode will not save an instance attribute on the element because its an object which is created purely to use it only once (think about el.addTo('#someDiv')).

So either we create an instance property anyway even if there is none or we use a new one. Wouldnt make much of a difference. However I dont like reusing the same property if it does not mean the same.

I am not a fan of your second solution. That are 2 methods which basically do the same thing. They remove an element from the dom. There is no difference from the user perspective. Js user do not tend to think about mempory management so they might not care which one they use. Furthermore it does not solute the problen when you bind events to native elements again

[wout]

True, when HTML elements are thrown into the mix, we are screwed. In that case, neither is a viable solution.

What about, instead of polluting the native API even more, create an object (our namespace) to store things related to SVG.js? So:

node.instance

Becomes:

node.svgjs.instance

Then we can store the events in there:

node.svgjs.events

We need to add at least one property on the original node anyway, so why not make it to one that can serve many purposes. Who knows what else we need to reference in the future?

[Fuzzyma]

I am quiet for so long because I just dont know :D.
I like the simplicity of just node.instance but I understand that its required to keep dom pollution to an absolute minimum.
The biggest problem with this approach is, that we cant do stuff like node.instance || node anymore. Using node.svgjs.instance || node would fail when svgjs is not defined. It than would need to be node.svgjs && node.svgjs.instance || node which I personally dislike.

We might keep the advantages when we use node.instance.events. That would require a bit more care in event handling and adoption of elements since we cannot just check for instance but we must also check if node.instance instanceof SVG.Element. But I think this way round its better than the other way

[wout]

Surely it adds more hassle and it looks a bit less clean, I'm also on the fence about this one :D.

In your last example, would you create an object called instance on HTML nodes? Because we could also just store the events on the node itself if it's a HTML node (! node instanceof SVGElement). That way, we don't need to abuse the instance property.

[Fuzzyma]

Yes ofc we could do this but I wanted to simplify access to the events so thats always the same path you need to take instead of node.instance ? node.instance.events : node.events (in this example you cant even be sure that node.events exists and you need to check that again, too. Imo it gets kinda complicated if you store the events different on HTMLNode vs SVGElement