search

dfilatov / vidom

Library to build UI based on virtual DOM
MIT License
415 stars 16 forks source link

quick fix solution for removeEventlisteners #41

Closed ghost closed 9 years ago

ghost commented 9 years ago

It just stroke my mind. Have a look at this: https://github.com/dfilatov/vidom/blob/master/lib/nodes/TagNode.js#L122

You are treating this as attributes now as I understand. So you wouldn't need this line anymore. When you do a diffing / patching and updates the attributes, you should also update the events there. And add / remove directly.

Got it? It will also give you performance gains in the iteration loop.

Further. I don't think this code snippet are needed anymore? https://github.com/dfilatov/vidom/blob/master/lib/nodes/TagNode.js#L25

dfilatov commented 9 years ago

https://github.com/dfilatov/vidom/blob/master/lib/nodes/TagNode.js#L122 -- It's needed in case of recursive unmount. because https://github.com/dfilatov/vidom/blob/master/lib/nodes/TagNode.js#L25 -- yes, it's redundant.

ghost commented 9 years ago

Then I suggest you do a rewrite on that part. I see no point in keeping on when you never use it, and it's a slow down in the iteration loop.

ghost commented 9 years ago

@dfilatov I see your issue with unmount. Again I need to think a little here. I will open a new issue ticket if I come up with a suggestion.