Semantic-Org/Semantic-UI-React

Remove or update use of refs

levithomason opened this issue · 9 comments

See update below:

I'd like to remove as many use of refs as possible, and those that are 100% necessary should be callbacks.

Some components use a ref where they can likely use findDOMNode(this) instead. Regarding nested components, we have reliable markup and classNames to use. We should prefer findDOMNode(this).querySelector('...') to find nodes nested within ourselves. We should be able to remove most, if not all, use of refs in this way.

Can you explain the downside of using ref? At least the syntax looks quite clean to me.

Facebook will likely deprecate ref='string' syntax in favor of the callback syntax.

Although string refs are not deprecated, they are considered legacy, and will likely be deprecated at some point in the future. Callback refs are preferred.

https://facebook.github.io/react/docs/more-about-refs.html#the-ref-string-attribute

However, this issue should actually be updated. As Dan Abramov noted in eslint-plugin-react, findDOMNode hopes to deprecated as well. Starting first with an eslint rule against it to help move the community.The replacement again is a ref callback.

In general, refs should be avoided. React is a virtual DOM environment, when using a ref, you break out of that virtual DOM into the real DOM. It makes testing more difficult and opens the door for issues. I'd like to remove as many use of refs as possible, and those that are 100% necessary should be callbacks.

More context, refs do not work with stateless functional components. Which, most of our components are functional components. React refs can only be used with classes. There are workarounds, such as passing the ref through a different prop name or wrapping the functional component in a class component. Though, I think neither of these are preferred patterns.

Finally, I'd like to share this point of view. When accessing a ref, you are accessing a regular DOM node. You've left React and the Virtual DOM. Odds are, you are doing so to manage focus or measure the size/position of the node in the DOM. That is because React does not yet have a model for focus nor size/position (though Sebastian Markbåge says they are thinking of ways to handle focus).

I'm starting to think it makes more sense to just use querySelector() to locate your node. My thought process on this is:

  • you have left React to do something React doesn't support
  • you are going to use regular DOM APIs to do this thing, not React (focus or measure)
  • you are not going to mutate the DOM (there's no React consequence)
  • you have no idea if a ref will return a node at all (functional components cannot use refs)

So, what justification is there to use React refs to locate the node at all for focus / measuring purposes?

Just checked and the limited refs we must have are now callback form.

What about the use case used as the example on React documentation?

https://facebook.github.io/react/docs/refs-and-the-dom.html

You could use the name of the input instead of the reference but seems legit to me.

It will not work for a stateless functional component (no refs) nor will it work for a composite component since the ref is a reference to the instance of the class itself, not a DOM node. From the outside, as a consumer of a 3rd party component, there is no mechanism for telling the difference between these cases nor a mechanism for dealing with the differences.

I see your point now. So, if you want to focus on a specific element after page load, you would do something like this?

componentDidMount() {
    let queryInput = ReactDOM.findDOMNode(document.querySelector('input[name=query]'));
    queryInput.focus();
}

?

Thanks

Well, findDOMNode is also on its way out in the long run, so it would just be:

componentDidMount() {
    const queryInput = document.querySelector('input["name=query"]')
    if (queryInput) queryInput.focus();
}

I should note, Dan Abramov still suggests the use of ref for this case. You can read all about the debate here:

jsx-eslint/eslint-plugin-react#678

All of the points I've raised here are raised in great detail by others there ^. They also give code examples and even example repos of these issues.

Great. Thanks for your notes.