[Fiber] Update root children using appendChild/insertBefore/removeChild #8400
Conversation
sebmarkbage
force-pushed
the
fibercontainerchildren
branch
from
8dab603
to
ceffdd4
Compare
sebmarkbage
force-pushed
the
fibercontainerchildren
branch
from
ceffdd4
to
a1fadde
Compare
|
I looked through this but am not sure I understand it. Will try looking again later. |
sebmarkbage
force-pushed
the
fibercontainerchildren
branch
from
a1fadde
to
bdd6e96
Compare
| } else if (node.tag === Portal) { | ||
| // If this is a portal, then the parent is actually the portal itself. | ||
| // We need to keep track of which parent we're removing from. | ||
| // TODO: This uses a recursive call. We can get rid of that by mutating |
There was a problem hiding this comment.
Is this the right fix?
sebmarkbage#1
There was a problem hiding this comment.
Yea I think it is. Updated.
| // but at commit. Therefore we need to track insertions which the normal | ||
| // flow doesn't do during mount. This doesn't happen at the root because | ||
| // the root always starts with a "current" with a null child. | ||
| // TODO: Consider unifying this with how the root works. |
There was a problem hiding this comment.
How could this be unified?
There was a problem hiding this comment.
This only happens to work because HostContainer always end up with two Fibers but that seems unnecessary.
There was a problem hiding this comment.
So if we fix that, then this same logic will need to be applied for HostContainer too
sebmarkbage
force-pushed
the
fibercontainerchildren
branch
2 times, most recently
from
03850dc
to
292a82b
Compare
| @@ -230,6 +224,11 @@ module.exports = function<T, P, I, TI, C>( | |||
| return; | |||
| } | |||
| node = node.return; | |||
| if (node.tag === Portal) { | |||
There was a problem hiding this comment.
Need to be really careful when rebasing this onto master. It didn't give any merge conflicts but magically moved this into commitInsertion instead of unmountHostComponents when I rebased locally because I guess the code looks the same.
| // When we go into a portal, it becomes the parent to remove from. | ||
| // We will reassign it back when we pop the portal on the way up. | ||
| parent = node.stateNode.containerInfo; | ||
| node = node.child; |
There was a problem hiding this comment.
(Fails flow due to nullability.)
…ethods This removes updateContainer and instead uses the regular child mutation methods to insert into the root container and portals. Since we're no longer clearing out the container DOM in updateContainer we have to do that manually during initial mount. This now works on a document and one of the tests end up unmounting the body when you render into the document so I had to work around that bit since we don't yet properly support rendering into the document root.
The goal of this is to avoid passing an opaque data structure that needs
to be recursively searched by the host.
I considered having some helper for doing the recursion but I figured it
might be helpful to let the reconciler move this around. For example we
might want to create an instance in beginWork and add to it as we go.
This would let us avoid traversing the tree twice and would solve the IE11
perf issue.
So instead, we create the instance first then call appendChild. I could
just call the normal one but I figured that I would make a special one
just in case. For example if you wanted to perform commits on a separate
thread from creation. This turned out to be useful in ReactNoop where I
can avoid searching the array for an existing one since I know the child
isn't there already. (Although splitting placement into insertion/move
might be better.)
Finally, we need the ability to update an instance after all the children
have been insertion. Such as `<select value={...} />`. I called this
finalizeInitialChildren.
sebmarkbage
force-pushed
the
fibercontainerchildren
branch
from
292a82b
to
ea34204
Compare
|
I think this will help server render reviving. Especially if we create in beginWork since in that case we're always expected to have an instance as we traverse down, we can simply just pick one up from the DOM instead of creating. |
| return domElement; | ||
| }, | ||
|
|
||
| appendInitialChild(parentInstance : Instance, child : Instance | TextInstance) : void { |
There was a problem hiding this comment.
will this only be called on the initial mount? or also when new children get's appended on a already mounted node?
will appendChild or appendInitialChild handle: ["Foo"] -> ["Foo", "Bar"] ?
There was a problem hiding this comment.
This would be handled by appendChild if the parent is already in the DOM.
This removes updateContainer and instead uses the regular child mutation methods to insert into the root container and portals.
Since we're no longer clearing out the container DOM in updateContainer we have to do that manually during initial mount. This now works on a document and one of the tests end up unmounting the body when you render into the document so I had to work around that bit since we don't yet properly support rendering into the document root.