Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

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

Merged
merged 21 commits into from
Nov 16, 2018
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
e46e2c3
chore(package): correct peer dependencies
layershifter Nov 8, 2018
386e455
docs(CHANGELOG): add entry
layershifter Nov 8, 2018
d051f55
Merge branch 'master' of https://github.com/stardust-ui/react
layershifter Nov 8, 2018
f592c87
Merge branch 'master' of https://github.com/stardust-ui/react into fe…
layershifter Nov 9, 2018
720cd64
docs(ComponentExample): fix types
layershifter Nov 9, 2018
700f2a4
feat(handleRef): add a new util
layershifter Nov 9, 2018
25a62c6
feat(Ref): add willUnmount(), export component on top level
layershifter Nov 9, 2018
ba764d7
docs(handleRef): add JSDoc
layershifter Nov 9, 2018
371103d
test(handleRef|Ref): add tests
layershifter Nov 9, 2018
7559f05
Merge branches 'feat/handle-refd' and 'master' of https://github.com/…
layershifter Nov 13, 2018
77cc8d1
docs(CHANGELOG): add entry
layershifter Nov 14, 2018
1b7dfa6
improve description
layershifter Nov 14, 2018
2df0825
Merge branches 'feat/handle-refd' and 'master' of https://github.com/…
layershifter Nov 14, 2018
16992aa
Merge branch 'master' into feat/handle-refd
layershifter Nov 14, 2018
f4eee57
Merge branches 'feat/handle-refd' and 'master' of https://github.com/…
layershifter Nov 15, 2018
9f4845e
apply changes
layershifter Nov 15, 2018
220df2e
remove useless empty lines
layershifter Nov 15, 2018
fd8d49f
Merge branch 'master' into feat/handle-refd
layershifter Nov 15, 2018
4830a16
Merge branch 'master' into feat/handle-refd
layershifter Nov 15, 2018
1c33d31
Merge branch 'master' into feat/handle-refd
layershifter Nov 15, 2018
29d6a9a
Merge branch 'master' into feat/handle-refd
layershifter Nov 15, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
- Set default `chatBehavior` which uses Enter/Esc keys @sophieH29 ([#443](https://github.com/stardust-ui/react/pull/443))
- Add `iconPosition` property to `Input` component @mnajdova ([#442](https://github.com/stardust-ui/react/pull/442))
- Add `color`, `inverted` and `renderContent` props and `content` slot to `Segment` component @Bugaa92 ([#389](https://github.com/stardust-ui/react/pull/389))
- Export `Ref` component and add `handleRef` util @layershifter ([#459](https://github.com/stardust-ui/react/pull/459))

### Documentation
- Add all missing component descriptions and improve those existing @levithomason ([#400](https://github.com/stardust-ui/react/pull/400))
Expand Down
63 changes: 63 additions & 0 deletions docs/src/examples/components/Ref/Types/RefExampleRef.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
import React from 'react'
import { Button, Grid, Ref, Segment } from '@stardust-ui/react'

class RefExampleRef extends React.Component {
state = { isMounted: false }

createdRef = React.createRef<HTMLButtonElement>()
functionalRef = null

handleRef = node => (this.functionalRef = node)

componentDidMount() {
this.setState({ isMounted: true })
}

render() {
const { isMounted } = this.state

return (
<Grid columns={2}>
<Segment>
<Ref innerRef={this.handleRef}>
<Button primary>With functional ref</Button>
</Ref>
<Ref innerRef={this.createdRef}>
<Button>
With <code>createRef()</code>
</Button>
</Ref>
</Segment>

{isMounted && (
<code style={{ margin: 10 }}>
<pre>
{JSON.stringify(
{
nodeName: this.functionalRef.nodeName,
nodeType: this.functionalRef.nodeType,
textContent: this.functionalRef.textContent,
},
null,
2,
)}
</pre>
<pre>
{JSON.stringify(
{
nodeName: this.createdRef.current.nodeName,
nodeType: this.createdRef.current.nodeType,
textContent: this.createdRef.current.textContent,
},
null,
2,
)}
</pre>
</code>
)}
</Grid>
)
}
}

export default RefExampleRef
21 changes: 21 additions & 0 deletions docs/src/examples/components/Ref/Types/index.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import * as React from 'react'

import ComponentExample from 'docs/src/components/ComponentDoc/ComponentExample'
import ExampleSection from 'docs/src/components/ComponentDoc/ExampleSection'

const RefTypesExamples = () => (
<ExampleSection title="Types">
<ComponentExample
title="Ref"
description={
<span>
A component exposes the <code>innerRef</code> prop that always returns the DOM node of
both functional and class component children.
</span>
}
examplePath="components/Ref/Types/RefExampleRef"
/>
</ExampleSection>
)

export default RefTypesExamples
10 changes: 10 additions & 0 deletions docs/src/examples/components/Ref/index.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import * as React from 'react'
import Types from './Types'

const RefExamples: React.SFC = () => (
<div>
<Types />
</div>
)

export default RefExamples
23 changes: 14 additions & 9 deletions src/components/Ref/Ref.tsx
Original file line number Diff line number Diff line change
@@ -1,19 +1,20 @@
import * as PropTypes from 'prop-types'
import * as _ from 'lodash'
import { Children, Component } from 'react'
import * as React from 'react'
import { findDOMNode } from 'react-dom'
import { ReactChildren } from 'utils'

import { ReactChildren } from '../../../types/utils'
import { handleRef } from '../../lib'

export interface RefProps {
children?: ReactChildren
innerRef?: (ref: HTMLElement) => void
innerRef?: React.Ref<any>
Copy link
Member Author

Choose a reason for hiding this comment

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

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

}

/**
* This component exposes a callback prop that always returns the DOM node of both functional and class component
* children.
*/
export default class Ref extends Component<RefProps> {
export default class Ref extends React.Component<RefProps> {
static propTypes = {
/**
* Used to set content when using childrenApi - internal only
Expand All @@ -22,18 +23,22 @@ export default class Ref extends Component<RefProps> {
children: PropTypes.element,

/**
* Called when componentDidMount.
* Called when a child component will be mounted or updated.
*
* @param {HTMLElement} node - Referred node.
*/
innerRef: PropTypes.func,
innerRef: PropTypes.oneOfType([PropTypes.func, PropTypes.object]),
}

componentDidMount() {
_.invoke(this.props, 'innerRef', findDOMNode(this))
handleRef(this.props, 'innerRef', findDOMNode(this))
}

componentWillUnmount() {
handleRef(this.props, 'innerRef', null)
}

render() {
return this.props.children && Children.only(this.props.children)
return this.props.children && React.Children.only(this.props.children)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@layershifter layershifter Nov 15, 2018

Choose a reason for hiding this comment

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

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 🤔

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@kuzhelov kuzhelov Nov 16, 2018

Choose a reason for hiding this comment

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

@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).

Copy link
Member Author

@layershifter layershifter Nov 16, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

}
}
1 change: 1 addition & 0 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ export {
RadioGroupItemProps,
} from './components/RadioGroup/RadioGroupItem'

export { default as Ref, RefProps } from './components/Ref/Ref'
export { default as Segment, SegmentProps } from './components/Segment/Segment'

export { default as Status, StatusPropsWithDefaults, StatusProps } from './components/Status/Status'
Expand Down
36 changes: 36 additions & 0 deletions src/lib/handleRef.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import * as React from 'react'

/** A type that ensures that passed prop is defined in props and matches the type. */
type HandleRefProps<TProp extends string, N> = { [K in TProp]?: React.Ref<N> }

/**
* The function that correctly handles passing refs.
*
* @param props All components props
* @param propName The name of a ref prop
* @param node A node that should be passed by ref
*/
const handleRef = <P extends string, N>(props: HandleRefProps<P, N>, propName: P, node: N) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe setRefValue/setRefNode name would be more expressive?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@kuzhelov kuzhelov Nov 15, 2018

Choose a reason for hiding this comment

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

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).

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@kuzhelov kuzhelov Nov 16, 2018

Choose a reason for hiding this comment

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

@levithomason

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

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.

const ref: React.Ref<N> = props[propName]

if (process.env.NODE_ENV !== 'production') {
if (typeof ref === 'string') {
throw new Error(
'We do not support refs as string, this is a legacy API and will be likely to be removed in one of the future releases of React.',
)
}
}

if (typeof ref === 'function') {
ref(node)
return
}

if (typeof ref === 'object') {
// @ts-ignore The `current` property is defined as readonly, however it's a valid way because
// `ref` is a mutable object
ref.current = node
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

}
}

export default handleRef
2 changes: 2 additions & 0 deletions src/lib/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ export { default as getElementType } from './getElementType'
export { default as getUnhandledProps } from './getUnhandledProps'
export { default as mergeThemes } from './mergeThemes'
export { default as renderComponent, RenderResultConfig } from './renderComponent'

export { default as handleRef } from './handleRef'
export {
htmlImageProps,
htmlInputAttrs,
Expand Down
19 changes: 17 additions & 2 deletions test/specs/components/Ref/Ref-test.tsx
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import * as React from 'react'
import { shallow, mount } from 'enzyme'
import * as React from 'react'

import { CompositeClass, CompositeFunction, DOMClass, DOMFunction } from './fixtures'
import Ref from 'src/components/Ref/Ref'
import { CompositeClass, CompositeFunction, DOMClass, DOMFunction } from './fixtures'

const testInnerRef = Component => {
const innerRef = jest.fn()
Expand Down Expand Up @@ -42,5 +42,20 @@ describe('Ref', () => {
it('returns node from a class component', () => {
testInnerRef(CompositeClass)
})

it('returns "null" after unmount', () => {
const innerRef = jest.fn()
const wrapper = mount(
<Ref innerRef={innerRef}>
<CompositeClass />
</Ref>,
)

innerRef.mockClear()
wrapper.unmount()

expect(innerRef).toHaveBeenCalledTimes(1)
expect(innerRef).toHaveBeenCalledWith(null)
})
})
})
30 changes: 30 additions & 0 deletions test/specs/lib/handleRef-test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import * as React from 'react'
import handleRef from 'src/lib/handleRef'

describe('handleRef', () => {
it('throws an error when "ref" is string', () => {
const node = document.createElement('div')

expect(() => {
handleRef({ ref: 'ref' }, 'ref', node)
}).toThrowError()
})

it('calls with node when "ref" is function', () => {
const ref = jest.fn()
const node = document.createElement('div')

handleRef({ ref }, 'ref', node)

expect(ref).toBeCalledWith(node)
})

it('assigns to "current" when "ref" is object', () => {
const ref = React.createRef<HTMLDivElement>()
const node = document.createElement('div')

handleRef({ ref }, 'ref', node)

expect(ref.current).toBe(node)
})
})