[Fiber] Fix portal bugs #8532
[Fiber] Fix portal bugs #8532
Conversation
| @@ -400,6 +400,19 @@ describe('ReactDOMFiber', () => { | |||
|
|
|||
| it('should keep track of namespace across portals (medium)', () => { | |||
| assertNamespacesMatch( | |||
| <svg {...expectSVG}> | |||
| <image {...expectSVG} /> | |||
| {portal( | |||
There was a problem hiding this comment.
Heads up: I renamed this to usePortal
gaearon
force-pushed
the
annoying-portal-bugs
branch
from
7346920
to
865105b
Compare
When unmount a portal, we need to unmount *its* children from itself. This is similar to what we would do for a root if we allowed deleting roots.
A portal is not part of that host tree despite being a child.
gaearon
force-pushed
the
annoying-portal-bugs
branch
from
6268ac7
to
4596fc4
Compare
It is failing because portal bails out of update, seeing null in pendingProps. It is null because we set pendingProps to nextPortal.children, which is null in this test.
If pendingProps is null, we do a bailout in beginWork. This prevents unmounting of the existing child when the new child is null. We fix this by changing portal fiber's pendingProps to be the portal object itself instead of its children. This way, it is never null, and thus doesn't cause a false positive in the bailout condition.
| @@ -152,7 +152,7 @@ module.exports = function<T, P, I, TI, C, CX>( | |||
| // If we don't have a child, try the siblings instead. | |||
| continue siblings; | |||
| } | |||
| if (!node.child) { | |||
| if (!node.child || node.tag === HostPortal) { | |||
There was a problem hiding this comment.
This was non-obvious to me. Could use a comment.
| // Then detach refs and call componentWillUnmount() on the whole subtree. | ||
| if (current.tag === HostPortal) { | ||
| // When deleting a portal, there are no host nodes above it. | ||
| // It is a host parent itself. |
There was a problem hiding this comment.
There was a problem hiding this comment.
Because of the recursive case which gets us into commitDeletion().
There was a problem hiding this comment.
I could probably inline it there if we know for sure that portal can never be tagged with a deletion effect (can it not? I don’t understand that part well)
There was a problem hiding this comment.
The "can render nested portals" test definitely gets a commitDeletion() call from outside CommitWork with a portal. So inlining it is not enough.
Alternatively we could figure out how to avoid tagging portals themselves with Deletion, and tag their children instead. Not sure if there's any benefit to this?
There was a problem hiding this comment.
It can. <div>{c ? portal : null}</div> does.
But my question is why calling unmountHostComponents(parent, current); below doesn't just work? Since current will be the portal and that will be the first node and then unmountHostComponents will do exactly this.
There was a problem hiding this comment.
My theory is that we shouldn't need this extra tag check because we're going to need one inside unmountHostComponents anyway. We unnecessarily call getHostParent but that's not the common case.
There was a problem hiding this comment.
If the portal gets the unmount, then we should hit this https://github.com/gaearon/react/blob/c69be1c5f499e24dd08878df612321edc6a8135f/src/renderers/shared/fiber/ReactFiberCommitWork.js#L274 before we reach the <p />. So we override the parent to be the portal before we do that.
There was a problem hiding this comment.
Hmm you're right, I don't understand this.
Will think about it.
There was a problem hiding this comment.
Apparently something is being deleted twice in some loop.
There was a problem hiding this comment.
Hm. Well that seems to be the problem then.
| @@ -341,7 +341,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { | |||
| } else { | |||
| // Update | |||
| const existing = useFiber(current, priority); | |||
| existing.pendingProps = portal.children; | |||
| existing.pendingProps = portal; | |||
There was a problem hiding this comment.
An alternate solution could be portal.children || [].
This is also a problem with HostRoot so we should choose a consistent strategy.
However, I'm also considering using an effect tag to indicate the lack or existence of pendingProps instead.
unmountHostComponents() should have worked despite finding the wrong parent because it should have changed the parent when pushing and popping the portals.
This leads to a "Cannot find host parent" bug because commitDeletion() clears the return field. When we're inside the loop, we assign node.sibling.return to node.return but by this moment node.return has already been nulled. As a solution we inline code from commitDeletion() without the nulling. It still fails tests but for a different reason (unrelated bug).
We are currently already visiting them in commitUnmount() portal case since it's recursive. This condition avoids visting them twice.
It doesn't seem to influence existing tests but we have this protection in all other similar places. It protects against infinite loops.
This reverts commit ed9747d. I'll solve this by using an array in place of null instead.
This avoids a false positive bailout with pendingProps == null when portal is empty.
|
Ready for another review. |
|
I don't understand why Circle is failing. Something something coverage but tests are passing. |
Enable additional (failing) portal tests 8eb192b
Added new tests for portals. One of them is completely new, another one is the old one that used to loop forever but now just fails.
Fix portal unmounting 3ba8069
When we unmount a portal, we need to unmount its children from itself.
This is similar to what we would do for a root if we allowed deleting roots.
This fixes an error unmouting a portal.
Skip portals when looking for host siblings 4596fc4
A portal is not part of that host tree despite being a child.
This fixes an error inserting a portal sibling.
Add a failing test for portal child reconciliation 3f2129a
It is failing because portal bails out of update, seeing
nullinpendingProps.It is
nullbecause we setpendingPropstonextPortal.children, which isnullin this test.Fix the bug when switching to a null portal child ed9747d
If
pendingPropsisnull, we do a bailout inbeginWork.This prevents unmounting of the existing child when the new child is
null.We fix this by changing portal fiber's
pendingPropsto be the portal object itself instead of its children. This way, it is nevernull, and thus doesn't cause a false positive in the bailout condition.See more commits below after discussion.