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

feat(Ref): support of forwardRef() API #491

Merged
merged 25 commits into from
Dec 5, 2018
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
62929d9
feat(Ref): support of `forwardRef()` API
layershifter Nov 19, 2018
9882478
fix styling
layershifter Nov 19, 2018
46c6913
Merge branches 'feat/ref-forward' and 'master' of https://github.com/…
layershifter Nov 19, 2018
17cbc97
update yarn.lock
layershifter Nov 19, 2018
33c95c1
add entry to changelog
layershifter Nov 19, 2018
57faf87
rename examples
layershifter Nov 21, 2018
bb93b77
Merge branch 'master' of https://github.com/stardust-ui/react into fe…
layershifter Nov 21, 2018
306ca3b
Merge branches 'feat/ref-forward' and 'master' of https://github.com/…
layershifter Nov 27, 2018
97594a7
fix changelog
layershifter Nov 27, 2018
c418eec
clean up test
layershifter Nov 27, 2018
747931c
Merge branches 'feat/ref-forward' and 'master' of https://github.com/…
layershifter Nov 29, 2018
d11dcc4
regenerate lock
layershifter Nov 29, 2018
1501b0d
Merge branches 'feat/ref-forward' and 'master' of https://github.com/…
layershifter Nov 29, 2018
0d75b11
Merge branches 'feat/ref-forward' and 'master' of https://github.com/…
layershifter Dec 3, 2018
6f0039f
fix review comments
layershifter Dec 3, 2018
1c77d7b
Merge branch 'master' of https://github.com/stardust-ui/react into fe…
layershifter Dec 3, 2018
31de50a
fix tests
layershifter Dec 3, 2018
5642eab
Merge branches 'feat/ref-forward' and 'master' of https://github.com/…
layershifter Dec 3, 2018
bce1d43
fix types
layershifter Dec 3, 2018
098ee71
Merge branch 'master' of https://github.com/stardust-ui/react into fe…
layershifter Dec 4, 2018
a2f7fd1
Merge branches 'feat/ref-forward' and 'master' of https://github.com/…
layershifter Dec 5, 2018
8e41c02
add entry to changelog
layershifter Dec 5, 2018
b8a3384
Merge branch 'master' of https://github.com/stardust-ui/react into fe…
layershifter Dec 5, 2018
e1d73e1
update changelog
layershifter Dec 5, 2018
921f35a
Merge branches 'feat/ref-forward' and 'master' of https://github.com/…
layershifter Dec 5, 2018
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm

## [Unreleased]

### Features
- `Ref` components uses `forwardRef` API by default @layershifter ([#491](https://github.com/stardust-ui/react/pull/491))

<!--------------------------------[ v0.12.0 ]------------------------------- -->
## [v0.12.0](https://github.com/stardust-ui/react/tree/v0.12.0) (2018-11-19)
[Compare changes](https://github.com/stardust-ui/react/compare/v0.11.0...v0.12.0)
Expand Down
54 changes: 54 additions & 0 deletions docs/src/examples/components/Ref/Types/RefExampleForwardRef.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
import React from 'react'
layershifter marked this conversation as resolved.
Show resolved Hide resolved
import { Grid, Ref, Segment } from '@stardust-ui/react'

const ExampleButton = React.forwardRef<HTMLButtonElement>((props, ref) => (
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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)

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'm planning to left this thing as is and push #495 that will resolve all our issues.

<div>
<button {...props} ref={ref} />
</div>
))

class RefExampleForwardRef extends React.Component {
Copy link
Contributor

@kuzhelov kuzhelov Nov 20, 2018

Choose a reason for hiding this comment

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

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?

forwardedRef = React.createRef<HTMLButtonElement>()
state = { isMounted: false }

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

render() {
const { isMounted } = this.state
const buttonNode = this.forwardedRef.current

return (
<Grid columns={2}>
<Segment>
<p>
A button below uses <code>forwardRef</code> API.
</p>

<Ref innerRef={this.forwardedRef}>
<ExampleButton>A button</ExampleButton>
</Ref>
</Segment>

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

export default RefExampleForwardRef
9 changes: 9 additions & 0 deletions docs/src/examples/components/Ref/Types/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,15 @@ const RefTypesExamples = () => (
}
examplePath="components/Ref/Types/RefExampleRef"
layershifter marked this conversation as resolved.
Show resolved Hide resolved
/>
<ComponentExample
title="Forward Ref"
description={
<span>
Works with <code>forwardRef</code> API.
</span>
}
examplePath="components/Ref/Types/RefExampleForwardRef"
/>
</ExampleSection>
)

Expand Down
2 changes: 2 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@
"lodash": "^4.17.10",
"prop-types": "^15.6.1",
"react-fela": "^7.2.0",
"react-is": "^16.6.3",
Copy link
Member Author

Choose a reason for hiding this comment

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

(!) A new dependency.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

"react-popper": "^1.0.2",
"what-input": "^5.1.2"
},
Expand All @@ -105,6 +106,7 @@
"@types/react": "^16.3.17",
"@types/react-custom-scrollbars": "^4.0.5",
"@types/react-dom": "^16.0.6",
"@types/react-is": "^16.5.0",
"@types/react-router": "^4.0.27",
"awesome-typescript-loader": "^5.2.1",
"connect-history-api-fallback": "^1.3.0",
Expand Down
47 changes: 41 additions & 6 deletions src/components/Ref/Ref.tsx
Original file line number Diff line number Diff line change
@@ -1,26 +1,37 @@
import * as PropTypes from 'prop-types'
import * as React from 'react'
import { findDOMNode } from 'react-dom'
import { isForwardRef } from 'react-is'

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

export interface RefProps {
children?: ReactChildren
children: ReactChildren
innerRef?: React.Ref<any>
}

export interface RefState {
child: React.ReactElement<any> & { ref: React.Ref<any> }
isForward: boolean
}

/**
* This component exposes a callback prop that always returns the DOM node of both functional and class component
* children.
*/
export default class Ref extends React.Component<RefProps> {
export default class Ref extends React.Component<RefProps, RefState> {
state = {
child: null,
isForward: false,
}

static propTypes = {
/**
* Used to set content when using childrenApi - internal only
* @docSiteIgnore
*/
children: PropTypes.element,
children: PropTypes.element.isRequired,

/**
* Called when a child component will be mounted or updated.
Expand All @@ -30,15 +41,39 @@ export default class Ref extends React.Component<RefProps> {
innerRef: PropTypes.oneOfType([PropTypes.func, PropTypes.object]),
}

static getDerivedStateFromProps(props: RefProps) {
const child = React.Children.only(props.children)

return {
child,
isForward: isForwardRef(child),
}
}

layershifter marked this conversation as resolved.
Show resolved Hide resolved
componentDidMount() {
handleRef(this.props.innerRef, findDOMNode(this))
if (!this.state.isForward) {
layershifter marked this conversation as resolved.
Show resolved Hide resolved
handleRef(this.props.innerRef, findDOMNode(this))
}
}

componentWillUnmount() {
handleRef(this.props.innerRef, null)
if (!this.state.isForward) {
handleRef(this.props.innerRef, null)
}
}

private handleRefOverride = (node: HTMLElement) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

handleRef(this.state.child.ref, node)
handleRef(this.props.innerRef, node)
}

render() {
return this.props.children && React.Children.only(this.props.children)
const { child, isForward } = this.state

return isForward
? React.cloneElement(child, {
ref: this.handleRefOverride,
})
: child
}
}
20 changes: 20 additions & 0 deletions test/specs/components/Ref/Ref-test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,12 @@ import * as React from 'react'
import Ref from 'src/components/Ref/Ref'
import { CompositeClass, CompositeFunction, DOMClass, DOMFunction } from './fixtures'

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

const testInnerRef = Component => {
const innerRef = jest.fn()
const node = mount(
Expand Down Expand Up @@ -57,5 +63,19 @@ describe('Ref', () => {
expect(innerRef).toHaveBeenCalledTimes(1)
expect(innerRef).toHaveBeenCalledWith(null)
})

it('works with "forwardRef" API', () => {
const forwardedRef = React.createRef<HTMLButtonElement>()
const innerRef = React.createRef()

mount(
<Ref innerRef={innerRef}>
<TestButton ref={forwardedRef} />
</Ref>,
)

expect(forwardedRef.current).toBeInstanceOf(Element)
expect(innerRef.current).toBeInstanceOf(Element)
})
})
})
9 changes: 8 additions & 1 deletion yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,13 @@
"@types/node" "*"
"@types/react" "*"

"@types/react-is@^16.5.0":
version "16.5.0"
resolved "https://registry.yarnpkg.com/@types/react-is/-/react-is-16.5.0.tgz#6b0dd43e60fa7c82b48faf7b487543079a61015a"
integrity sha512-yUYPioB2Sh5d4csgpW/vJwxWM0RG1/QbGiwYap2m/bEAQKRwbagYRc5C7oK2AM9QC2vr2ZViCgpm0DpDpFQ6XA==
dependencies:
"@types/react" "*"

"@types/react-router@^4.0.27":
version "4.0.27"
resolved "https://registry.yarnpkg.com/@types/react-router/-/react-router-4.0.27.tgz#553f54df7c4b09d6046b0201ce9b91c46b2940e3"
Expand Down Expand Up @@ -7492,7 +7499,7 @@ react-is@^16.4.1:
resolved "https://registry.yarnpkg.com/react-is/-/react-is-16.4.1.tgz#d624c4650d2c65dbd52c72622bbf389435d9776e"
integrity sha512-xpb0PpALlFWNw/q13A+1aHeyJyLYCg0/cCHPUA43zYluZuIPHaHL3k8OBsTgQtxqW0FhyDEMvi8fZ/+7+r4OSQ==

react-is@^16.6.1:
react-is@^16.6.1, react-is@^16.6.3:
version "16.6.3"
resolved "https://registry.yarnpkg.com/react-is/-/react-is-16.6.3.tgz#d2d7462fcfcbe6ec0da56ad69047e47e56e7eac0"
integrity sha512-u7FDWtthB4rWibG/+mFbVd5FvdI20yde86qKGx4lVUTWmPlSWQ4QxbBIrrs+HnXGbxOUlUzTAP/VDmvCwaP2yA==
Expand Down