patrick-steele-idem / morphdom

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

Morphdom fails to discard all removed childnodes #92

Closed MelleB closed 7 years ago

MelleB commented 7 years ago

Hi,

I noticed that Morphdom fails to remove childnodes in some situations:

old

new

It only only seems to remove one of the elements without an id. I have a failing test in https://github.com/melleb/morphdom/tree/morphdom-issue-92 Specifically: https://github.com/MelleB/morphdom/blob/morphdom-issue-92/test/browser/test.js#L882-L921

The order of the children matters. If the childnode with the id is the first child everything works.

MelleB commented 7 years ago

Offtopic: It wasn't trivial for me where to put the test code, so I put it in a place of which I'm sure it needs to be moved to a different location. Maybe it's a good idea to extend the contribution guidelines?

patrick-steele-idem commented 7 years ago

I'll take a look at this. Thank you for the failing test.

Maybe it's a good idea to extend the contribution guidelines?

Agreed. I'll see if I can find some time to update the contributing docs to talk about running and adding tests and how to test in real browsers. Thanks for the feedback.

patrick-steele-idem commented 7 years ago

I'm surprised it took this long for the bug to be visible, but it only occurs if a keyed node needs to be removed and that keyed node has siblings. morphdom was incorrectly reading the next sibling of the old node after it had been removed from the DOM. A simple reordering of the lines fixed the problem.

New version published: morphdom@2.2.1

nrkn commented 7 years ago

I seem to be having the opposite problem - that it was correctly removing child nodes in 2.2.0 but now leaving some behind with 2.2.1 - I'm going to do some more testing and try to reduce it to a simple test case tomorrow, I'll let you know if it's a real problem or if I'm just an idiot who's made some simple mistake somewhere :)

nrkn commented 7 years ago

I'm doing something stupid - my min test case works correctly with both versions - as you were. Great package btw.