Add ref to Fragment (alternative) #32465
Conversation
github-actions
bot
added
the
React Core Team
|
Comparing: 22e39ea...d30251d Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show |
jackpope
force-pushed
the
fragment-refs-alt
branch
from
fb008d4 to
87d09a8
Compare
| @@ -3122,6 +3145,7 @@ function commitReconciliationEffects( | |||
| const flags = finishedWork.flags; | |||
| if (flags & Placement) { | |||
| commitHostPlacement(finishedWork); | |||
| commitFragmentInstanceInsertionEffects(finishedWork); | |||
There was a problem hiding this comment.
You assume that finishedWork is a HostComponent here but it might not be.
E.g. <Fragment ref={...}>{c ? <Component><div /></Component> : null}</Fragment>.
In this case it would be the <Component> fiber that's being placed.
Instead, you should do this in insertOrAppendPlacementNode and insertOrAppendPlacementNodeIntoContainer. Those are recursively traversing down to find all the HostComponents inside the placement.
There was a problem hiding this comment.
One reason I put it here is because commitHostPlacement is gated by !supportsMutation. We eventually want this all to work in Fabric. Though there are probably additional considerations to make with that, I haven't taken a closer look at the RN implementation yet.
| function getFragmentInstanceChildren( | ||
| fragmentInstance: FragmentInstance, | ||
| ): Set<Element> { | ||
| const children = new Set<Element>(); |
There was a problem hiding this comment.
This creates a new allocation and a pretty heavy one. You don't need a Set here because you should know whether it's a duplicate or not just by the algorithm. No need to run expensive hash map code to dedupe.
Even with an array it's an unnecessary allocation and build up. Every time you perform an operation. It would be better to just do the loop inline inside the function that performs the operation. It's a little annoying to duplicate that code but it's not that much code and we have that loop duplicated so many times anyway.
Maybe it could be a helper that takes a static function without allocating any closures like:
traverseFragmentInstanceChildren(fn, arg) {
...
fn(instance, arg);
}
One possible benefit of the array is that you could stash it on the instance as a cache and then reset it any time you get an insert/deletion. That way if you have multiple addEventListeners in a sequence you don't have to traverse. However, it might not actually be worth this tradeoff because it still requires allocation and extra memory storage for something that is likely a very shallow traversal anyway and not slow to begin with.
There was a problem hiding this comment.
I updated to use a helper, though its a bit awkward with multiple args so I'm binding them. I believe that should avoid a lot of the perf hit of a closure allocation. Other traversals such as focusLast will need some refactoring or their own traversal loop, so maybe it will just end up with duplication afterall.
Also not sure about the tradeoff with caching the children. Seems unlikely there would be a large amount of listeners or observers on a single fragment instance. But something we can keep an eye on in experimental usage.
| this: FragmentInstance, | ||
| fragmentFiber: Fiber, | ||
| ) { | ||
| this._fragmentFiber = fragmentFiber; |
There was a problem hiding this comment.
You set this only once but that will be the initial Fiber which means that any time you traverse it, you'll observe the initial state of the tree. Because of alternate model you have a 50/50 chance of getting it right but you might actually get the alternate and not current here.
Instead, you need to update this to the new Fiber any time a new one commits. Basically in commitMutationEffectsOnFiber on the Fragment branch you update it.
Technically you only need to update it if there are Placement/Deletion in the subtree but that would hold on to an old Fiber for longer and would be confusing. So you might as well always update it.
Note that this is what we do for HostComponents too but only for props, but we should probably just update the Fiber instead.
There was a problem hiding this comment.
Thanks for the catch! This definitely revealed bugs once I logged the current state of the fiber tree after Placement/Deletion. I just didn't have a test yet that asserted on anything requiring the traversal of a newly added node. I've updated to set current's reference to finishedWork in commitMutationEffectsOnFiber
| childElements.add(child.stateNode); | ||
| } | ||
|
|
||
| if (child.sibling !== null) { |
There was a problem hiding this comment.
It's typically better to do this with a loop since otherwise the recursion can get very deep and lead to stack overflow.
Since React does use a stack we do require a tree to be more shallow than required for a typical recursion but we don't require it to also handle sibling depth. You can have long list of items in deep trees.
There was a problem hiding this comment.
This also leads to a sort of breadth first search which I don't think is necessarily the intended order of insertion.
<Fragment>
<Component><div id="a" /></Component>
<div id="b" />
</Fragment>
This should have "a" first and then "b" but the current algorithm would visit "b" first.
Look at how we do other traversals where we do siblings in a loop and then recurse each child in that loop. Depth first.
There was a problem hiding this comment.
Updated to be depth first and loop the siblings.
| return; | ||
| } | ||
|
|
||
| if (child.tag === HostComponent) { |
There was a problem hiding this comment.
This probably need to consider some other child tags as well. For example inside an <Activity> that's currently hidden we shouldn't discover any elements. (If we did then they would be duplicate when it gets revealed since they're added again.) So we need to bail out from searching inside Activity here.
There was a problem hiding this comment.
Added a boundary based on OffscreenComponent. Another that could be relevant is HostPortal, though everything in portals like events and context follow the React tree, so it seems like for consistency we should include any host nodes within portals here as well.
This API is experimental and subject to change or removal.
This PR is an alternative to #32421 based on feedback: #32421 (review) . The difference here is that we traverse from the Fragment's fiber at operation time instead of keeping a set of children on the
FragmentInstance. We still need to handle newly added or removed child nodes to apply event listeners and observers, so we treat those updates as effects.Fragment Refs
This PR extends React's Fragment component to accept a
refprop. The Fragment's ref will attach to a custom host instance, which will provide an Element-like API for working with the Fragment's host parent and host children.Here I've implemented
addEventListener,removeEventListener, andfocusto get started but we'll be iterating on this by adding additional APIs in future PRs. This sets up the mechanism to attach refs and perform operations on children. The FragmentInstance is implemented inreact-domhere but is planned for Fabric as well.The API works by targeting the first level of host children and proxying Element-like APIs to allow developers to manage groups of elements or elements that cannot be easily accessed such as from a third-party library or deep in a tree of Functional Component wrappers.
In this case, calling
fragmentRef.current.addEventListener()would apply an event listener toA,B, andD.Cis skipped because it is nested under the first level of Host Component. If another Host Component was appended as a sibling toA,B, orD, the event listener would be applied to that element as well and any other APIs would also affect the newly added child.This is an implementation of the basic feature as a starting point for feedback and further iteration.