patrick-steele-idem / morphdom

Fast and lightweight DOM diffing/patching (no virtual DOM needed)
MIT License
3.21k stars 129 forks source link

`onNodeAdded` called with node removed from the document #183

Closed chromakode closed 4 years ago

chromakode commented 4 years ago

Thank you for morphdom!

I ran into a corner case today in an onNodeAdded, in which it can be called with nodes that have been removed from the DOM.

When a keyed node is reused and moved to a new parent in a "to" tree, onNodeAdded uses replaceChild to replace the "to" childNode node with the existing unmatchedFromEl node:

https://github.com/patrick-steele-idem/morphdom/blob/fe35db9adda1f22fe5856e8e0f78048f8f4b0f18/src/morphdom.js#L156-L164

If I'm understanding correctly, this does two things which surprised me as a user:

The documentation of onNodeAdded states:

onNodeAdded (Function(node)) - Called after a Node in the to tree has been added to the from tree.

I discovered this because I was trying to access the parentNode of an node in an onNodeAdded callback, and was surprised to find it null -- I'd assumed all nodes onNodeAdded would only receive nodes currently in the document. So I think this probably is a bug and merits some change.

However, what should the correct behavior be here? In this case, the node was originally in the from tree, so should morphdom skip calling onNodeAdded? Or, because this node (and its children?) were removed/readded to the DOM, should onNodeAdded be called with the "from" node?

I'd be happy to add a test and implement the required changes if once it's clear what the correct behavior here should be.

snewcomer commented 4 years ago

@chromakode 👋 I think a test case would be very useful! I'm a bit confused since handleNodeAdded happens when a node is added, not removed so it would be great to learn from your example!

Here is a test that deals with this block of code that you could copy -

https://github.com/patrick-steele-idem/morphdom/blob/master/test/browser/test.js#L1040

snewcomer commented 4 years ago

@chromakode There was in fact a bug! ref #194