Skip to content

Commit

Permalink
fix: validate that getMenuProps has set the ref correctly (downshift-…
Browse files Browse the repository at this point in the history
…js#525)

* fix: validate that getMenuProps has set the ref correctly

* fixup! fix: validate that getMenuProps has set the ref correctly

* fixup! fix: validate that getMenuProps has set the ref correctly
  • Loading branch information
alexandernanberg authored and Rendez committed Sep 30, 2018
1 parent bf87684 commit 2360777
Show file tree
Hide file tree
Showing 4 changed files with 169 additions and 2 deletions.
15 changes: 14 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -561,7 +561,7 @@ overridden (or overriding the props returned). For example:
| `getInputProps` | `function({})` | returns the props you should apply to the `input` element that you render. |
| `getItemProps` | `function({})` | returns the props you should apply to any menu item elements you render. |
| `getLabelProps` | `function({})` | returns the props you should apply to the `label` element that you render. |
| `getMenuProps` | `function({})` | returns the props you should apply to the `ul` element (or root of your menu) that you render. |
| `getMenuProps` | `function({},{})` | returns the props you should apply to the `ul` element (or root of your menu) that you render. |
| `getRootProps` | `function({},{})` | returns the props you should apply to the root element that you render. It can be optional. |

#### `getRootProps`
Expand Down Expand Up @@ -627,6 +627,13 @@ This method should be applied to the element which contains your list of items.
Typically, this will be a `<div>` or a `<ul>` that surrounds a `map` expression.
This handles the proper ARIA roles and attributes.

Required properties:

- `refKey`: if you're rendering a composite component, that component will need
to accept a prop which it forwards to the root DOM element. Commonly, folks
call this `innerRef`. So you'd call: `getMenuProps({refKey: 'innerRef'})` and
your composite component would forward like: `<ul ref={props.innerRef} />`

Optional properties:

- `aria-label`: By default the menu will add an `aria-labelledby` that refers
Expand All @@ -635,6 +642,12 @@ Optional properties:
available, then `aria-labelledby` will not be provided and screen readers
can use your `aria-label` instead.

In some cases, you might want to completely bypass the `refKey` check. Then you
can provide the object `{suppressRefError : true}` as the second argument to
`getMenuProps`.
**Please use it with extreme care and only if you are absolutely sure that the ref
is correctly forwarded otherwise `Downshift` will unexpectedly fail.**

```jsx
<ul {...getMenuProps()}>
{!isOpen
Expand Down
5 changes: 5 additions & 0 deletions src/__tests__/__snapshots__/downshift.get-menu-props.js.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`not applying the ref prop results in an error 1`] = `"downshift: The ref prop \\"ref\\" from getMenuProps was not applied correctly on your menu element."`;

exports[`using a composite component and calling getMenuProps without a refKey results in an error 1`] = `"downshift: The ref prop \\"ref\\" from getMenuProps was not applied correctly on your menu element."`;
114 changes: 114 additions & 0 deletions src/__tests__/downshift.get-menu-props.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
import React from 'react'
import {render} from 'react-testing-library'
import Downshift from '../'

const oldError = console.error

beforeEach(() => {
console.error = jest.fn()
})

afterEach(() => {
console.error = oldError
})

const Menu = ({innerRef, ...rest}) => <div ref={innerRef} {...rest} />

test('using a composite component and calling getMenuProps without a refKey results in an error', () => {
const MyComponent = () => (
<Downshift
children={({getMenuProps}) => (
<div>
<Menu {...getMenuProps()} />
</div>
)}
/>
)
expect(() => render(<MyComponent />)).toThrowErrorMatchingSnapshot()
})

test('not applying the ref prop results in an error', () => {
const MyComponent = () => (
<Downshift
children={({getMenuProps}) => {
getMenuProps()
return (
<div>
<ul />
</div>
)
}}
/>
)
expect(() => render(<MyComponent />)).toThrowErrorMatchingSnapshot()
})

test('renders fine when rendering a composite component and applying getMenuProps properly', () => {
const MyComponent = () => (
<Downshift
children={({getMenuProps}) => (
<div>
<Menu {...getMenuProps({refKey: 'innerRef'})} />
</div>
)}
/>
)
expect(() => render(<MyComponent />)).not.toThrow()
})

test('using a composite component and calling getMenuProps without a refKey does not result in an error if suppressRefError is true', () => {
const MyComponent = () => (
<Downshift
children={({getMenuProps}) => (
<div>
<Menu {...getMenuProps({}, {suppressRefError: true})} />
</div>
)}
/>
)
expect(() => render(<MyComponent />)).not.toThrow()
})

test('returning a DOM element and calling getMenuProps with a refKey does not result in an error if suppressRefError is true', () => {
const MyComponent = () => (
<Downshift
children={({getMenuProps}) => (
<div>
<ul {...getMenuProps({refKey: 'blah'}, {suppressRefError: true})} />
</div>
)}
/>
)
expect(() => render(<MyComponent />)).not.toThrow()
})

test('not applying the ref prop results in an error does not result in an error if suppressRefError is true', () => {
const MyComponent = () => (
<Downshift
children={({getMenuProps}) => {
getMenuProps({}, {suppressRefError: true})
return (
<div>
<ul />
</div>
)
}}
/>
)
expect(() => render(<MyComponent />)).not.toThrow()
})

test('renders fine when rendering a composite component and applying getMenuProps properly even if suppressRefError is true', () => {
const MyComponent = () => (
<Downshift
children={({getMenuProps}) => (
<div>
<Menu
{...getMenuProps({refKey: 'innerRef'}, {suppressRefError: true})}
/>
</div>
)}
/>
)
expect(() => render(<MyComponent />)).not.toThrow()
})
37 changes: 36 additions & 1 deletion src/downshift.js
Original file line number Diff line number Diff line change
Expand Up @@ -780,7 +780,14 @@ class Downshift extends Component {

menuRef = node => (this._menuNode = node)

getMenuProps = ({refKey = 'ref', ref, ...props} = {}) => {
getMenuProps = (
{refKey = 'ref', ref, ...props} = {},
{suppressRefError = false} = {},
) => {
this.getMenuProps.called = true
this.getMenuProps.refKey = refKey
this.getMenuProps.suppressRefError = suppressRefError

return {
[refKey]: callAll(ref, this.menuRef),
role: 'listbox',
Expand Down Expand Up @@ -918,6 +925,14 @@ class Downshift extends Component {
}, 200)

componentDidMount() {
if (
this.getMenuProps.called &&
!this.getMenuProps.suppressRefError &&
/* istanbul ignore next (react-native) */ preval`module.exports = process.env.BUILD_REACT_NATIVE !== 'true'`
) {
validateGetMenuPropsCalledCorrectly(this._menuNode, this.getMenuProps)
}

/* istanbul ignore if (react-native) */
if (preval`module.exports = process.env.BUILD_REACT_NATIVE === 'true'`) {
this.cleanup = () => {
Expand Down Expand Up @@ -983,6 +998,14 @@ class Downshift extends Component {
}

componentDidUpdate(prevProps, prevState) {
if (
this.getMenuProps.called &&
!this.getMenuProps.suppressRefError &&
/* istanbul ignore next (react-native) */ preval`module.exports = process.env.BUILD_REACT_NATIVE !== 'true'`
) {
validateGetMenuPropsCalledCorrectly(this._menuNode, this.getMenuProps)
}

if (
this.isControlledProp('selectedItem') &&
this.props.selectedItemChanged(
Expand Down Expand Up @@ -1031,6 +1054,10 @@ class Downshift extends Component {
this.getRootProps.called = false
this.getRootProps.refKey = undefined
this.getRootProps.suppressRefError = undefined
// we do something similar for getMenuProps
this.getMenuProps.called = false
this.getMenuProps.refKey = undefined
this.getMenuProps.suppressRefError = undefined
// we do something similar for getLabelProps
this.getLabelProps.called = false
// and something similar for getInputProps
Expand Down Expand Up @@ -1063,6 +1090,14 @@ class Downshift extends Component {

export default Downshift

function validateGetMenuPropsCalledCorrectly(node, {refKey}) {
if (!node) {
throw new Error(
`downshift: The ref prop "${refKey}" from getMenuProps was not applied correctly on your menu element.`,
)
}
}

function validateGetRootPropsCalledCorrectly(element, {refKey}) {
const refKeySpecified = refKey !== 'ref'
const isComposite = !isDOMElement(element)
Expand Down

0 comments on commit 2360777

Please sign in to comment.