WIP: Use node id as a hint of which node to compare to #61
Conversation
| var oldLength = oldNode.childNodes.length | ||
| var length = Math.max(oldLength, newLength) | ||
| var newIndex = 0 | ||
| for (var oldIndex = 0; oldIndex < oldChildren.length; oldIndex++) { |
There was a problem hiding this comment.
Given that oldChildren.length is constant, we can evade doing an extra lookup on every loop:
for (var oldIndex = 0, len = oldChildren.length; oldIndex < len; oldIndex++) {There was a problem hiding this comment.
lol, nope not this one - apparently we modify the length of oldChildren - the next one is actually static tho
There was a problem hiding this comment.
.lengthis often optimized in engines too
| oldNode.appendChild(retChildNode) | ||
| iNew-- | ||
| function findNewChild () { | ||
| for (; newIndex < newChildren.length; newIndex++) { |
There was a problem hiding this comment.
Here too, newChildren has a constant length
| }) | ||
|
|
||
| t.test('remove an element', function (t) { | ||
| var a = html`<ul><li id="a"></li><li id="b"></li><li id="c"></li></ul>` |
There was a problem hiding this comment.
Could you also multiline this example like the one above? Think it helps with readability. Thanks!
There was a problem hiding this comment.
I need to debug this one. If I introduce newlines, there will be TextNodes in between the li and it will not work anymore.
There was a problem hiding this comment.
use .childNodes instead of .children then as those ignore text nodes
There was a problem hiding this comment.
turns out .children ignores text nodes https://developer.mozilla.org/en-US/docs/Web/API/ParentNode/children
There was a problem hiding this comment.
oh, you're right .children is correct
|
Super neato patch! - really digging this 😁 Did some super quick testing, and it doesn't appear to be making much of a difference in Even though this patch is sound, I wonder how much it'll improve performance. I think where it'll truly shine, is eliminating a class of errors for stateful elements (cached or otherwise) to prevent their placeholder return value ( So I think if we're gonna bench, we should bench with fancy, cached nodes so we actually get a perf benefit from comparing two similar nodes ✨ |
| var oldLength = oldNode.childNodes.length | ||
| var length = Math.max(oldLength, newLength) | ||
| var newIndex = 0 | ||
| for (var oldIndex = 0; oldIndex < oldChildren.length; oldIndex++) { |
There was a problem hiding this comment.
.lengthis often optimized in engines too
| }) | ||
|
|
||
| t.test('remove an element', function (t) { | ||
| var a = html`<ul><li id="a"></li><li id="b"></li><li id="c"></li></ul>` |
There was a problem hiding this comment.
use .childNodes instead of .children then as those ignore text nodes
|
I'm giving this a try in a real world application now that hit exactly this bug, will report back here |
|
works for me! 🎆 |
|
I was pretty sure that the TextNodes still caused bugs. Are you sure this was good to merge? |
as far as i can tell the bug was only in the test code, right? |
|
Think it might trigger anywhere, wrote a patch for Bel so random whitespace no longer creates text nodes; think this should help (: edit: writing a patch for |
|
Ya, confirmed - that patch fixes our tests when made multiline (: |
|
Awesome 👍 |
|
And now also shama/yo-yoify#45 ✨ |
Hey,
this pull request tries to implement an algorithm for #8. It should perform well on removal and addition of elements in a list. However for reordering it will be worse, especially if the list of dom nodes is inverted. But I guess there is not much we can do in that case.
The theoretical optimal dom modification would be similar to a "Shortest Edit Script" and quite expensive to perform. This algorithm here has multiple nested loops, but has a linear time.
I haven't yet figured out how "mixed content" should be treated, e.g.