feat(Ref): support of forwardRef() API

[layershifter]

This PR is a next stage for #417.

---

findDOMNode() is deprecated in StrictMode, ref. and we can't rely on it, it also forces us to use react-dom that means that it will be impossible to use it with React Native.

This PR is redesign of Ref component to use forwardRef API by default and use findDOMNode() only as a fallback. If React's team will decide to remove findDOMNode() it will be not issue for us.

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@b355abd). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master     #491   +/-   ##
=========================================
  Coverage          ?   88.28%           
=========================================
  Files             ?       42           
  Lines             ?     1468           
  Branches          ?      190           
=========================================
  Hits              ?     1296           
  Misses            ?      167           
  Partials          ?        5

Impacted Files	Coverage Δ
src/components/Ref/Ref.tsx	100% <100%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b355abd...921f35a. Read the comment docs.

[layershifter]

The usage of generic in docs :( It's impossible to omit it there.

[kuzhelov]

yes, and this is definitely not the problem of this PR :) seems that we should use JS engine to process this code, as docs are aimed to present JS variant of examples to the client (this is the case for now, at least)

[layershifter]

I'm planning to left this thing as is and push #495 that will resolve all our issues.

[layershifter]

(!) A new dependency.

[kuzhelov]

👍

[layershifter]

We can separate this logic obviously to two subcomponents RefForwardRef and RefFindDOMNode, it will be very clean. But it will add one more level of nesting to React's tree.

/cc @miroslavstastny

[kuzhelov]

according to ref's docs: https://reactjs.org/docs/refs-and-the-dom.html

The function receives the React component instance or HTML DOM element as its argument, which can be stored and accessed elsewhere.

The parameter type used in this function leads to false assumptions.

[layershifter]

See my comment below, we will enforce user to pass correct refs.

[kuzhelov]

actually, very confused by trying to understand how Ref component is helpful in case of ref forwarding (and this, as stated in the description, is its main purpose: This PR is redesign of Ref component to use forwardRef API by default and use findDOMNode() only as a fallback.)

Let me explain resaons for this by following the example. Suppose that I am a Stardust client and have the following component:

const MyButton = React.forwardRef((props, ref) => (
  <div>
    <button {...props} ref={ref} />
  </div>
))

With this component I already expect the following to be true:

// when mounted, this.ref.current will refer to <button />
<MyButton ref={this.ref} />

Then comes Stardust and proposes to use Ref - with no clear benefits to me, as now it will be either:

• I will use Ref as another wrapper - but what are the benefits of that :/ ?

const WithRef = (props) => (
  <Ref innerRef={props.forwardedRef}>
    { React.forwardRef((props, ref) => <MyButton ref={ref} />
  </Ref>
)

• or what is the scenario where I might need it?

---

Ref, in case of ref forwarding case, acts as a simple ref translator - so, what are the reasons I wouldn't use built-in React ref forwarding capabilities and would prefer Ref component abstraction instead?

[kuzhelov]

Have expressed points of general concerns/questions related to the approach.

Also, to me it seems wrong that we are exposing different semantics for innerRef for these cases:

• forward ref case

// element can be React Component reference (this) or DOM element
<Ref innerRef={ref => .. }>
  ...
</Ref>

• findDOMNode() case

// DOM element is always guaranteed to be returned (and never React component ref)
// semantics is absolutely different from react ref
<Ref innerRef={domNode => ..}>
  ...
</Ref>

Consider the following potential implementation for render method of some component - you would never able to tell which logic will be fired:

<Ref innerRef={ (?) => ... }>
  <SomeComponent />
</Ref>

Have expressed these concerns earlier here: #459 (comment). Also, React docs have warnings even about just introducing ref forwarding (https://reactjs.org/docs/forwarding-refs.html) - this is clearly might be a breaking change; and with this implicit logic switch we will make problem even worse:

[kuzhelov]

TL;DR

By making checks against the most popular component libraries have found out that the satement we've made

Ref forwarding to DOM node should be supported by third-party components. If this is not the case, then it is a problem of third-party component.

doesn't stay for any of them (Material UI, React Bootstrap, Ant Design).

Ref forwarding mechanism is not supported by any of them for the provided components - in other words, if we will remove findDOMNode from our API, there won't be any chance for us to use third-party components in our component slots, given our ref-forwarding approach taken. More than that, currently even the following cases (which previously were working) will result in broken behavior with updated Ref's logic:

// note, this is the general principle that Wrapper component may implement 
// with the intent to auto forward ref
const StyledButton = React.forwardRef((props, ref) => <MaterialUIButton ref={ref} style={...} {...props} />

<Popup
  trigger={<StyledButton />}    // !!! NO EVENT HANDLERS BOUND - but previously worked fine !!!
  ..
/>

Detailed story

Final thoughts that I'd like to share, those relate to the following point we've made to argue necessity to introduce ref forwarding to Ref component:

'Proper' Ref forwarding should be supported by third-party components, so that refs provided would always resolve to the DOM element. If this is not the case, then it is a problem of third-party component.

For the sake fo argument I've considered the following component libraries to make checks on - was selecting by 'quite popular ones' principle:

• Material UI
• React Bootstrap
• Ant Design

For each of them I've tried to see what ref will be resolved to for the components they expose - because, as we were claiming, if ref is resolved to DOM node, then it is not our problem, it is a problem of component (or third-party component library in our case).

Results are provided below.

Material UI (GH likes: 8,4K)

import Button from '@material-ui/core/Button';
....
<Button ref={node => console.warn(node)}>Hey</Button>

RESULT: COMPONENT (not a DOM node)

React Bootstrap (GH likes: 14,2K)

import Alert from 'react-bootstrap/lib/Alert'
....
<Alert ref={node => console.warn(node)} />

RESULT: COMPONENT (not a DOM node)

Ant Design (GH likes: 36,4K)

import { DatePicker } from 'antd'
....
<DatePicker ref={node => console.warn(node)} />

RESULT: COMPONENT (not a DOM node)

---

Conclusion

With these results, I still see this step

• introducing much more complexity
• introducing breaking change for Ref component, as type of returned value in the callback is not defined now
• doesn't introducing any clear value (I mean, what are the cases we would like to support? Ref forwarding with returning DOM nodes - but the we should provide an implementation that would still guarantee that DOM node is provided as innerRef callback arg, then I will understand that)
• based on this review - would fail seamless integration with third-party components

given all these points I really believe that we are making premature mistaken step here.

[levithomason]

I think this analysis is too surface for two reasons. GitHub stars are not a good indicator of the latest best practices or the best ideas. The current state of a repo is not a good indicator of the current direction or views of the maintainers and contributors.

Material UI

Note that Material UI provides a RootRef component that always returns a DOM node:
https://material-ui.com/api/root-ref. This was done per the guidance given by the React team.

It is also worth noting that the primary voices in the ref discussion are the React Bootstrap maintainers. You can see there that their intent is that the Ref component pattern should always return a DOM node, not a class instance. Note the use of findDOMNode() in the utility.

Also, if you read Material UI's issues and PRs about Refs, you'll see they are struggling with the same problems we are. Specifically, you can see the community issues complaining that their refs don't support forwardRef. You can also see their PR where they've updated their refs to do so mui/material-ui#11757.

Note, they check for an existing ref, whether the ref has a ref (ie it is a class), handle functional refs explicitly, and also handle forwardRef (ref.current) explicitly:

handleRefInput = node => {
  this.input = node;

  let ref;

  if (this.props.inputRef) {
    ref = this.props.inputRef;
  } else if (this.props.inputProps && this.props.inputProps.ref) {
    ref = this.props.inputProps.ref;
  }

  if (ref) {
    if (typeof ref === 'function') {
      ref(node);
    } else {
      ref.current = node;
    }
  }
};

React Bootstrap

Note that React Bootstrap also solve this problem by exposing props like inputRef which also return the DOM nodes. These are the refs you would need in these cases, DOM nodes that have APIs you sometimes need, not component instances.

---

The point I'm making is that on the surface your argument seems correct, however, if you dig deeper and allow more time and real-world usage to pass you will find that the useful thing about refs is getting DOM nodes. User's don't want class instances and, as much as possible, they don't want special cases.

[layershifter]

const StyledButton = React.forwardRef((props, ref) => <MaterialUIButton ref={ref} style={...} {...props} />

@kuzhelov the provided case is fully invalid, you're forwarding ref to component that doesn't support this feature. It's a shot in the foot 💣

---

Users want to have ref forwarding:
mui/material-ui#13492 (comment)
mui/material-ui#10825 (comment)

mui/material-ui#13221 (issue marked as important)

And the same problems with deprecation:
mui/material-ui#13394

---

facebook/react#13841 (comment)

[kuzhelov]

@levithomason

yes, but the key difference from our Ref is that these components are guaranteeing to return DOM node by their API - this is what I was talking about through all the comments to this PR: our Ref now is not guaranteed to return DOM node anymore

---

Speaking of the analysis taken:

This analysis is too surface

I was aimed to challenge the following statement:

any third-party component should resolve ref to DOM node, and if it is not the case, then it is a component's problem

So, that was the motivation for the approach to analysis I've taken, and to my mind its results loudly prove that statement above doesn't reflect current state of things.

---

@layershifter

the provided case is fully invalid, you're forwarding ref to component that doesn't support this feature. It's a shot in the foot

when this code is written by the same person (and, thus, entirely visible) - yes, it is. But consider the case when

• wrapped component is client's one (Foo). She has used this component as Stardust's Popup trigger, and everything was working fine
• next day some wrappers third-party lib was considered by client to wrap her Foo component. This third-party lib is following ref-forwarding ideology, based on the assumption that wrappers should forward refs and let wrapper components resolve them to DOM nodes.

// this what client's code now is:
import { wrapped } from 'wrappers'

const Foo = () => ...

export wrapped(Foo)

and, suddenly, after that everything has stopped to work. Is it clear for the client that she has done something that might shoot her leg? Don't think so

[levithomason]

@kuzhelov it is not quite that solid. The Material UI inputRef attempts to resolve nested refs by checking for this.props.inputRef.ref. That would be an inputRef passed by the user which resulted in a class, that also had a ref. However, Material has no control over whether or not the final ref obtained will resolve to a DOM node. All they can do is call ref() if it is a function or handle ref.current if it is forwarded. What the result of that will be is not in the control of the library since it is passed in by the user at runtime. This is an infinite loop, there is no way to ensure that all components from all users will always use refs that resolve to DOM nodes. Therefore, it is impossible to create a ref API that guarantees all refs will return DOM nodes every time.

---

I believe we are all saying the same thing in regard to the problems that are present. Let's turn to solutions at this point. What are the optimal solutions to these issues?

I would start with the statement that returning a class is not a solution, but a problem as well. Our goals are:

• return the DOM node as often as possible, every time if possible
• support every form of ref API

Let's shift our focus to APIs and patterns for meeting these goals.

[miroslavstastny]

I vote for merging this. Let's iterate when we hit the wall.

[kuzhelov]

@levithomason

Speaking of Material's case provided here: #491 (comment) - please, note that this is about Input component only, as well the fact that Material doesn't support slot functionality, where any arbitrary client's component may be passed - they are handling ref forwarding for their own Input component. Thus, they are doing pretty safe thing:

• they know what is the implementation of their Input component, and they do know for certain what ref will be resolved to (it is always guaranteed to be the DOM node, as this is how this component is designed)
• thus, there are no exceptions in the promises of the docs where Material states that DOM node is guaranteed to be always returned

Note, this is completely different from the Stardust's case where there is possibility for any client component to be used as a slot content.

---

This is an infinite loop, there is no way to ensure that all components from all users will always use refs that resolve to DOM nodes.

There is, actually - and this will be about following forward refs chain and do findDOMNode at the final stage.

Note that all the implementations we are considering from other libraries, as well as the one that is proposed by this PR - those still largely rely on the findDOMNode support, as there is no other reliable mechanism to fetch it. Thus, with that being acknowledged, the only thing that I am, in fact, proposing (and arguing about) is the API consistency - if we agree that findDOMNode cannot be removed from Ref's implementation, we can utilize it then to provide this API consistency.

[levithomason]

Thanks for the correction on my misreading of materials' input. I think I've become a bit hasty in trying to close this issue. Let's move on.