Skip to content

Replace useRefObjectAsForwardedRef with useMergedRefs internally#7638

Open
iansan5653 wants to merge 30 commits intomainfrom
create-use-combined-refs
Open

Replace useRefObjectAsForwardedRef with useMergedRefs internally#7638
iansan5653 wants to merge 30 commits intomainfrom
create-use-combined-refs

Conversation

@iansan5653
Copy link
Contributor

@iansan5653 iansan5653 commented Mar 6, 2026

After #7672 deprecates useRefObjectAsForwardedRef, we need to replace internal calls with the new approach, which has several advantages:

  1. Fixes the bug identified in Revert "perf(useRefObjectAsForwardedRef): add dependency array to useImperativeHandle" #7635 (comment) where callback refs would be called on every render, by using a callback ref itself as the trigger for updating the refs. This prevents possible infinite render loops where a callback ref is used to set state.
  2. Using a callback ref, only updates the refs when the target changes, gaining the perf improvement from perf(useRefObjectAsForwardedRef): add dependency array to useImperativeHandle #7553 without the bug that came with it.
  3. Doesn't care about the order of passed arguments; the current hook will silently fail to work if you switch the arguments.
  4. Can work with two callback refs, allowing for components to use callback refs internally while still accepting external refs.
  5. Is React 19 forward-compatible with callback ref cleanup functions while remaining backwards-compatible with React 18.
  6. Has better type definitions to reduce the need for casting.
  7. Accepts undefined, a significant DX improvement for using optional React 19 ref props without having to default to null.

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 still left the old hook in place (because it's public) but deprecated it so we can remove it in a future major release.

Internally, we can migrate everything over to the new approach now, which is what I've done in this PR.

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan
  • None; if selected, include a brief description as to why

Testing & Reviewing

Merge checklist

@changeset-bot
Copy link

changeset-bot bot commented Mar 6, 2026

🦋 Changeset detected

Latest commit: 7349459

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/react Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Mar 6, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Mar 6, 2026

⚠️ Action required

👋 Hi, this pull request contains changes to the source code that github/github-ui depends on. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the integration-tests: skipped manually label to skip these checks.

@iansan5653 iansan5653 marked this pull request as ready for review March 6, 2026 22:06
@iansan5653 iansan5653 requested a review from a team as a code owner March 6, 2026 22:07
@iansan5653 iansan5653 requested review from TylerJDev and Copilot March 6, 2026 22:07
@github-actions github-actions bot requested a deployment to storybook-preview-7638 March 6, 2026 22:10 Abandoned
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

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 useCombinedRefs hook (+ hook docs + unit tests) and exported it from public entrypoints.
  • Deprecated useRefObjectAsForwardedRef with inline migration instructions.
  • Migrated multiple components from useRefObjectAsForwardedRef to useCombinedRefs.

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.

@github-actions github-actions bot requested a deployment to storybook-preview-7638 March 6, 2026 22:16 Abandoned
@primer
Copy link
Contributor

primer bot commented Mar 6, 2026

🤖 Lint issues have been automatically fixed and committed to this PR.

@github-actions github-actions bot requested a deployment to storybook-preview-7638 March 6, 2026 22:22 Abandoned
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.

Copy link
Member

@francinelucca francinelucca left a comment

Choose a reason for hiding this comment

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

Maybe I'm understanding this incorrectly, but how is this different than useMergedRefs? 🤔

@iansan5653 iansan5653 changed the base branch from main to update-use-merged-refs March 17, 2026 16:46
@iansan5653 iansan5653 changed the title Create new useCombinedRefs hook to deprecate and replace useRefObjectAsForwardedRef Replace useRefObjectAsForwardedRef with useMergedRefs internally Mar 17, 2026
@github-actions github-actions bot requested a deployment to storybook-preview-7638 March 17, 2026 16:55 Abandoned
@iansan5653
Copy link
Contributor Author

iansan5653 commented Mar 17, 2026

Maybe I'm understanding this incorrectly, but how is this different than useMergedRefs? 🤔

Great callout! I rebuilt this PR on top of another one (#7672) to update useMergedRefs, replacing the useCombinedRefs name entirely.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

integration-tests: skipped manually Changes in this PR do not require an integration test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants