-
Notifications
You must be signed in to change notification settings - Fork 933
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Validate that getMenuProps has set the ref correctly #525
Changes from 1 commit
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,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."`; |
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() | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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', | ||
|
@@ -918,6 +925,10 @@ class Downshift extends Component { | |
}, 200) | ||
|
||
componentDidMount() { | ||
if (this.getMenuProps.called && !this.getMenuProps.suppressRefError) { | ||
validateGetMenuPropsCalledCorrectly(this._menuNode, this.getMenuProps) | ||
} | ||
|
||
/* istanbul ignore if (react-native) */ | ||
if (preval`module.exports = process.env.BUILD_REACT_NATIVE === 'true'`) { | ||
this.cleanup = () => { | ||
|
@@ -983,6 +994,10 @@ class Downshift extends Component { | |
} | ||
|
||
componentDidUpdate(prevProps, prevState) { | ||
if (this.getMenuProps.called && !this.getMenuProps.suppressRefError) { | ||
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 should probably be: if (
this.getMenuProps.called &&
!this.getMenuProps.suppressRefError &&
preval`module.exports = process.env.BUILD_REACT_NATIVE !== 'true'`
) { so this code can be eliminated for native. Same with the above. 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’ll fix that tomorrow! Hmm any reason why we dont do that for ’getRootProps’ as well? Im not so familiar with RN but why wouldnt we want to validate the ref? 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. Yeah, I just checked and the only place we're using I'm thinking about making a custom macro that will shorten things for us/make these kinds of branches easier to maintain. But that'll be for later :) 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. Oh okay! I've pushed the latest changes now, and I think you should be able to use I like the sound of that idea! |
||
validateGetMenuPropsCalledCorrectly(this._menuNode, this.getMenuProps) | ||
} | ||
|
||
if ( | ||
this.isControlledProp('selectedItem') && | ||
this.props.selectedItemChanged( | ||
|
@@ -1031,6 +1046,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 | ||
|
@@ -1063,6 +1082,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) | ||
|
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.
Should we add this to a
componentDidMount
as well?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.
Yep that would make sense!