Add getRootNode to fragment instances #32682
Conversation
github-actions
bot
added
the
React Core Team
There was a problem hiding this comment.
You assume that you don't have to update the parent but at the same time you implement updateFragmentInstanceFiber to take an updated parent. Each time you do that, you have to traverse back. The really bad part is that this happens for every update inside the subtree - not just updates to this fragment itself. So for an update deep inside the tree it has also do a few back traversal for every parent Fragment fiber too. This isn't really an issue for just setting the Fiber but anything more gets costly. However, you don't really need to do this at all if you assume it won't update.
There is however one case where it updates. When it unmounts. Ideally you wouldn't keep using a ref after it has been unmounted but you can still have a handle on it. When you do that on a DOM node and the DOM node is disconnected then you get parent DOM node where it was disconnected.
If the Fragment unmounts but the HostParent doesn't unmount, then the remaining instance of the Fragment shouldn't have that as the Parent anymore. This becomes especially important when you implement dispatchEvent because that shouldn't dispatch the event if the Fragment is unmounted.
I think it would be better to find it during operations like we do for children.
This saves some memory and you don't have to do any work on mount or updates until you need it, nor when it unmounts.
That also solves the problem when it unmounts because you'd traverse the .return pointer back. If it's disconnected, it'll find a null. If you find a HostComponent then that HostComponent might also be disconnected. Like if the fragment and host component was both removed by a parent at the same time. However, then you do get the right semantics because getRootNode() will still get you the top most disconnected element.
The special case is if you hit null on the return path before you hit a HostComponent like if only the Fragment was deleted. In the DOM getRootNode() returns the Element itself in this case. So I think in this case where you reach null return Fiber you should return the FragmentInstance itself since that's the equivalent of the "Element itself".
|
Moving the traversal to avoid the extra work in update makes sense. Returning self in the case of a null host parent is simple enough. The other unmount case I'm still having trouble producing in a test.
The problem I'm running into is after removing the fragment and host parent, the |
| }); | ||
|
|
||
| // @gate enableFragmentRefs | ||
| it('returns the topmost disconnected element if the fragment and parent are unmounted', async () => { |
There was a problem hiding this comment.
The assertion here fails because the fragment's return is null after unmounting it with its parent
There was a problem hiding this comment.
Ok so this happens because we now have this aggressive pass to clean up the return pointers recursively of the whole unmounted tree.
This pass is unfortunate because it slows down unmounts but it's also observable semantics in this case.
It exists as a way to mitigate the cost when users cause memory leaks that point into React. This wouldn't be so much an issue if we changed the data structure to be split between Fiber tree nodes and an Instance because the return pointer would be on the Instance which then doesn't point back own into children so the leak is just proportional to the parent path.
Or if we just lived with people leaking. In either case we'd have the right semantics too.
However, the semantics that you have now is not bad neither. It's just unfortunate because if we changed the implementation details later this would change and we'd actually have to add cost to preserve the current semantics.
4 participants
This implements
getRootNode(options)on fragment instances as the equivalent of callinggetRootNodeon the fragment's parent host node.The parent host instance will also be used to proxy dispatchEvent in an upcoming PR.