[BUG?] Paths not being tracked properly

[trusktr]

When we try to add a Node to a parent Node a second time, we expect an error like this, as in

var logo = scene.addChild();
scene.addChild(logo);

, we expect an error like this:

Uncaught Error: Node is already mounted at: body/0

However, I've got some complex code that is removing and adding child nodes often, and at some point, when I use Node#addChild to add a Node to a parent Node, I'm getting this error from PathStore instead:

Uncaught Error: item already exists at path: [data-reactid=".0"][data-scene-id="1"]/0/97/0/0/0/0/0/1

The "item" in question happens to be a Transform who's path is being (re)added TransformSystem#registerTransformAtPath, based on the stack trace:

ncaught Error: item already exists at path: [data-reactid=".0"][data-scene-id="1"]/0/97/0/0/0/0/0/1
    at PathStore.insert (http://localhost:3000/client/lib/_famous/famous.entry.js?bab126b262ed37f8bcf79944013b074b89476f49:2117:16)
    at TransformSystem.registerTransformAtPath (http://localhost:3000/client/lib/_famous/famous.entry.js?bab126b262ed37f8bcf79944013b074b89476f49:240:21)
    at Node.mount (http://localhost:3000/client/lib/_famous/famous.entry.js?bab126b262ed37f8bcf79944013b074b89476f49:3503:26)
    at Dispatch.mount (http://localhost:3000/client/lib/_famous/famous.entry.js?bab126b262ed37f8bcf79944013b074b89476f49:1644:64)
    at Node.mount (http://localhost:3000/client/lib/_famous/famous.entry.js?bab126b262ed37f8bcf79944013b074b89476f49:3512:15)
    at Dispatch.mount (http://localhost:3000/client/lib/_famous/famous.entry.js?bab126b262ed37f8bcf79944013b074b89476f49:1644:64)
    at Node.mount (http://localhost:3000/client/lib/_famous/famous.entry.js?bab126b262ed37f8bcf79944013b074b89476f49:3512:15)
    at Dispatch.mount (http://localhost:3000/client/lib/_famous/famous.entry.js?bab126b262ed37f8bcf79944013b074b89476f49:1644:64)
    at Node.mount (http://localhost:3000/client/lib/_famous/famous.entry.js?bab126b262ed37f8bcf79944013b074b89476f49:3512:15)
    at Dispatch.mount (http://localhost:3000/client/lib/_famous/famous.entry.js?bab126b262ed37f8bcf79944013b074b89476f49:1644:64)

This leads me to believe that the path system might have a bug because I'm strictly using only Node#addChild/Node#removeChild. At some points between those calls I'm adding and removing Position and Rotation components.

[trusktr]

I tried reproducing with this simple example:

var scene = FamousEngine.createScene()
var logo = scene.addChild()
var node2 = scene.addChild()
scene.removeChild(logo)
node2.addChild(logo)
new Position(logo)
node2.removeChild(logo)
scene.addChild(logo)

but that works. In my case, I'm removing and adding hundreds of nodes at a time.

[trusktr]

Alright, so I did an experiment to double check that I'm not accidentally adding nodes to the same parent node (in fact, they shouldn't be on any node at all because I'm calling removeChild elsewhere first). I've got code like this:

                    if (viewNode === cardFamousNode.getParent()) {
                        console.log('viewNode is already parent...')
                        debugger
                    }
                    viewNode.addChild(cardFamousNode)

The console.log and debugger statements never fire, and the error happens on the line after the conditional check.

[michaelobriena]

Sorry, this is really hard to debug without the specific code. Not much we can do here.

[artichox]

Here is the situation in which this bug occurs for me:

class MyNode extends Node
 constructor: ()->
        super
        @.nextBtn = new NextBtnNode() #this class also extends Node
        @.addChild( @.nextBtn )
        @.moduleNodes = []
    #remove all of the current module nodes and insert the new modules  
setModules: (modules)->
    @.modules = modules
    for node in @.moduleNodes
       @.removeChild node
    for module in @.modules
       newNode = new ModuleNode(module)
       @.moduleNodes.push newNode
       @.addChild newNode

myNode = new MyNode()
myNode.setModules( [1, 2, 3] )
myNode.setModules( [4, 5, 6] ) #ERROR item already exists at path

On the last line, the node's TransformSystem tries to register a new node at the path of the current NextBtn node, which was never removed. If I remove all the nodes including the NextBtn and then add all of the nodes, I don't receive an error. Thus, the path to the NextBtn node needs to update itself to reflect that it has moved from being at index 1 among all the children (for example) to index 0.

[trusktr]

@artichox Is that code who's repo you don't mind sharing?

[trusktr]

I have this problem in another project now. My case is simple. I've got some code like this:

import Node from 'react-famous/Node'
import {SplashScreen, SignUpScreen, LoginScreen} from './screens'

// This must contain React components that extend from react-famous/Node
let loginlayouts = {
    SplashScreen, SignUpScreen, LoginScreen
}


class Login extends Node {
    render() {
        return (
            <Node _famousParent  = {this} id="rootNode">

                {React.createElement(this.data.loginLayout)}

            </Node>
        )
    }

    getMeteorData() {
        console.log('login root data changed.')
        return {
            loginLayout: loginlayouts[Session.get('loginlayout')]
        }
    }
}
reactMixin(Login.prototype, ReactMeteorData)

export default Login

When the value of this.data.loginLayout changes, the Login component is re-rendered (in React's sense of rendering). The Node instance in the return function creates a Famous Node behind the scenes, and that instance remains instantiated across React renders. What changes is the value of {React.createElement(this.data.loginLayout)}. The value of that is a component that looks like this:

import Node from 'react-famous/Node'
import Node from 'react-famous/dom-rederables/DOMElement'

export default
class SplashScreen extends Node {
    render() {
        return (
            <Node _famousParent={this} id="SplashRoot">
                <DOMElement>
                    <h1>...</h1>
                </DOMElement>
                <Node>
                    ...
                </Node>
            </Node>
        )
    }
}

When the value of {React.createElement(this.data.loginLayout)} in the Login component changes, that causes the (React) component to be unmounted, which behind the scene unmounts the Famous Node that it represents. Then, the new value of {React.createElement(this.data.loginLayout)} is another React component that, when mounted (in React's sense of mounting) goes a head and creates a new Famous Node and adds that with addChild to the Login component's Famous Node.

So, in my case, all that is happening (when the value of this.data.loginLayout changes) is a single node is removed (and all it's sub-nodes along with it) and a single node is added (and all it's sub-nodes along with it), and that's when I get the error.

Maybe the index in one of the loops in the path logic is off by one or something?

[trusktr]

@artichox I'm gonna look into fixing this today.

[artichox]

Great thank you @trusktr
Here is the repo
client/modules/base/ModulesView.coffee is the node class. setModules is called in client/Scene.coffee

Also, I was mistaken: the bug still persists if I remove all of the child nodes including the NextBtn

[trusktr]

@artichox I didn't get to solve this today. I found a temporary workaround to the problem in my case: I'm swapping views by positioning them off screen or on screen instead of adding/removing nodes. It's lame, but that'll do for now.

[talves]

@trusktr this should be one of the highest priorities to fix

[trusktr]

@talves @michaelobriena @alexanderGugel As mentioned on Slack in #engine, the current workaround is to manually remove child nodes before removing parent nodes, in post-order. I'm gonna test that out.

[artichox]

Manually removing all of the child nodes in the tree does prevent path collisions, but unfortunately the recycling of DOM elements will sometimes result in some of the node's content being retained. So when I make a new node, it occasionally will have the content of one of my previous nodes in its background. Pretty funky looking. Preemptively setting content to "" in the node constructor doesn't fix the issue. Anyone found a workaround for this?

[trusktr]

@artichox Maybe we can override a method in the dom renderer to not recycle anything, for now?