Ehancement #2365 : Add markup root directly in ReactMount cache

[ThomasCrevoisier]

@syranide Could you check this ?
I wanted to add checking on node parameter because we need a HTML element, but I'm not sure if this is the best way of doing this.
Also, if this okay, I might write some test for ReactMount.cacheNodeById.

[syranide]

I know I pointed you in this direction, but it should actually be preferable to just use ReactMount.getID instead. PS. Also you can't use the id argument because it's for the old node, not the new.

[ThomasCrevoisier]

I struggled with this because I don't know what kind of object is newNode. So I don't know how to access to the id of the new node.

[syranide]

@ThomasCrvsr node is a DOM node. ReactMount.getID(node) reads the ID from the node and if it isn't in the cache, puts it in the cache, we don't need the return value. So it fulfills all the criteria for making sure the node is in the cache.

PS. Technically, I would like the component instance to be an argument to ReactDOMIDOperations.* instead and read the ID from that, but it's probably not something we should do now.

[syranide]

@spicyj Brought up a good point yesterday as well. DOMChildrenOperations 'INSERT_MARKUP' (source) is affected as well. But we probably don't want to require ReactMount from there... so we should probably return the rendered nodes from DOMChildrenOperations.processUpdates and ReactMount.getID them afterwards.

Perhaps @zpao @spicyj has some thoughts on this.

PS. Ultimately this will probably have to change when web workers arrive as DOMChildrenOperations is supposed to be async I believe? But then it should simply be a matter of using a callback or requiring ReactMount or similar anyway I would think.

[ThomasCrevoisier]

I added some changes : I return renderedMarkup in DOMChildrenOperations.processUpdates and in ReactDOMIDOperations.dangerouslyProcessChildrenUpdates I use ReactMount.getID on rendered nodes.

[syranide]

@ThomasCrvsr I think it looks good! Can you write a test that verifies that it works and doesn't regress? (Mock ReactMount.findComponentRoot and assert the number of times called should work I think...)

@sebmarkbage Do you have an issues with this PR?

[syranide]

One last thing :)
A comment briefly explaining that getID puts the node in the cache and that we do this to avoid holes in the cache (which can otherwise cause all siblings to be unnecessarily re-cached). It would be great info for anyone else stumbling upon this code.

[ThomasCrevoisier]

Okay, it's done !

[syranide]

Grammar is a little off, something like this perhaps:

// `getNode` populates ReactMount's node cache with all siblings, but the
// replaced node creates a hole. `getID` fills the hole with the new node.

[sebmarkbage]

Stale. I'm going to close this out. We can revisit but it is likely this path will have/get more drastic changes.