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

Remove or update use of refs #405

Closed
levithomason opened this issue Aug 18, 2016 · 9 comments
Closed

Remove or update use of refs #405

levithomason opened this issue Aug 18, 2016 · 9 comments

Comments

@levithomason
Copy link
Member

levithomason commented Aug 18, 2016

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.

@chuan137
Copy link

chuan137 commented Sep 1, 2016

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

@levithomason
Copy link
Member Author

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.

@levithomason
Copy link
Member Author

levithomason commented Sep 20, 2016

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?

@levithomason
Copy link
Member Author

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

@saulobrito
Copy link

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.

@levithomason
Copy link
Member Author

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.

@saulobrito
Copy link

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

@levithomason
Copy link
Member Author

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.

@saulobrito
Copy link

Great. Thanks for your notes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants