-
Notifications
You must be signed in to change notification settings - Fork 53
[RFC] feat(handleRef): add an util for handling passed refs #459
Changes from all commits
e46e2c3
386e455
d051f55
f592c87
720cd64
700f2a4
25a62c6
ba764d7
371103d
7559f05
77cc8d1
1b7dfa6
2df0825
16992aa
f4eee57
9f4845e
220df2e
fd8d49f
4830a16
1c33d31
29d6a9a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 |
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 |
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 |
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> | ||
} | ||
|
||
/** | ||
* 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 | ||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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}
</>
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This ideally matches There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A
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.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @levithomason
Here, as you might see, semantics of these two has completely diverged - and, thus, it is not right to compare common practices of React If we would ask ourselves what is the closest React utility which functionality is mimicked by It turns out that 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.
Here is an example where this API will do that: render() {
return <Ref ...>{this.props.renderItems()}</Ref>
} If, for some reason, <Ref ...>
<>
{this.props.renderItems()}
</>
</Ref> So, the question is why we won't make the API safe at the first place then?. Note that This all leads to broader discussion related to terminology used for naming. Actually, the name <FindDOMNode handleNode={domNode => ...}>
<One />
<NoThrowIfAnotherOne />
</FindDOMNode> As you might see, there is really nothing in this logic that deals with component references ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We're planning to redesign this component to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I should note that |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
import * as React from 'react' | ||
|
||
/** | ||
* The function that correctly handles passing refs. | ||
* | ||
* @param ref An ref object or function | ||
* @param node A node that should be passed by ref | ||
*/ | ||
const handleRef = <N>(ref: React.Ref<N>, node: N) => { | ||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 I've checked React's source and they do the same thing in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://reactjs.org/docs/hooks-reference.html#useref
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
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', node) | ||
}).toThrowError() | ||
}) | ||
|
||
it('calls with node when "ref" is function', () => { | ||
const ref = jest.fn() | ||
const node = document.createElement('div') | ||
|
||
handleRef(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, node) | ||
expect(ref.current).toBe(node) | ||
}) | ||
}) |
There was a problem hiding this comment.
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.