[RFC] feat(handleRef): add an util for handling passed refs

[layershifter]

Addresses ideas from #417.

---

This PR solves the first described problem. Our Ref component and other components should consistently support functional and createRef() API.

Goals

• exports Ref component at top level
• adds support of createRef() API for the Ref component with reusable handleRef() util

[codecov]

Codecov Report

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

@@           Coverage Diff            @@
##             master    #459   +/-   ##
========================================
  Coverage          ?   88.4%           
========================================
  Files             ?      41           
  Lines             ?    1423           
  Branches          ?     181           
========================================
  Hits              ?    1258           
  Misses            ?     161           
  Partials          ?       4

Impacted Files	Coverage Δ
src/index.ts	100% <100%> (ø)
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 723f829...29d6a9a. Read the comment docs.

[layershifter]

We should use the React.Ref type for all our refs.

[layershifter]

May be someone knows React internals better. Is this valid thing?

https://github.com/facebook/react/blob/e58ecda9a2381735f2c326ee99a1ffa6486321ab/packages/react-reconciler/src/ReactFiberCommitWork.js#L618
https://github.com/facebook/react/blob/e58ecda9a2381735f2c326ee99a1ffa6486321ab/packages/react-reconciler/src/ReactFiberCommitWork.js#L629

I've checked React's source and they do the same thing in commitAttachRef() and commitDetachRef()

[layershifter]

https://reactjs.org/docs/hooks-reference.html#useref

useRef returns a mutable ref object whose .current property is initialized to the passed argument (initialValue). The returned object will persist for the full lifetime of the component.

[kuzhelov]

yes, the later comment suggests that this is a safe move

[kuzhelov]

maybe setRefValue/setRefNode name would be more expressive?

[layershifter]

For me, the current name is more obvious because we using the handle*Ref pattern for naming methods.

[kuzhelov]

would argue that we are using this pattern for naming is not an argument per se - the same way we can argue anything that is currently present in code base and may be improved. It seems that names that are more expressive is a better argument, and naming convention could be adjusted for that. Actually, while reviewing other PR today have noticed that FocusZone's code uses setRef name for the method with same semantics, and it seems much more intuitive (handle doesn't reflect any specifics and provides no clue on how exactly ref will be processed).

[layershifter]

ref can be a function or an object. When you're going to call a function you will never say set function, you will say call function.

Yesterday, we discussed the naming issue and agreed that setRef/invokeRef/applyRef doesn't provide more sense. I will ask Levi about this again.

[levithomason]

Let's merge it as is for now. We've had discussions on this in the past and have decided that this.ref / this.*Ref should be used for instance property naming and that handle* should be used for instance methods than handle callbacks (events and refs).

Semantic-Org/Semantic-UI-React#607
Semantic-Org/Semantic-UI-React#1360 (comment)
Semantic-Org/Semantic-UI-React#1360 (comment)

I don't have time at present to find the discussions on handle* naming for callback handlers, but it also a community standard and something that has already been debated.

---

If there is still strong disagreement on this @kuzhelov let's please address it by an RFC and vote after this is merged.

[kuzhelov]

@levithomason

not sure why these links are suggested - they seem to be not relevant to the discussed topic for the following reasons:

• Performance: Update usage of stateless, functional components to class-based components when event handlers are used Semantic-Org/Semantic-UI-React#607
  • not sure where I should follow from it - could you, please, suggest what exactly should be considered?
• fix(Dropdown|Modal|TextArea): update refs handlers Semantic-Org/Semantic-UI-React#1360 (comment)
• fix(Dropdown|Modal|TextArea): update refs handlers Semantic-Org/Semantic-UI-React#1360 (comment)
  • the discussion that is referenced by these two above is about 'handleNode' vs 'handleRef' - in contrast, we are discussing whether abstract 'handle' or specific 'set verb' should be used in the name.

I don't have time at present to find the discussions on handle* naming for callback handlers, but it also a community standard and something that has already been debated.

Not sure if that was debated for aforementioned reasons but, in either case, it seems that if we have strong motivation on using handle vs set, this motivation should be expressed (and it should be something other than 'this is what we used to', because we are not talking about public API that has lived for a while). In either case, the most (and, probably, best) I can do now is just to express my thoughts and reasons -and I've done that.

Will file an RFC for that as you've suggested.

[kuzhelov]

yes, the later comment suggests that this is a safe move

[kuzhelov]

why we introduce this restriction? An alternative approach would be to encapsulate the same thing as client may do to resolve this issue for mutliple children (given that its client intent to get first rendered DOM element), like:

render() {
  <>
     {this.props.children}
  </>
}

[layershifter]

Verifies that children has only one child (a React element) and returns it. Otherwise this method throws an error.

This ideally matches Ref idea because it will return a ref to a single element.

[kuzhelov]

to me it seems to be a question of safer API - with alternative suggested approach we will never throw an exception and prevent client's tree from being rendered, while, at the same time, for all of the cases will preserve behavior client would expect. There are certain reasons while classic refs could be assigned to single component only, but even with the fact how this ref abstraction is implemented in a Ref component we already violate some things related to ref semantics - for example, with this implementation reference to component instance is never returned, in contrast to the way ref is implemented for React.

So, don't see any serious violations here, and thus I would suggest to follow the rule of introducing as safe abstraction as possible to the client - the one that won't break entire rendering process for the client.

[layershifter]

A Children.only will throw this exception for multiple children:

Invariant Violation: React.Children.only expected to receive a single React element child.

And it's safe API, API that doesn't allow to shoot the leg. Your proposal is not an improvement it allows non obvious and confusing behaviour. It changes the behaviour silently.

• I will pass a single element and I will receive a single ref 🌟
• I will pass multiple elements and I will receive only first ref 🤔

[levithomason]

Refs only work on a single component. It is common practice for components that require a single child to use React.Children.only to enforce this. Let's not support multiple children.

[kuzhelov]

@levithomason
If we are talking about common practice (and, thus, user expectations), we should start with the fact that Ref component we are introducing, in contrast to React ref, have absolutely different semantics

• Ref captures the topmost (or the first from multiple topmost) DOM element from rendered component's tree
• React ref always captures component reference (this) for composite components, and DOM node for string components (like div, button, etc).

Here, as you might see, semantics of these two has completely diverged - and, thus, it is not right to compare common practices of React ref and Ref afterwards.

If we would ask ourselves what is the closest React utility which functionality is mimicked by Ref, it would be easy to conclude that Ref serves just as wrapper for ReactDOM.findDOMNode() method - and, thus, we should rather care about client's expectations on how this method behaves, not React ref.

It turns out that ReactDOM.findDOMNode() won't fail in cases when multiple elements are rendered, in contrast to what we have now for Ref component:

class Example extends React.Component {
  render() {
    return ['hello', 'hi'].map(text => <div>{text}</div>))
  }

  componentDidMount() {
    // returns first rendered <div />, no exception is thrown
    console.log('DOM node', ReactDOM.findDOMNode(this))
  }
}

Thus, from 'common practice' and 'what are user expectations' point of view, a behavior where no exception will be thrown (with first DOM node being captured) is what is really expected in this case.

---

@layershifter

And it's safe API, API that doesn't allow to shoot the leg.

Here is an example where this API will do that:

render() {
  return <Ref ...>{this.props.renderItems()}</Ref>
}

If, for some reason, renderItems() would (change to) return array of React components, the leg will be shoot - as the entire app's tree won't be rendered because of thrown exception. And in all these cases client fix will be to ensure that Fragment wrapper is used:

<Ref ...>
  <>
     {this.props.renderItems()}
  </>
</Ref>

So, the question is why we won't make the API safe at the first place then?. Note that findDOMNode() which semantics is reflected in implementation for Ref, doesn't throw in cases of components array rendered.

---

This all leads to broader discussion related to terminology used for naming. Actually, the name Ref seems to be quite misleading, as there is no React ref involved in the component's logic - its logic is about capturing DOM nodes, not component refs (it is a different story around innerRef that is used as storage for captured DOM node). It seems that 'FindDOMNode'
('CaptureDOMNode') would be proper name for this component, as it would reflect logic semantics in precise and correct way:

<FindDOMNode handleNode={domNode => ...}>
  <One />
  <NoThrowIfAnotherOne />
</FindDOMNode>

As you might see, there is really nothing in this logic that deals with component references (this).

[layershifter]

findDOMNode() is deprecated in StrictMode and we can't rely on it, it also forces us to use react-dom.

We're planning to redesign this component to use forwardRef API in the most cases and use findDOMNode() only as a fallback. So, if React's team will decide to remove findDOMNode() it will be not issue for us.

[layershifter]

I should note that Ref component is used widely in our components, so we should be more accurate with usage of DOM specific APIs and behaviours.

[layershifter]

I've made the changes that we discussed yesterday with @levithomason. Ready to go!

[kuzhelov]

still would be glad if we'll be able to agree on the questions raised - those are certainly not the PR blockers, but still are the ones that I see as points for improvement

[levithomason]

The naming handleRef is not the greatest, however, after an hour of discussion and review I cannot see a better name. Let's merge this 👍