Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 12 additions & 27 deletions packages/@react-aria/utils/src/useObjectRef.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,7 @@
* governing permissions and limitations under the License.
*/

import {MutableRefObject, useRef} from 'react';
import {useLayoutEffect} from './';
import {MutableRefObject, useMemo, useRef} from 'react';

/**
* Offers an object ref for a given callback ref or an object ref. Especially
Expand All @@ -24,31 +23,17 @@ import {useLayoutEffect} from './';
*/
export function useObjectRef<T>(forwardedRef?: ((instance: T | null) => void) | MutableRefObject<T | null> | null): MutableRefObject<T> {
const objRef = useRef<T>();

/**
* We're using `useLayoutEffect` here instead of `useEffect` because we want
* to make sure that the `ref` value is up to date before other places in the
* the execution cycle try to read it.
*/
useLayoutEffect(() => {
if (!forwardedRef) {
return;
}

if (typeof forwardedRef === 'function') {
forwardedRef(objRef.current);
} else {
forwardedRef.current = objRef.current;
}

return () => {
return useMemo(() => ({
get current() {
return objRef.current;
},
set current(value) {
objRef.current = value;
if (typeof forwardedRef === 'function') {
forwardedRef(null);
} else {
forwardedRef.current = null;
forwardedRef(value);
} else if (forwardedRef) {
forwardedRef.current = value;
}
};
}, [forwardedRef]);

return objRef;
}
}), [forwardedRef]);
}
11 changes: 11 additions & 0 deletions packages/@react-aria/utils/test/useObjectRef.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,17 @@ describe('useObjectRef', () => {
expect(inputElem.placeholder).toBe('Foo');
});

it('should only be called once', () => {
const TextField = React.forwardRef((props, forwardedRef) => {
const ref = useObjectRef(forwardedRef);
return <input {...props} ref={ref} />;
});

let ref = jest.fn();
render(<TextField ref={ref} />);
expect(ref).toHaveBeenCalledTimes(1);
});

/**
* This describe would completely fail if `useObjectRef` did not account
* for order of execution and rendering, especially when other components
Expand Down
2 changes: 1 addition & 1 deletion packages/react-aria-components/src/utils.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ export function useContextProps<T, U, E extends Element>(props: T & SlotProps, r
}
// @ts-ignore - TS says "Type 'unique symbol' cannot be used as an index type." but not sure why.
let {ref: contextRef, [slotCallbackSymbol]: callback, ...contextProps} = ctx;
let mergedRef = useObjectRef(mergeRefs(ref, contextRef));
let mergedRef = useObjectRef(useMemo(() => mergeRefs(ref, contextRef), [ref, contextRef]));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if merge refs should always be memo'd, it looks like there are a few other instances in our react-aria-components as well as datepicker and link where we do not do this

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah ideally. We'd need to make useMergeRefs so it can be a hook

let mergedProps = mergeProps(contextProps, props) as unknown as T;

// A parent component might need the props from a child, so call slot callback if needed.
Expand Down