cyclejs: DOMException: Failed to execute 'insertBefore' on 'Node': The node before which the new node is to be inserted is not a child of this node.

Here is a bug I’ve met with simple list and items selections:

Code to reproduce the issue:

import xs from 'xstream'
import { run } from '@cycle/run'
import { makeDOMDriver, div } from '@cycle/dom'

console.clear()

const Main = ({DOM}) => {
   const selectedIndex$ = DOM.select(`[index]`).events('click')
    .map(ev => parseInt((ev.currentTarget).getAttribute('index')))
    .startWith(0)

   const selectedDOM$ = xs.of(div(['Selected']))
  
  return {
    DOM: xs.combine(selectedIndex$, selectedDOM$)
      .map(([selectedIndex, selectedDOM]) => {
        const list = ['One', 'Two', 'Three']
        return div(
          list.map((item, index) => {
            const isSelected = index === selectedIndex
            return div({
              attrs: {index}
            }, [isSelected ? selectedDOM : item ])
          })
        )
      })
  }
}

run(Main, {
  DOM: makeDOMDriver('body')
})

Expected behavior:

When clicked the item (One, Two, or Three ) is replaced with Selected text.

Actual behavior:

After a few clicks:

DOMException: Failed to execute ‘insertBefore’ on ‘Node’: The node before which the new node is to be inserted is not a child of this node.

Versions of packages used:

Here is webpackbin: https://www.webpackbin.com/bins/-Kw01D616yUWznH1mbTA

About this issue

Original URL
State: closed
Created 7 years ago
Comments: 18 (13 by maintainers)

Commits related to this issue

chore(dom): add test for #713 — committed to cyclejs/cyclejs by jvanbruegge 6 years ago

Most upvoted comments

Thanks, I’ll have a deeper look during the week. But from first sight, there are ways to optimize it performance-wise (specially the try/catch statements). But I see the idea behind what you are doing. So it is about estimating the shallow copy costs. I mean I have to shallow copy always myself in my combinator library because of the bug anyways so I don’t expect it to be a big difference vs. the copying happening in the library. My conclusion is that snabbdom is tricky to extend and build upon as you have to guard against caveats (how would you know when you are reusing a vNode somewhere deep in the hierarchy), but basic use case as intended is perfectly fine.

On 9/4/18, Géraud Henrion notifications@github.com wrote:

Sure brucou, here it is :

https://gist.github.com/fuunnx/24dc7745e4d71111e4f83eb2de6255ab

You’ll have to use patch-package and work without typescript for now sorry. I’ll add the modified .js file instead of the diff file tomorrow when I’m at work.

It’s hackish but did work fine for a few months now 😄 . I already tried to optimise it by avoiding to shallow copy the node when it is not necessary 😃

– You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/cyclejs/cyclejs/issues/713#issuecomment-418201110

–

brucou on Sep 3, 2018

I was hoping Object.assign would be fast? It would be interesting to actually compare how speed is affected by the change to orient the decision. It is not clear that the case is rare, and how rare it actually is, given that we only see those who reported it complaining. I encountered that issue years ago and struggled hours before I decided to just clone vNode. I am happy to know that shallow copy is enough, I was deep copying. But I notified only a few months ago because somebody else did. So it is not easy to conclude about rarity because our observations are necessarily partial.

Anyways I guess the bottom line is whether to make snabbdom’s life easier or snabbdom’s users life easier… But yes, in any case, documentation and visibility of that documentation is key. If we leave it as is, the ‘no duplicated vNode’ becomes a contract or precondition to using the library, so it should be proeminently notified.

brucou on Sep 3, 2018

The shallow copy is too expensive for snabbdom, I know that it would help, but the case is rare an easy to fix in user code (by said shallow copy) so it’s fine as long as it is well documented

jvanbruegge on Sep 3, 2018

Hi @jvanbruegge, here are my two cents related to https://github.com/cyclejs/cyclejs/issues/713 :

First, I don’t know how much it is desirable to fix this bug for the community. However I encountered the same issue for user generated html that was converted to dynamic vtrees (something inspired by https://github.com/snabbdom/snabbdom-template). I managed to fix it in the scope of my customer’s needs :

I forked the snabbdom source code at the points of DOM node creation. Each DOM node creation takes care to shallow-copy the corresponding vnode and mutates the provided vtree BEFORE appending the .elm property so that the original vnode/vtree is untouched and reusable anywhere. This operation happens only once on the lifecycle of a DOM element, so it’s not really expensive.

I don’t have the code at hand right now (and it’s hacky at fuck ^^), but I hope you get the idea :

function createElm(siblings: VNode[], vnode_: VNode, insertedVnodeQueue: VNodeQueue): Node {
    let vnode = vnode_.elm
      ? Object.assign({}, vnode_) 
      : vnode_
    siblings[siblings.indexOf(vnode_)] = vnode
    // ...    
    // later :
    vnode.elm = elm
}

Of course I don’t understand fully the mechanics of snabbdom and my scope was very narrow. But I did not encounter any issue at this point.

Any opinion if it could work or would lead to some impredictable behaviour ? There are probably some downsides

Hope it can be usefull 😃

fuunnx on Sep 3, 2018

Passng not arrays are not supported by TS typings.

That bug happens when you click on elements that are located BEFORE selected.

wclr on Oct 12, 2017