choojs / nanomorph

πŸš… - Hyper fast diffing algorithm for real DOM nodes
MIT License
726 stars 58 forks source link

Update skipping every other element #30

Closed rreusser closed 7 years ago

rreusser commented 7 years ago

It seems like nanomorph element iteration is skipping every other element somewhere. I (blindly) tried adding ids, but the result is unchanged.

Version: nanomorph@2.1.1 with bel@4.5.1

Expected behavior:

  1. Start with a root node with zero children.
  2. run nanomorph(new, old) where new tree has ten children
  3. tree now contains ten children

Observed behavior: DOM is updated, but only contains elements 0, 2, 4, 6, 8

Example: http://codepen.io/rsreusser/pen/KazMMa?editors=0010

Code to reproduce:

const makeHtml = (n) => bel`
  <ul>${
    new Array(n).fill(0).map((d, i) => bel`<li>${i}</li>`)
  }</ul> 
`;
const tree = makeHtml(0);
document.body.appendChild(tree);
nanomorph(makeHtml(10), tree);

Thoughts: Aside from a bug, the next most likely issue seems to me that I'm misusing the API. (right after that is that this is a very experimental library πŸ˜„ )

Random thought: This isn't that thing that happens when you mutate a list as you're iterating over it, is it? Some things solve that by letting each operation return an updated loop index. Just a random guess without proper debugging.

rreusser commented 7 years ago

Seems like maybe that's exactly what it is:

https://github.com/yoshuawuyts/nanomorph/blob/master/index.js#L58

  for (var i = 0; i < length; i++) {
    var newChildNode = newNode.childNodes[i]
    var oldChildNode = oldNode.childNodes[i]
    var retChildNode = walk(newChildNode, oldChildNode)
    if (!retChildNode) {
      if (oldChildNode) oldNode.removeChild(oldChildNode)
    } else if (!oldChildNode) {
      if (retChildNode) oldNode.appendChild(retChildNode)
    } else if (retChildNode !== oldChildNode) {
      oldNode.replaceChild(retChildNode, oldChildNode)
    }
  }

Doesn't one of the counters need to be updated conditionally based on the result of the operation?

rreusser commented 7 years ago

There's more territory to cover, I think, but #31 fixes this particular case for me.