From e4dd465dc6861cfd0dcc4607deaa3c59d253323c Mon Sep 17 00:00:00 2001 From: olfedias Date: Mon, 19 Nov 2018 18:04:32 +0200 Subject: [PATCH 1/4] feat(Ref): add support of `createRef` API --- .../addons/Ref/Types/RefExampleRef.js | 64 +++++++++++++------ src/addons/Portal/Portal.js | 5 +- src/addons/Ref/Ref.d.ts | 2 +- src/addons/Ref/Ref.js | 16 +++-- src/elements/Input/Input.js | 3 +- src/lib/handleRef.js | 31 +++++++++ src/lib/index.js | 1 + src/modules/Dropdown/DropdownSearchInput.js | 8 ++- test/specs/lib/handleRef-test.js | 36 +++++++++++ 9 files changed, 134 insertions(+), 32 deletions(-) create mode 100644 src/lib/handleRef.js create mode 100644 test/specs/lib/handleRef-test.js diff --git a/docs/src/examples/addons/Ref/Types/RefExampleRef.js b/docs/src/examples/addons/Ref/Types/RefExampleRef.js index be8883f6f7..dbb864c387 100644 --- a/docs/src/examples/addons/Ref/Types/RefExampleRef.js +++ b/docs/src/examples/addons/Ref/Types/RefExampleRef.js @@ -1,36 +1,64 @@ -import React, { Component } from 'react' +import React, { Component, createRef } from 'react' import { Grid, Table, Ref, Segment } from 'semantic-ui-react' export default class RefExampleRef extends Component { state = {} - handleRef = node => this.setState({ node }) + createdRef = createRef() + functionalRef = null + + handleRef = (node) => { + this.functionalRef = node + } + + componentDidMount() { + // eslint-disable-next-line react/no-did-mount-set-state + this.setState({ isMounted: true }) + } render() { - const { node } = this.state + const { isMounted } = this.state return ( - - - - An example node - + + + + + An example node with functional ref + + + + An example node with ref from createRef() + + + - - {node && ( + + {isMounted && ( + + Type + + nodeName + + + textContent + + + - nodeName - {node.nodeName} - - - nodeType - {node.nodeType} + Functional ref + {this.functionalRef.nodeName} + {this.functionalRef.textContent} + - textContent - {node.textContent} + + From createRef() + + {this.createdRef.current.nodeName} + {this.createdRef.current.textContent}
diff --git a/src/addons/Portal/Portal.js b/src/addons/Portal/Portal.js index 7c1bf0b664..48a881dc69 100644 --- a/src/addons/Portal/Portal.js +++ b/src/addons/Portal/Portal.js @@ -7,6 +7,7 @@ import { AutoControlledComponent as Component, doesNodeContainClick, eventStack, + handleRef, makeDebugger, } from '../../lib' import Ref from '../Ref' @@ -115,7 +116,7 @@ class Portal extends Component { * * @param {HTMLElement} node - Referred node. */ - triggerRef: PropTypes.func, + triggerRef: PropTypes.oneOfType([PropTypes.func, PropTypes.object]), } static defaultProps = { @@ -334,7 +335,7 @@ class Portal extends Component { handleTriggerRef = (c) => { this.triggerNode = c - _.invoke(this.props, 'triggerRef', c) + handleRef(this.props.triggerRef, c) } render() { diff --git a/src/addons/Ref/Ref.d.ts b/src/addons/Ref/Ref.d.ts index a9ae253c41..9f3355298e 100644 --- a/src/addons/Ref/Ref.d.ts +++ b/src/addons/Ref/Ref.d.ts @@ -13,7 +13,7 @@ export interface StrictRefProps { * * @param {HTMLElement} node - Referred node. */ - innerRef?: (node: HTMLElement) => void + innerRef?: React.Ref } declare class Ref extends React.Component {} diff --git a/src/addons/Ref/Ref.js b/src/addons/Ref/Ref.js index 47ba20f137..53534ddc80 100644 --- a/src/addons/Ref/Ref.js +++ b/src/addons/Ref/Ref.js @@ -2,6 +2,8 @@ import PropTypes from 'prop-types' import { Children, Component } from 'react' import { findDOMNode } from 'react-dom' +import handleRef from '../../lib/handleRef' + /** * This component exposes a callback prop that always returns the DOM node of both functional and class component * children. @@ -12,20 +14,20 @@ export default class Ref extends Component { 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() { - const { innerRef } = this.props - - // Heads up! Don't move this condition, it's a short circuit that avoids run of `findDOMNode` - // if `innerRef` isn't passed // eslint-disable-next-line react/no-find-dom-node - if (innerRef) innerRef(findDOMNode(this)) + handleRef(this.props.innerRef, findDOMNode(this)) + } + + componentWillUnmount() { + handleRef(this.props.innerRef, null) } render() { diff --git a/src/elements/Input/Input.js b/src/elements/Input/Input.js index 606e9d3274..c67fcd1da4 100644 --- a/src/elements/Input/Input.js +++ b/src/elements/Input/Input.js @@ -10,6 +10,7 @@ import { customPropTypes, getElementType, getUnhandledProps, + handleRef, partitionHTMLProps, SUI, useKeyOnly, @@ -129,7 +130,7 @@ class Input extends Component { ...defaultProps, ...child.props, ref: (c) => { - _.invoke(child, 'ref', c) + handleRef(child.ref, c) this.handleInputRef(c) }, }) diff --git a/src/lib/handleRef.js b/src/lib/handleRef.js new file mode 100644 index 0000000000..eeb3102487 --- /dev/null +++ b/src/lib/handleRef.js @@ -0,0 +1,31 @@ +/** + * The function that correctly handles passing refs. + * + * @param {Function|Object} ref An ref object or function + * @param {HTMLElement} node A node that should be passed by ref + */ +const handleRef = (ref, node) => { + 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.', + ].join(' '), + ) + } + } + + if (typeof ref === 'function') { + ref(node) + return + } + + if (ref !== null && typeof ref === 'object') { + // The `current` property is defined as readonly, however it's a valid way because `ref` is a mutable object + // eslint-disable-next-line no-param-reassign + ref.current = node + } +} + +export default handleRef diff --git a/src/lib/index.js b/src/lib/index.js index 875d3d042e..996781f41e 100644 --- a/src/lib/index.js +++ b/src/lib/index.js @@ -20,6 +20,7 @@ export eventStack from './eventStack' export * from './factories' export getUnhandledProps from './getUnhandledProps' export getElementType from './getElementType' +export handleRef from './handleRef' export { htmlInputAttrs, diff --git a/src/modules/Dropdown/DropdownSearchInput.js b/src/modules/Dropdown/DropdownSearchInput.js index 5046d7ef1a..ea1137a464 100644 --- a/src/modules/Dropdown/DropdownSearchInput.js +++ b/src/modules/Dropdown/DropdownSearchInput.js @@ -3,7 +3,7 @@ import _ from 'lodash' import PropTypes from 'prop-types' import React, { Component } from 'react' -import { createShorthandFactory, customPropTypes, getUnhandledProps } from '../../lib' +import { createShorthandFactory, customPropTypes, getUnhandledProps, handleRef } from '../../lib' /** * A search item sub-component for Dropdown component. @@ -20,7 +20,7 @@ class DropdownSearchInput extends Component { className: PropTypes.string, /** A ref handler for input. */ - inputRef: PropTypes.func, + inputRef: PropTypes.oneOfType([PropTypes.func, PropTypes.object]), /** An input can receive focus. */ tabIndex: PropTypes.oneOfType([PropTypes.number, PropTypes.string]), @@ -43,7 +43,9 @@ class DropdownSearchInput extends Component { _.invoke(this.props, 'onChange', e, { ...this.props, value }) } - handleRef = c => _.invoke(this.props, 'inputRef', c) + handleRef = (c) => { + handleRef(this.props.inputRef, c) + } render() { const { autoComplete, className, tabIndex, type, value } = this.props diff --git a/test/specs/lib/handleRef-test.js b/test/specs/lib/handleRef-test.js new file mode 100644 index 0000000000..824bd9abdf --- /dev/null +++ b/test/specs/lib/handleRef-test.js @@ -0,0 +1,36 @@ +import React from 'react' + +import handleRef from 'src/lib/handleRef' +import { sandbox } from 'test/utils' + +describe('handleRef', () => { + it('throws an error when "ref" is string', () => { + const node = document.createElement('div') + + expect(() => { + handleRef('ref', node) + }).to.throw() + }) + + it('does not do anything when "ref" is null', () => { + expect(() => { + handleRef('ref', null) + }).to.not.throw() + }) + + it('calls with node when "ref" is function', () => { + const ref = sandbox.spy() + const node = document.createElement('div') + + handleRef(ref, node) + ref.should.have.calledWith(node) + }) + + it('assigns to "current" when "ref" is object', () => { + const ref = React.createRef() + const node = document.createElement('div') + + handleRef(ref, node) + ref.current.should.be(node) + }) +}) From 8c02a92a15ce962033115689dc63575285de241a Mon Sep 17 00:00:00 2001 From: olfedias Date: Mon, 19 Nov 2018 18:11:13 +0200 Subject: [PATCH 2/4] fix type --- src/modules/Dropdown/DropdownSearchInput.d.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/modules/Dropdown/DropdownSearchInput.d.ts b/src/modules/Dropdown/DropdownSearchInput.d.ts index c6d87be938..7a4b0c2fd2 100644 --- a/src/modules/Dropdown/DropdownSearchInput.d.ts +++ b/src/modules/Dropdown/DropdownSearchInput.d.ts @@ -15,7 +15,7 @@ export interface StrictDropdownSearchInputProps { className?: string /** A ref handler for input. */ - inputRef?: (c: HTMLInputElement) => void + inputRef?: React.Ref /** An input can receive focus. */ tabIndex?: number | string From cc7a583a6a04aebe471175de48363c5e80259440 Mon Sep 17 00:00:00 2001 From: olfedias Date: Mon, 19 Nov 2018 18:18:07 +0200 Subject: [PATCH 3/4] fix example --- .../examples/addons/Ref/Types/RefExampleRef.js | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/docs/src/examples/addons/Ref/Types/RefExampleRef.js b/docs/src/examples/addons/Ref/Types/RefExampleRef.js index dbb864c387..fe5a52ce0a 100644 --- a/docs/src/examples/addons/Ref/Types/RefExampleRef.js +++ b/docs/src/examples/addons/Ref/Types/RefExampleRef.js @@ -37,13 +37,15 @@ export default class RefExampleRef extends Component { {isMounted && ( - Type - - nodeName - - - textContent - + + Type + + nodeName + + + textContent + + From 1943dbe5d16fad2b61e28b754cb6631535b5eeef Mon Sep 17 00:00:00 2001 From: olfedias Date: Mon, 19 Nov 2018 18:26:14 +0200 Subject: [PATCH 4/4] fix tests --- test/specs/lib/handleRef-test.js | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/test/specs/lib/handleRef-test.js b/test/specs/lib/handleRef-test.js index 824bd9abdf..144e3a106f 100644 --- a/test/specs/lib/handleRef-test.js +++ b/test/specs/lib/handleRef-test.js @@ -5,16 +5,14 @@ import { sandbox } from 'test/utils' describe('handleRef', () => { it('throws an error when "ref" is string', () => { - const node = document.createElement('div') - expect(() => { - handleRef('ref', node) + handleRef('ref', document.createElement('div')) }).to.throw() }) it('does not do anything when "ref" is null', () => { expect(() => { - handleRef('ref', null) + handleRef(null, document.createElement('div')) }).to.not.throw() }) @@ -31,6 +29,6 @@ describe('handleRef', () => { const node = document.createElement('div') handleRef(ref, node) - ref.current.should.be(node) + ref.should.have.property('current', node) }) })