Support MergedRef #246
Support MergedRef #246
Conversation
atootoonchianmeta
commented
Jul 1, 2025
atootoonchianmeta
commented
- Update data type of nodeRef in ALSurfaceProps support both object reference and callback reference. The later is used for mergedRef
- use nodeRef if the data type of nodeRef is object
facebook-github-bot
added
the
CLA Signed
| @@ -66,7 +66,7 @@ export type ALSurfaceProps = Readonly<{ | |||
| metadata?: ALMetadataEvent['metadata']; | |||
| uiEventMetadata?: EventMetadata, | |||
| capability?: ALSurfaceCapability, | |||
| nodeRef?: React.RefObject<HTMLElement | null | undefined>, | |||
| nodeRef?: React.RefObject<HTMLElement | null | undefined> | React.RefCallback<HTMLElement | null | undefined>, | |||
There was a problem hiding this comment.
I think you can just use React.Ref which is defined as the combination
| @@ -340,7 +340,8 @@ export function init(options: InitOptions): ALSurfaceHOC { | |||
| * we have the node value, we can accurately assign the attribute, and | |||
| * also use that for our mount/unmount event. | |||
| */ | |||
| const nodeRef = props.nodeRef ?? localRef; | |||
| const isPropsNodeRef = props.nodeRef != null && typeof props.nodeRef === "object"; | |||
There was a problem hiding this comment.
We still need to support the function one. What I think you need to do for callback case is to use the localRef bellow, but inside of the useEffect the props.nodeRef with the localRef.currentValue.
There was a problem hiding this comment.
Thanks for your feedback. I want to understand your comment correctly. We only use nodeRef to extract the element value, and the element is only available when the nodeRef is an object. Within the useEffect we can only check the type of nodeRef and return the localRef or nodeRef based on the type of nodeRef.
There was a problem hiding this comment.
Thanks for your feedback. I want to understand your comment correctly. We only use nodeRef to extract the element value, and the element is only available when the nodeRef is an object. Within the useEffect we can only check the type of nodeRef and return the localRef or nodeRef based on the type of nodeRef.
If we have a nodeRef is available in either case. The difference is how you can read it. For objects you read the .current value, but for function, it will be called.
So, I think you should switch to use a function ref for our own local usage and inside of it:
- use the passed node
- update the .current of passed node ref
- or call the ref that is passed
Take a look at how function refs work in react. You can also look at some of the 'mergeRef` functions in the internal code.
| @@ -13,7 +13,7 @@ import * as IReactElementVisitor from 'hyperion-react/src/IReactElementVisitor'; | |||
| import * as IReactFlowlet from "hyperion-react/src/IReactFlowlet"; | |||
| import * as IReactPropsExtension from "hyperion-react/src/IReactPropsExtension"; | |||
| import * as Types from "hyperion-util/src/Types"; | |||
| import type * as React from 'react'; | |||
| import * as React from 'react'; | |||
There was a problem hiding this comment.
Why did you remove the type from here? We should make sure that autologigng does not import react directly as much as possible. So far we have able to get around direct import of react
| if (typeof ref === 'function') { | ||
| // Function refs don't expose current; you can't get element here safely | ||
| // unless you track it yourself, so return null | ||
| return null; |
There was a problem hiding this comment.
This is still incorrect. If there is function used for ref, you would return null which will bypass all of the event emitting logic in the rest of the function that calls this function.
| @@ -259,7 +270,7 @@ export function init(options: InitOptions): ALSurfaceHOC { | |||
| const { surface: parentSurface, nonInteractiveSurface: parentNonInteractiveSurface } = surfaceCtx; | |||
|
|
|||
| let addSurfaceWrapper = props.nodeRef == null; | |||
| let localRef = ReactModule.useRef<Element>(); | |||
| const localRef = ReactModule.useRef<Element | null>(null); | |||
There was a problem hiding this comment.
Not sure making this null helps much.
Also, see the line above, if a ref is passed, then we won't add any wrapper, which means your merged ref function is never called!
I see this a core issue we need to think about how to solve! The goal of having refs is to avoid wrappers, but then a function ref will disable almost everything (i.e. no wrapper, and no events)
| }; | ||
|
|
||
| return ( | ||
| <SurfaceComp surface='nodeRefs'> |
There was a problem hiding this comment.
to really test this, you should pass a function ref to Surface and see if everything still works.
|
I spent a bunch of time on this, but couldn't figure out any mechanism to handle the ref function callbacks AND avoid adding our own wrapper. Therefore, passing a ref function is the same as not passing ref at all and there is not much we can do differently. So, it might be better leave the code as is to signal to users we are not using it. |
