[Fiber] DOM reviving through DOM walking seems blocked by the ReactFiberReconciler API

[aickin]

After a discussion with @sebmarkbage about how the Fiber client renderer should revive a server-rendered DOM, I started to dip my toe into seeing how hard it would be to implement in ReactDOMFiber.

My first thought for implementation was to use createInstance to crawl the existing DOM element-by-element, comparing the existing DOM elements to the createInstance arguments. That would look something like this (some parts having been simplified):

createInstance(type, props) {
 // TODO: the actual check to make sure the markup is correct is probably a little
 // more complicated than the conditional here.
 if (currentDOMElement && 
   currentDOMElement.type === type && 
   compareProps(currentDOMElement, props) {
  // the current DOM element in the document should be reused.
  return currentDOMElement;
 } else {
  // TODO: warn about markup mismatch.
  const newElement = <call the existing implementation of createInstance() in ReactDomFiber.js>;
  newElement[elementInDocumentToReplace] = currentDOMElement;
  return newElement;
 }
}

appendChild(parent, child) {
 if(child[elementInDocumentToReplace]) {
  // we need to patch up the DOM tree because there was a markup mismatch.
  parent.replaceChild(child, child[elementInDocumentToReplace]);
 } else if (child.parentNode === null) {
  // either there was no SSR markup tree or this is a descendant of a node that
  // needed to be patched up.
  parent.appendChild(child);
 } else {
  // the instance is already correctly in the document from SSR, so
  // this call to appendChild is a no-op.
 }
}

This all seems fairly straightforward to me, with one big problem: I can't figure out any good way to figure out what currentDOMElement is. Unfortunately, createInstance doesn't take in an argument identifying its parent instance; as far as I can tell, there's no way to determine where in the tree you are during a call to createInstance.

I looked into using hostContext, which is currently used in ReactDOMFiber to pass down the type of ancestors in the tree. I'm fairly certain that hostContext isn't good enough to get access to the current DOM node, though, because the method that generates the host context for a particular node, getChildHostContext, doesn't receive the parent instance, only the parent's host context and type.

There is one truly silly way to solve this with the current API, which is to defer creation of DOM instances to appendChild, when you know where in the tree the instance is supposed to be. In this solution, createInstance would return the element metadata (type & props) as an Instance instead of a real DOM element, and you'd make the decision whether to use existing markup or make a new DOM element in appendChild, when you have all the information you need about where you are in the tree. This strikes me as a pretty bad solution, though, since the whole point of the two calls (at least as I understand it) is to do createInstance calls over multiple frames and all the appendChild calls synchronously in one frame. This idea would put all the work into the appendChild frame, which would kind of defeat the point of Fiber.

So: my knowledge of the Fiber codebase is minimal at best, and I could be totally wrong about all this, but I think that createInstance's signature may need to change to support DOM revival on the client. If createInstance took in the parent instance and closest preceding sibling instance as arguments, this problem would be solved, but I'm sure there are other solutions, too. Thoughts?

[acdlite]

I assume you mean createInstance rather than createElement

[acdlite]

I think this API would require us to add additional fields to the Fiber type, which isn't ideal, although we've seen this problem come up in other contexts before: during the commit phase, when we insert a DOM node, we have to traverse the parents to find the nearest DOM ancestor. Same with siblings. If we solve one, we can probably solve both.

[acdlite]

The render/commit phase distinction seems like the harder bit

[aickin]

I assume you mean createInstance rather than createElement

Argh, you're right; fixed in the original post. Thanks!