Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Callback Ref Cleanup #205

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

RFC: Callback Ref Cleanup #205

wants to merge 3 commits into from

Conversation

KurtGokhan
Copy link

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

View the formatted RFC

}} />
```

To keep backward compatibility, Callback Refs should still be called with `null`
Copy link
Member

Choose a reason for hiding this comment

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

This seems like it would be confusing because you can’t rely on the instance being there and it’s not fully symmetric.

What are the alternative designs? Considering we want all the existing code to continue to work if it doesn’t return a function. But maybe if it returns a function then we can change the behavior? There’s also a question of what happens when sometimes a ref returns a function and sometimes it doesn’t. Can you explore these issues and different options in more detail?

Copy link
Author

Choose a reason for hiding this comment

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

It is possible to conditionally return a cleanup function from useEffect. It should also be possible for the Callback Ref.

image

The reason why we should not call cleanup when the ref is null:

Because a ref is usually null when the parent component is unmounting. It is not clear when the cleanup should execute in this case. That is why I thought we should explicitly disallow it. But there is no harm in allowing it too. If we want to keep things symmetrical, we can call the cleanup right after calling the callback with null. In this case, user can decide if they want a cleanup of null ref, like in this example:

image

Both examples here: https://codesandbox.io/s/react-playground-forked-ng6x6?file=/index.js

Copy link
Author

Choose a reason for hiding this comment

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

@gaearon I proposed another solution in the comment below.

@KurtGokhan
Copy link
Author

KurtGokhan commented Sep 8, 2021

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
Copy link
Member

gaearon commented Sep 14, 2021

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. facebook/react#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
Copy link

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: facebook/react#15176 (comment).

@gaearon
Copy link
Member

gaearon commented Nov 16, 2022

facebook/react#25686

@jacobp100
Copy link

jacobp100 commented Nov 17, 2022

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
Copy link
Author

KurtGokhan commented Nov 17, 2022

@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} />
})

jackpope added a commit to facebook/react that referenced this pull request Apr 22, 2024
Resources
- RFC: reactjs/rfcs#205
- Warning implemented in #22313
- Warning enabled in #23145
- Feature added in #25686

We have warned to prevent the old behavior since 18.0.0.

The new feature has been on in canary for a while but still triggering
the warning. This PR cleans up the warning for 19
github-actions bot pushed a commit to facebook/react that referenced this pull request Apr 22, 2024
Resources
- RFC: reactjs/rfcs#205
- Warning implemented in #22313
- Warning enabled in #23145
- Feature added in #25686

We have warned to prevent the old behavior since 18.0.0.

The new feature has been on in canary for a while but still triggering
the warning. This PR cleans up the warning for 19

DiffTrain build for commit db913d8.
github-actions bot pushed a commit to facebook/react that referenced this pull request Apr 22, 2024
Resources
- RFC: reactjs/rfcs#205
- Warning implemented in #22313
- Warning enabled in #23145
- Feature added in #25686

We have warned to prevent the old behavior since 18.0.0.

The new feature has been on in canary for a while but still triggering
the warning. This PR cleans up the warning for 19

DiffTrain build for [db913d8](db913d8)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants