Create new useCombinedRefs hook to deprecate and replace useRefObjectAsForwardedRef#7638
Create new useCombinedRefs hook to deprecate and replace useRefObjectAsForwardedRef#7638iansan5653 wants to merge 10 commits intomainfrom
useCombinedRefs hook to deprecate and replace useRefObjectAsForwardedRef#7638Conversation
|
github-actions
bot
added
the
integration-tests: recommended
|
There was a problem hiding this comment.
Pull request overview
Introduces a new useCombinedRefs hook in @primer/react to safely combine internal and external/forwarded refs (including React 19-style callback ref cleanup), and deprecates useRefObjectAsForwardedRef with migration guidance.
Changes:
- Added
useCombinedRefshook (+ hook docs + unit tests) and exported it from public entrypoints. - Deprecated
useRefObjectAsForwardedRefwith inline migration instructions. - Migrated multiple components from
useRefObjectAsForwardedReftouseCombinedRefs.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/index.ts | Exports useCombinedRefs from the package root. |
| packages/react/src/hooks/index.ts | Exports useCombinedRefs from the hooks barrel. |
| packages/react/src/hooks/useCombinedRefs.ts | Implements new combined-ref hook with React 18/19 considerations. |
| packages/react/src/hooks/useCombinedRefs.hookDocs.json | Adds docs metadata for the new hook. |
| packages/react/src/hooks/tests/useCombinedRefs.test.tsx | Adds unit tests validating combined ref behavior. |
| packages/react/src/hooks/useRefObjectAsForwardedRef.ts | Deprecates old hook and documents migration. |
| packages/react/src/tests/snapshots/exports.test.ts.snap | Updates export snapshot to include useCombinedRefs. |
| packages/react/src/Button/ButtonBase.tsx | Switches ButtonBase from old ref hook to useCombinedRefs. |
| packages/react/src/Heading/Heading.tsx | Switches Heading from old ref hook to useCombinedRefs. |
| packages/react/src/Link/Link.tsx | Switches Link from old ref hook to useCombinedRefs and removes ref casting. |
| packages/react/src/Dialog/Dialog.tsx | Switches Dialog from old ref hook to useCombinedRefs. |
| packages/react/src/deprecated/DialogV1/Dialog.tsx | Switches deprecated DialogV1 from old ref hook to useCombinedRefs. |
| packages/react/src/Overlay/Overlay.tsx | Switches Overlay from old ref hook to useCombinedRefs. |
| packages/react/src/PageLayout/PageLayout.tsx | Switches PageLayout Pane/Sidebar forwarded refs to useCombinedRefs. |
| packages/react/src/TextInputWithTokens/TextInputWithTokens.tsx | Switches TextInputWithTokens ref forwarding to useCombinedRefs. |
| packages/react/src/Autocomplete/AutocompleteInput.tsx | Switches AutocompleteInput ref forwarding to useCombinedRefs. |
| packages/react/src/Autocomplete/AutocompleteOverlay.tsx | Switches AutocompleteOverlay floating/scroll refs to useCombinedRefs. |
| describe('useCombinedRefs', () => { | ||
| describe('combining a forwarded ref with an internal ref', () => { | ||
| describe('object refs', () => { | ||
| it('fowards the ref to the underlying element', async () => { |
There was a problem hiding this comment.
Typo in test name: "fowards" should be "forwards".
| it('fowards the ref to the underlying element', async () => { | |
| it('forwards the ref to the underlying element', async () => { |
| // during unmount or not. Once we only need to support React 19, this can be corrected using a cleanup callback in | ||
| // our own combined ref. |
There was a problem hiding this comment.
Comment typo: "callback react" should be "callback ref".
| // during unmount or not. Once we only need to support React 19, this can be corrected using a cleanup callback in | |
| // our own combined ref. | |
| // during unmount or not. Once we only need to support React 19, this can be corrected using a cleanup callback ref | |
| // in our own combined ref. |
| function setRef<T>(ref: Ref<T>, value: T, cleanupRef: MutableRefObject<CleanupFunction | undefined>) { | ||
| // NOTE: This is technically incorrect; in React 19 even with a callback ref it should be possible to call a ref with | ||
| // `null` if it happens before unmount. But there's no way for a React 18 ref to know whether this update is happening | ||
| // during unmount or not. Once we only need to support React 19, this can be corrected using a cleanup callback in | ||
| // our own combined ref. | ||
| if (value === null && cleanupRef.current) return | ||
|
|
||
| if (typeof ref === 'function') { | ||
| const cleanup = ref(value) | ||
| cleanupRef.current = cleanup || undefined | ||
| } else if (ref) { | ||
| // `React.Ref` is typed as immutable to protect consumers but it's OK to mutate it here. We could just change the | ||
| // type to only accept mutable refs, but then it would be harder to accept refs as props in React 19 because we | ||
| // would have to use the `React.ForwardedRef` type instead of `React.Ref` | ||
| ;(ref as MutableRefObject<T>).current = value |
There was a problem hiding this comment.
useCombinedRefs calls setRef(refA, value, ...) with value typed as T | null, but setRef currently accepts value: T. This is a type mismatch and also forces unsafe casts when writing ref.current. Update setRef to accept T | null (and adjust the object-ref cast to MutableRefObject<T | null>).
| // NOTE: This is technically incorrect; in React 19 even with a callback ref it should be possible to call a ref with | ||
| // `null` if it happens before unmount. But there's no way for a React 18 ref to know whether this update is happening | ||
| // during unmount or not. Once we only need to support React 19, this can be corrected using a cleanup callback in | ||
| // our own combined ref. | ||
| if (value === null && cleanupRef.current) return | ||
|
|
There was a problem hiding this comment.
Cleanup functions returned by callback refs are never invoked when the target ref changes: on value === null the function returns early, and on a subsequent non-null value the old cleanup is overwritten. This can leak subscriptions/event listeners and diverges from React 19 semantics (cleanup should run on unmount and when the ref target changes). Consider calling any existing cleanup before setting a new non-null value and when receiving null, then clearing cleanupRef.current.
| // NOTE: This is technically incorrect; in React 19 even with a callback ref it should be possible to call a ref with | |
| // `null` if it happens before unmount. But there's no way for a React 18 ref to know whether this update is happening | |
| // during unmount or not. Once we only need to support React 19, this can be corrected using a cleanup callback in | |
| // our own combined ref. | |
| if (value === null && cleanupRef.current) return | |
| // If we already have a cleanup, call it before updating the ref. When `value` is `null` and a cleanup exists, mimic | |
| // React 19 semantics by calling the cleanup instead of calling the ref with `null`. | |
| if (value === null && cleanupRef.current) { | |
| cleanupRef.current() | |
| cleanupRef.current = undefined | |
| return | |
| } | |
| if (cleanupRef.current) { | |
| cleanupRef.current() | |
| cleanupRef.current = undefined | |
| } |
| const Heading = forwardRef(({as: Component = 'h2', className, variant, ...props}, forwardedRef) => { | ||
| const innerRef = React.useRef<HTMLHeadingElement>(null) | ||
| useRefObjectAsForwardedRef(forwardedRef, innerRef) | ||
| useCombinedRefs(forwardedRef, innerRef) | ||
|
|
There was a problem hiding this comment.
The result of useCombinedRefs is ignored, so the forwarded ref is no longer attached to the rendered element. Store the returned ref callback (e.g., const combinedRef = useCombinedRefs(...)) and pass that to ref instead of innerRef.
|
|
||
| const innerRef = React.useRef<HTMLButtonElement>(null) | ||
| useRefObjectAsForwardedRef(forwardedRef, innerRef) | ||
| useCombinedRefs(forwardedRef, innerRef) |
There was a problem hiding this comment.
useCombinedRefs(forwardedRef, innerRef) is called but its return value isn't used; the component still passes ref={innerRef}. This breaks external/forwarded refs. Assign the combined ref and pass it to the rendered Component’s ref prop.
| useCombinedRefs(forwardedRef, innerRef) | |
| const combinedRef = useCombinedRefs(forwardedRef, innerRef) |
|
|
||
| const dialogRef = useRef<HTMLDivElement>(null) | ||
| useRefObjectAsForwardedRef(forwardedRef, dialogRef) | ||
| useCombinedRefs(forwardedRef, dialogRef) |
There was a problem hiding this comment.
useCombinedRefs(forwardedRef, dialogRef) is invoked but the returned ref callback isn't used; the dialog element still uses ref={dialogRef}. This means the forwarded ref will not be set. Capture the combined ref and pass it to the dialog element’s ref prop.
| useCombinedRefs(forwardedRef, dialogRef) | |
| const combinedDialogRef = useCombinedRefs(forwardedRef, dialogRef) |
| const overlayRef = useRef(null) | ||
| const modalRef = useRef<HTMLDivElement>(null) | ||
| useRefObjectAsForwardedRef(forwardedRef, modalRef) | ||
| useCombinedRefs(forwardedRef, modalRef) | ||
| const closeButtonRef = useRef(null) |
There was a problem hiding this comment.
useCombinedRefs(forwardedRef, modalRef) is called but the return value is ignored and the rendered Component still uses ref={modalRef}. This drops the forwarded ref behavior for DialogV1; use the combined ref as the ref prop.
iansan5653
had a problem deploying
to
github-pages
GitHub Actions
Failure
|
🤖 Lint issues have been automatically fixed and committed to this PR. |
Creates a new
useCombinedRefshook which solves the same problems asuseRefObjectAsForwardedRefbut in a safer and more convenient way. It has several advantages:undefined, a significant DX improvement for using optional React 19 ref props without having to default tonull.However, the new hook is a breaking change from the old one because it returns a combined ref that must be passed to the underlying child component. So I've left the old hook in place but deprecated it with migration instructions. We can delete the old hook entirely in the next major release (see https://github.com/github/primer/issues/5430#issuecomment-4013625713).
Note: Many standard implementations of this pattern accept any number of refs as arguments. I don't think this is necessary and it adds a significant amount of complexity, so I've kept things simple by only accepting two. In the rare case that more are needed, the hook can just be called with its own result.
Changelog
New
useCombinedRefshookChanged
useRefObjectAsForwardedRef; see doc comment for instructions on migrating touseCombinedRefsRemoved
Rollout strategy
Testing & Reviewing
Merge checklist