patrick-steele-idem / morphdom

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

HTML comments breaks morph #70

Closed weslleyaraujo closed 8 years ago

weslleyaraujo commented 8 years ago

Hi all, at first great work!

I came across with something that seems to be a issue, consider the following:

var el = document.createElement('div');
el.className = 'foo';
el.innerHTML = 'bar';
document.body.appendChild(el); 

var updates = '<!-- some comment --> <div class="foo">bar</div>';
morphdom(el, updates);

the el variable behaves exactly as it should be:

expect(el.className).to.equal('foo'); // true

but in the dom what I actually got is this:

image

I am not sure if this is a bug or an expected behavior, any insights?

patrick-steele-idem commented 8 years ago

Hey @weslleyaraujo, thanks for the feedback!

This is the expected behavior, but morphdom should probably report an error to the user. What's happening behind the scenes is that morphdom is parsing the HTML provided as the second argument using createContextualFragment(html) and then it is picking only the first child as the target node: https://github.com/patrick-steele-idem/morphdom/blob/ecde8f4aa6afc5d37ed1490f6acb1c02b8d4452b/lib/index.js#L53

If the parsing of the contextual fragment results in multiple child nodes then that is really bad input for the current implementation of morphdom.

The fact that there is a comment node doesn't really matter in this case. While it might solve your issue to skip over the comment nodes, I am not sure that is the right solution.

Hopefully you can update your code accordingly? If you have any thoughts or concerns please share, but for now I am going to go ahead and close the issue. While we could throw an error if there are multiple child nodes, that could potentially be a breaking change and we would need to up the major version and I want to avoid doing that for now.

weslleyaraujo commented 8 years ago

Thanks for the insights!

I think that some kind of warning/error would be very useful in this case, the example above was just a test that I was doing to check... but back in a real world project, I took quite long to find out what was happening.

I think this might be something to add on the roadmap for the next major release,

thanks once again!

fehmi commented 6 months ago

he example above was just a test that I was doing to check... but back in a real world project, I took quite long to find out what was happening.

@weslleyaraujo @patrick-steele-idem Do you remember how to solve it? Same this is happening to me and I don't know how to solve it.