Add ref to Fragment (alternative) #32465
Conversation
github-actions
bot
added
the
React Core Team
jackpope
force-pushed
the
fragment-refs-alt
branch
from
fb008d4 to
87d09a8
Compare
jackpope
force-pushed
the
fragment-refs-alt
branch
from
3ae668f to
0f40078
Compare
jackpope
force-pushed
the
fragment-refs-alt
branch
from
0f40078 to
2c926ba
Compare
jackpope
force-pushed
the
fragment-refs-alt
branch
from
b0de151 to
0a2d423
Compare
| case Fragment: | ||
| if (enableFragmentRefs) { | ||
| if (flags & Ref) { | ||
| safelyAttachRef(finishedWork, finishedWork.return); |
There was a problem hiding this comment.
I expect to see two safelyAttachRef and two safelyDetachRef. They always come in pairs and is both for mount/unmount and hide/reappear.
There was a problem hiding this comment.
Added safelyDetachRef calls to commitDeletionEffectsOnFiber and disappearLayoutEffects
| case Fragment: | ||
| if (enableFragmentRefs) { | ||
| const instance: null | FragmentInstance = finishedWork.stateNode; | ||
| if (instance === null || instance._fragmentFiber !== finishedWork) { |
There was a problem hiding this comment.
instance._fragmentFiber !== finishedWork I don't think it'll always be this Fiber will it? It could be the alternate.
It should be the same Instance even if the alternate updates.
Also this field should be opaque to the react-reconciler. It's the Config that decides where this lives. E.g. like this, expando, private field, or WeakMap.
You shouldn't really need to access it from the inside of the react-reconciler anyway.
You can probably just assume that's its the same one and that nobody can modify it.
| @@ -318,6 +318,26 @@ export function cloneMutableTextInstance(textInstance) { | |||
| return textInstance; | |||
| } | |||
|
|
|||
| export type FragmentInstance = null | { | |||
| _fragmentFiber: Object, | |||
There was a problem hiding this comment.
The _fragmentFiber should be private to the config so this object form should not be necessary. It should be able to be just null.
In fact, ideally all Config types should be opaque. The places they're not is mainly because the Config itself is split into multiple files but it should really be a type internal to the package and then the exports in the Config file should always be opaque.
jackpope
force-pushed
the
fragment-refs-alt
branch
from
fd0ea51 to
bb9e7f0
Compare
| focus(): void, | ||
| }; | ||
|
|
||
| function FragmentInstancePseudoElement( |
There was a problem hiding this comment.
Nit: This is not really a "PseudoElement" in the DOM terminology. The reason ViewTransitionPseudoElement is called this is because it represents a reified/first-class version of a PseudoElement Instance as a DOM concept of an Element that's part of another Element.
This is similar but it's conceptually more like a real Element or just a Fragment.
So maybe FragmentInstance is the right name? or maybe just Fragment?
This matters for when you log these object in the console and the name of the instance shows as [Fragment].
There was a problem hiding this comment.
Yes just realized this as I started to read the view transition spec with the PsuedoElement concepts. Will rename to FragmentInstance and have the type be FragmentInstanceType
| newChild.key === null; | ||
| if (isUnkeyedTopLevelFragment) { | ||
| newChild.key === null && | ||
| (enableFragmentRefs ? newChild.props.ref === undefined : true); |
There was a problem hiding this comment.
This is subtle but this means that null and undefined are different and will reconcile differently. Which is technically a breaking change.
It also means that <div ref={maybeUndefined && ref}>{children}</div> will clear the state of the whole subtree when it switches.
If it was ref === null || ref === undefined then it would clear state even if you did <div ref={c ? ref : null}> which also doesn't seem right.
So I think what you have is probably the best compromise.
There was a problem hiding this comment.
This is specific to Fragment components, right? So your example of clearing state of the <div /> subtree would be limited to <Fragment ref={}> subtree?
I basically want to know here whether a ref prop was declared at all. But I don't think we can distinguish a maybeUndefined ref value.
There was a problem hiding this comment.
function Component({ children, header, c }) {
const ref = useRef();
return <Fragment ref={c ? ref : undefined}>
<h1>{header}</h1>
{children}
</Fragment>
}
This would clear the state of all children and header deeply any time c changes.
It could also be written as:
function Component({ children, header }) {
const ref = useRef();
if (header) {
return <Fragment ref={ref}>
<h1>{header}</h1>
{children}
</Fragment>
} else {
return <>
<h1>Default Header</h1>
{children}
</>
}
}
If header switches from existing to not existing this would clear the state of children if the ref exists but it would not if the ref doesn't exist.
It also shows up in situations like:
<div>
<Fragment ref={ref}>{...}</Fragment>
</div>
Or if it's a component that just returns children.
So not necessarily the return value of a Component.
| updateFragmentInstanceFiber(finishedWork, current.stateNode); | ||
| } | ||
| if (flags & Ref) { | ||
| safelyAttachRef(finishedWork, finishedWork.return); |
There was a problem hiding this comment.
I don't know where this pattern is coming from but this should be in reappearLayoutEffects, not here. Attaching refs happen in the layout effects which can happen either in commitLayoutEffects or reappearLayoutEffects.
There was a problem hiding this comment.
Question regarding reappearLayoutEffects, I see a bunch of // TODO: Check flags & Ref. What's the reason those aren't already checked? Does the flag not come through at this phase?
There was a problem hiding this comment.
I think it's not safe to check the flag because it would've already been reset at this point. The reappearing doesn't always happen on a tree that was just rendered so those can be old Fibers. I think it would have to be like a static flag. I think the TODO isn't quite right (which isn't unusual for TODOs).
| } | ||
|
|
||
| export function commitNewChildToFragmentInstance( | ||
| childElement: Element, |
There was a problem hiding this comment.
The type that this should reference is Instance because that's the type that's exposed to the react-reconciler. Technically it should be opaque and so this would be wrong if it was.
| while (parent !== null) { | ||
| if (isFragmentInstanceParent(parent)) { | ||
| const fragmentInstance: FragmentInstance = parent.stateNode; | ||
| commitNewChildToFragmentInstance(fiber.stateNode, fragmentInstance); |
There was a problem hiding this comment.
You expect this to be an Element but this Fiber can be either a HostText or HostComponent so it can also be a Text. Because you call commitFragmentInstanceInsertionEffects for both HostText and HostComponent.
Currently you don't have any methods that operate on Text Nodes so you could probably skip HostText or call another Config that is a noop for now. Like maybe React Native could focus HostText?
There was a problem hiding this comment.
For now I'll check against HostComponent specifically and then follow up with any additional work to enable text for React Native.
| @@ -298,6 +339,9 @@ function insertOrAppendPlacementNodeIntoContainer( | |||
| } else { | |||
| appendChildToContainer(parent, stateNode); | |||
| } | |||
| if (enableFragmentRefs) { | |||
| commitFragmentInstanceInsertionEffects(node); | |||
There was a problem hiding this comment.
This is called for both insertions AND moves which is not what I think you expected because you'd add duplicate event listeners any time something moves.
I think maybe it's currently ok to check if node.alternate === null to know if it's an initial insertion.
| @@ -214,6 +219,38 @@ function getHostParentFiber(fiber: Fiber): Fiber { | |||
| ); | |||
| } | |||
|
|
|||
| export function commitFragmentInstanceInsertionEffects(fiber: Fiber): void { | |||
| let parent = fiber.return; | |||
| while (parent !== null) { | |||
There was a problem hiding this comment.
I don't love this loop. If you add 1000 children in bunch of nested components, you'd have to backtrack the same number of levels 1000 times.
And this is for every insertion whether you use Fragment refs or not.
That's why getHostParentFiber is called earlier and passed into insertOrAppendPlacementNode. That way if one Component is mounted which has 1000 children, we only get the getHostParentFiber once.
There's two cases here. A) is that the Fragment is above the Placement or the same level as the Placement. B) is if the Fragment is below the Placement.
For A you could do the loop at the same time as getHostParentFiber and use the same loop. Since this is for every update whether you use Fragment refs or not, it really applies to every update anyway. Might as well unify. Then you can build up an array of parent FragmentInstances and pass that array down though the same way the parent argument is passed to insertOrAppendPlacementNodeIntoContainer. Otherwise pass null if there is none. So there's only an allocation if there are many matches. It's an unfortunate allocation but since there can be more than one match it kind of needs to be one but it would only be the case where the children of the Fragment directly changes. That way if you have 1000 children being inserted into one fragment, we only collect that parent fragment once.
If it's below the Placement itself then it really must be new itself and so it shouldn't even have a a FragmentInstance yet because that gets created in the layout phase. So we don't need to care about B.
There was a problem hiding this comment.
Unifying with getHostParentFiber makes sense to me, thanks for the explanation here.
commitFragmentInstanceInsertionEffects is also called from reappearLayoutEffects. I don't think we can avoid this loop at each HostComponent there, but let me know if you see a way there. If that turns out to slow down the reappear effects, maybe we could hold some state on the hidden HostComponent fibers to signal they were attached previously and should search for fragment instance parents again on reappear. Just to avoid running it on all HostComponent.
| } | ||
| case Fragment: { | ||
| if (enableFragmentRefs) { | ||
| if (flags & Ref) { |
There was a problem hiding this comment.
I think you can skip this check for now. Nothing else is doing it and I don't actually think it's safe to do it.
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.