Non referentially stable refs cause multiple updates

[yuchi]

When a React element receives a different ref on re-renderer the previous ref is updated to null and the new one is given the current value — regardless of changes in value.

I made a small CodeSandbox to verify this. In it you can also find a “solution”, which is to transform the API into an hook. This let us introduce useCallback to give the user a referentially stable ref.

This doesn’t solve the problem completely: if one of the refs changes all other refs receive a double (and useless) update too.

[gregberge]

I implemented a useMergeRefs that works in all cases, what do you think?

https://codesandbox.io/s/merge-refs-experiment-h5lgc

[yuchi]

Thanks for following up!

In your solution you use a Effect while refs are updated even before the execution of LayoutEffect, i suggest to move to useLayoutEffect.

But even with this correction in place you still have an issue, that refs are updated too late.
I made another CodeSandbox based on yours that shows the problem:

1. There are 2 new refs at the start of the component
2. standardRef is attached directly to a native element (ref={standardRef})
3. manualRef is attached by setting its .current during funcRef3’s execution
4. There’s a useLayoutEffect that expects to find both refs populated (after clicking once on the button)… but manualRef is attached after this effect runs.

[gregberge]

@yuchi OK, so we need to use useUniversalEffect to be compatible with SSR.

[yuchi]

I have no idea about SSR. Currently there’s no way to attach an effect or a callback of some sort before other layout effects are run (only the actual order of execution matters).

I’m seriously thinking this library should strike a balance between correctness and limitations, and an RFC should be made to cover this on the library itself.

[gregberge]

@yuchi we should take a decision to make it right. Could you submit a PR to fix problems?

[agriffis]

I don't think this needs effects at all. Doesn't this work to preserve the resulting callback ref?

/* eslint-disable react-hooks/exhaustive-deps */
import React from 'react'

export const useMergedRefs = refs =>
  React.useCallback(current => {
    refs.forEach(ref => {
      if (typeof ref === 'function') {
        ref(current)
      } else if (ref && !Object.isFrozen(ref)) {
        ref.current = current
      }
    })
  }, refs)

The reason for disabling react-hooks/exhaustive-deps is that we're passing the raw refs array to useCallback, which means that eslint can't statically check it. But it works fine for the actual checking which preserves and updates the callback.

Also, the Object.isFrozen test shows up for me in tests. I'm not sure under what conditions the ref becomes a frozen, empty object, but we just need to avoid assigning in that case.

[theKashey]

There are a lot of different versions of this pattern. Almost every project came with something similar:

• https://react-hooks.org/docs/use-fork-ref
• https://github.com/mitchellhamilton/apply-ref
• https://github.com/theKashey/use-callback-ref
• https://github.com/mui-org/material-ui/blob/9bd4277ecd660ebe2fd4cb08960f58e98ddb6d43/packages/material-ui/src/utils/useForkRef.js

And (strangely!) many are suffering from this issue.

Imho the current API should be just deprecated and hook(useMergedRefs) based version released instead of it.

[oliviertassinari]

Regarding the version in Material-UI. I believe we handle it correctly: https://codesandbox.io/s/merge-refs-experiment-forked-zcokt?file=/src/App.js.

import { unstable_useForkRef as useForkRef } from "@material-ui/utils";

We have faced this issue when rewriting all the components to be hook based. @yuchi would you confirm?

[theKashey]

@oliviertassinari - I can confirm that your realization is stable.

But we can get another problem in the future as the Strict and Concurrent modes are not very any ref tricks friendly.

[yuchi]

MUI's version is stable, but only in a single specific sense, and formally fragile. Let me explain.

A perfect ref merger should:

1. Be stable against re-renders, if no input ref has changed the resulting ref must be the same as the previous one, so that it doesn't uselessly trigger an update.
2. Be transparent, from the outside, from the standpoint of a single ref creator, there should be no difference between using the ref as-is and merged with other refs.

MUI's current implementation has the following problems:

1. It uses useMemo which as we all know as historically been defined as an optimization tool, and should not be used to have "singleton" semantics. In this case avoiding recreating a ref is some kind of an optimization trick, but since ref updating is in the imperative world (a function ref can have side effects ^{[citation needed]}), we need more guarantees.
2. When one of the ref changes a new merged ref is created and all refs gets updated, again ruining the expectation about when and how function refs are called.

[yuchi]

Again, I feel like this cannot be solved in user space.

[agriffis]

You could add a WeakMap to avoid calling any refs with the same current repeatedly:

/* eslint-disable react-hooks/exhaustive-deps */
import {useCallback, useRef} from 'react'

export const useMergedRefs = refs => {
  const weak = useRef(new WeakMap()).current
  return useCallback(current => {
    refs.forEach(ref => {
      if (!weak.has(ref) || weak.get(ref) !== current) {
        if (typeof ref === 'function') {
          ref(current)
        } else if (ref && !Object.isFrozen(ref)) {
          ref.current = current
        }
        weak.set(ref, current)
      }
    })
  }, refs)
}

[yuchi]

@agriffis I was thinking about it too, but I’m still trying to come up with a case where a ref is called again with the same value. If we find that case than this approach cannot work. Also, your implementation has a bug, when refs change the merged ref changes, so the previous merged ref is called with null by React, therefore setting al refs to null in the WeakMap…