Improve the useCombinedRefs hook by using a Proxy

[iansan5653]

Note: This was part of #2182, but I decided to pull it out into a separate PR to make reviewing easier.

The useCombinedRefs hook is intended to provide a single ref that cascades down to several other refs. This is useful when, for example, you have a forwarded ref (that could be an object or a function) and you want to also have a local ref available as well:

const Example = forwardRef(({}, forwardedRef) => {
  const combinedRef = useCombinedRefs(forwardedRef)

  return <div ref={combinedRef} />
}

When the combinedRef is updated, the forwardedRef is updated as well. There can even be multiple forwardedRefs that will all get updated.

The current implementation uses useLayoutEffect to synchronize all the refs on every render. This typically works well, but can have edge cases because refs can update without causing a re-render. This is particularly likely if the ref is being used to hold a value that is not an element or if the ref refers to some element that is a deep child (children could update without re-rendering the parent).

This PR changes the implementation to use a Proxy as the target ref instead, so that any time the target ref is updated, all dependent refs are updated immediately. No layout effect is required using this strategy.

We could have used a function as the target ref instead - this would have made change detection easy without use of a Proxy. But there are many use cases where the consumer also wants to get the current value of the ref using .current and that's not available with a function.

This change brings useCombinedRefs up to date with the latest code used in production in Projects. I also added the hook to hooks/index so that we can update that codebase to use the PRC version of the hook and remove some duplication.

[changeset-bot]

⚠️ No Changeset found

Latest commit: f91f682

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

[github-actions]

size-limit report 📦

Path	Size
dist/browser.esm.js	68.63 KB (+0.08% 🔺)
dist/browser.umd.js	69 KB (+0.07% 🔺)

[Lukeghenco]

This looks good to me (as someone testing the component that uses this right now on the PRs React team), but I don't have real permissions to approve here.

[colebemis]

Looks good to me 👍

It is possible to unit test this hook ensure that it works as we expect? Also, could you add a patch changeset to this PR?

[iansan5653]

@colebemis I can definitely add unit tests. How would I add a changeset?

[colebemis]

@iansan5653 You can click this link:

That will create a markdown file for you where you can describe your changes.

[mattcosta7]

Curious about whether we can basically remove this hook entirely (or just wrap the native react implemetation of it)

useImperativeHandle(ref, () => internalRef.current)


// used like
forwardRef((props, forwardedRef) => {
   const ref = useRef()
   useImperativeHandle(forwardedRef, () => ref.current)
   
   return <div ref={ref} />
})

This forwads the 'internal' ref to the potentially forwardedRef already - and is the native react impl of this (which means it's already included in the react bundle)

What an imperative handle does

https://github.com/facebook/react/blob/49f8254d6dec7c6de789116e20ffc5a9401aa725/packages/react-reconciler/src/ReactFiberHooks.old.js#L1769-L1860

While combined refs allows passing many refs, where a hook might be a little less ergonomic since it won't handle being called in a loop well, it gets some of this out of this library

we could use the callback to provide multiple refs together, which should limit the extra code we run for this if we need to

[iansan5653]

Curious about whether we can basically remove this hook entirely (or just wrap the native react implemetation of it)

Hmm, that actually might work. I'll bet that 9/10 or more uses of this hook would be resolved by that example - I think it's very rare to actually combine more than one ref.

[iansan5653]

Closing this in favor of #2204 - thanks @mattcosta7!