Rendered markup roots should be added to ReactMount node cache

[syranide]

ReactMount caches all children for each parent it visits, this avoids worst-case exponential cost. However, use of ReactDOMIDOperations for updates forces ReactMount to populate the cache with the previous children.

If a newly rendered node needs to be found, then "the children of the parent" of "the newly rendered root" has to be re-traversed as it does not exist in the node cache. Since it's guaranteed that all the siblings of newly rendered roots are cached and that we have a reference to the node, it's trivial to manually add it to the node cache which should avoid "double-traversals" entirely.

[claudiopro]

@syranide what would be a good test case to exercise this logic?

[sophiebits]

I am not sure I understand what you meant originally, but I think #4983 fixes this. In any event, this is not really a "good first bug".

[syranide]

@spicyj Scanning #4983 quickly it seems like it does yes, anyway the basic issue is/was:

<div>
  <u />
  <b key={random} ref={} />
</div>

Every time it re-renders and the DOM node for b is looked up, it would re-cache all the nodes of div because b could not be found in nodecache.

[sophiebits]

Oh, I see. I think this is slightly separate then. I'll probably get to fixing this soon though.

[syranide]

@spicyj FYI, IIRC this is not currently a very big issue as childIndex is used in some places to reach the actual node inside the parent node instead (rather than the actual node directly), but it probably will be when React gets around to dropping the wrapping spans and thus childIndex I would assume.

[claudiopro]

@spicyj I concur it's a tad beyond the beginner level 😅

[syranide]

@spicyj I'm thinking this is no longer relevant for the new renderer? Or is it still perhaps?

[sophiebits]

Yeah I got rid of all this! It should be a lot easier to reason about now.