RFC: Callback Ref Cleanup

[KurtGokhan]

This RFC proposes adding a cleanup functionality to Callback Refs. See the issue this was discussed for more context.

View the formatted RFC

[KurtGokhan]

There is another alternative.

The problem with current callback refs is, null means that the element is unmounted. However, this may not always be the case. With useImperativeHandle, the element can set its own ref to null. This is demonstrated here: https://codesandbox.io/s/react-playground-forked-eyj3s?file=/index.js

So another ref type may be needed, a ref type which does not associate null with unmounted.

This ref type may have the signature (Typescript):

interface RegisterRef<T> {
  register: (ref: T) => void;
  unregister?: (ref: T) => void;
}

And this can be used like:

const onClick = () => console.log('clicked!');

const logClicks = {
  register: (node) => node.addEventListener('click', onClick);
  unregister: (node) => node.removeEventListener('click', onClick);
};

function MyComponent(props) {
  return <button ref={logClicks} />;
}

Just like MutableRef, which uses current to keep the reference of element, this ref type is also object typed. React should check if register property of the ref is a function to disambiguate between RegisterRef and MutableRef.

Although this solution is quite different than what is discussed in this RFC, I believe it solves the same problems.

[gaearon]

We're going to have a look at how often ref callbacks return functions today, and when that happens. This could give some sense of how disruptive this proposal could potentially be. https://github.com/facebook/react/pull/22313

We don't think passing making the node nullable makes sense for the case where you return a function. Our current preferred version is where if you return a function, you opt into the new behavior, so you're not gonna get called with null at all.

[butchler]

Even if callback refs don't currently return functions, there is another way that this proposal could potentially be disruptive: code that makes assumptions about how callback refs work. Of course, code that makes assumptions about how the React API works always risks breakage, but it can be useful in some cases.

For example, in my own code I have made a hook that combines multiple callback refs from different sources into a single callback ref to be passed to a single element. This can be useful if you have multiple hooks for different purposes that both return callback refs, but you want to use both of them on the same element. (I suppose this could be a separate proposal: somehow supporting passing multiple refs, callback or object, to a single element.)

Additionally, because React has to re-call the callback ref if the function value changes, in most cases it is better to use useCallback to define the callback ref function. If you need to use a hook to define callback refs anyway, then it's probably less disruptive to introduce the new behavior as a new hook without changing the behavior of ref itself. This could also be done in user space. I shared one way that this hook could be implemented in the issue discussion: https://github.com/facebook/react/issues/15176#issuecomment-512654704.

[gaearon]

https://github.com/facebook/react/pull/25686

[jacobp100]

How does this work with useImperativeHandle? There were probably cases where you might have conditionally returned null for this cleanup style thing

forwardRef((props, passedRef) => {
  const [isEditing, setIsEditing] = useState(false)
  useImperativeHandle(passedRef, () => {
    if (isEditing) {
      return { selectText: () => {}, etc }
    } else {
      return null
    }
  }, [isEditing])
})

Also - was there anything broken with this pattern before?

forwardRef((props, passedRef) => {
  const ourRef = useRef()
  useImperativeHandle(passedRef, () => ourRef.current, [])
  return <div ref={ourRef} />
})

This always felt like the easiest way to clone refs. Seems like it would no longer work now though

[KurtGokhan]

@jacobp100 As far as I understand, this change does not affect the behavior of your first code. null will still propagate to passedRef. However, if passedRef expects null to be set also when this component is detached, it must not return a cleanup function.

Your second code seems wrong though. Changes to ourRef won't propagate to passedRef. Not sure if this is intended. It works in this case because afaik the value of a div's ref never changes. This would propagate the changes though:

forwardRef((props, passedRef) => {
  const [ourRef, setOurRef] = useState(null)
  useImperativeHandle(passedRef, () => ourRef, [ourRef])
  return <MyCustomComponent ref={setOurRef} />
})