Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(factories): handle falsy key values #1729

Merged
merged 4 commits into from
Jun 1, 2017
Merged

fix(factories): handle falsy key values #1729

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2b2f767
fix(Dropdown): fix key handling
Jun 1, 2017
64b89dd
fix(Dropdown): fix key handling
Jun 1, 2017
f94fed0
fix(factories): handle falsy keys
levithomason Jun 1, 2017
aab7f34
chore(package): update package-lock.json
levithomason Jun 1, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 12 additions & 7 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions src/lib/factories.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,10 +77,10 @@ export function createShorthand(Component, mapValueToProps, val, options = {}) {
// ----------------------------------------

// Use key, childKey, or generate key
if (!props.key) {
if (_.isNil(props.key)) {
const { childKey } = props

if (childKey) {
if (!_.isNil(childKey)) {
// apply and consume the childKey
props.key = typeof childKey === 'function' ? childKey(props) : childKey
delete props.childKey
Expand Down
7 changes: 5 additions & 2 deletions src/modules/Dropdown/Dropdown.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ import DropdownMenu from './DropdownMenu'

const debug = makeDebugger('dropdown')

const getKeyOrValue = (key, value) => _.isNil(key) ? value : key

/**
* A dropdown allows a user to select a value from a series of options.
* @see Form
Expand Down Expand Up @@ -1072,7 +1074,7 @@ export default class Dropdown extends Component {
<select type='hidden' aria-hidden='true' name={name} value={value} multiple={multiple}>
<option value='' />
{_.map(options, (option, i) => (
<option key={option.key || option.value} value={option.value}>{option.text}</option>
<option key={getKeyOrValue(option.key, option.value)} value={option.value}>{option.text}</option>
Copy link
Member Author

Choose a reason for hiding this comment

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

As described in #1720, 0 will break the previous condition while it's a valid key.

))}
</select>
)
Expand Down Expand Up @@ -1137,7 +1139,7 @@ export default class Dropdown extends Component {
const defaultProps = {
active: item.value === selectedLabel,
as: 'a',
key: item.key || item.value,
key: getKeyOrValue(item.key, item.value),
Copy link
Member Author

Choose a reason for hiding this comment

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

^

onClick: this.handleLabelClick,
onRemove: this.handleLabelRemove,
value: item.value,
Expand Down Expand Up @@ -1168,6 +1170,7 @@ export default class Dropdown extends Component {
onClick: this.handleItemClick,
selected: selectedIndex === i,
...opt,
key: getKeyOrValue(opt.key, opt.value),
Copy link
Member Author

Choose a reason for hiding this comment

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

Problem

Before #1639, key was captured from value. In #1639 we refactored this to factory usage, but as we know factory will not create key automatically when it recieves an object and it's expected behaviour.

But, it's breaking change that I think should be reverted, because our users rely on that key can be automatically generated from a value prop (#1710).

Copy link
Member

Choose a reason for hiding this comment

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

You are correct, however, this is a semver fail on my part. I should have incremented the minor (since we're pre 1.0).

I do not think we should ship this feature moving forward. In 1.0 we will not automatically generate keys when using shorthand objects or elements. Users will have to pass a key manually. If we add this now, it will encourage users to rely on value being used as the key (at least in the Dropdown only).

If we feel very strongly about fixing this regression, I am willing to ship this as a patch right now to take care of current users as long as we immediately follow it up with a breaking(): ... PR that removes it for 0.69.0.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I completely agree 👍 Let's ship this fix as 0.68.5 and remove this line in 0.69.0, and mark PR that makes this change as breaking.

// Needed for handling click events on disabled items
style: { ...opt.style, pointerEvents: 'all' },
}))
Expand Down
124 changes: 94 additions & 30 deletions test/specs/lib/factories-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,55 +168,119 @@ describe('factories', () => {
})
})

describe('child key', () => {
it('uses the `key` prop from an element', () => {
getShorthand({ value: <div key='foo' /> })
.should.have.property('key', 'foo')
describe('key', () => {
it('is not consumed', () => {
getShorthand({ value: { key: 123 } })
.props.should.have.property('key')
})

it('uses the `key` prop as a string', () => {
getShorthand({ value: { key: 'foo' } })
.should.have.property('key', 'foo')
})
describe('on an element', () => {
it('works with a string', () => {
getShorthand({ value: <div key='foo' /> })
.should.have.property('key', 'foo')
})

it('uses the `key` prop as a number', () => {
getShorthand({ value: { key: 123 } })
.should.have.property('key', '123')
it('works with a number', () => {
getShorthand({ value: <div key={123} /> })
.should.have.property('key', '123')
})

it('works with falsy values', () => {
getShorthand({ value: <div key={null} /> })
.should.have.property('key', 'null')

getShorthand({ value: <div key={0} /> })
.should.have.property('key', '0')

getShorthand({ value: <div key='' /> })
.should.have.property('key', '')
})
})

it('uses the `childKey` prop as a string', () => {
getShorthand({ value: { childKey: 'foo' } })
.should.have.property('key', 'foo')
describe('on an object', () => {
it('works with a string', () => {
getShorthand({ value: { key: 'foo' } })
.should.have.property('key', 'foo')
})

it('works with a number', () => {
getShorthand({ value: { key: 123 } })
.should.have.property('key', '123')
})

it('works with falsy values', () => {
getShorthand({ value: { key: null } })
.should.have.property('key', 'null')

getShorthand({ value: { key: 0 } })
.should.have.property('key', '0')

getShorthand({ value: { key: '' } })
.should.have.property('key', '')
})
})
})

it('uses the `childKey` prop as a number', () => {
describe('childKey', () => {
it('is consumed', () => {
getShorthand({ value: { childKey: 123 } })
.should.have.property('key', '123')
.props.should.not.have.property('childKey')
})

it('calls `childKey` with the final `props` if it is a function', () => {
const props = { foo: 'foo', childKey: sandbox.spy(({ foo }) => foo) }
it('is called with the final `props` if it is a function', () => {
const props = { foo: 'bar', childKey: sandbox.spy(({ foo }) => foo) }
const element = getShorthand({ value: props })

props.childKey.should.have.been.calledOnce()
props.childKey.should.have.been.calledWithExactly({ foo: 'foo', key: 'foo' })
props.childKey.should.have.been.calledWithExactly({ foo: 'bar', key: 'bar' })

element.key.should.equal('foo')
element.key.should.equal('bar')
})

it('consumes the childKey prop', () => {
getShorthand({ value: { childKey: 123 } })
.props.should.not.have.property('childKey')
})
describe('on an element', () => {
it('works with a string', () => {
getShorthand({ value: <div childKey='foo' /> })
.should.have.property('key', 'foo')
})

it('works with a number', () => {
getShorthand({ value: <div childKey={123} /> })
.should.have.property('key', '123')
})

it('works with falsy values', () => {
getShorthand({ value: <div childKey={null} /> })
.should.have.property('key', null)

it('is generated from shorthand string values', () => {
getShorthand({ value: 'foo' })
.should.have.property('key', 'foo')
getShorthand({ value: <div childKey={0} /> })
.should.have.property('key', '0')

getShorthand({ value: <div childKey='' /> })
.should.have.property('key', '')
})
})

it('is generated from shorthand number values', () => {
getShorthand({ value: 123 })
.should.have.property('key', '123')
describe('on an object', () => {
it('works with a string', () => {
getShorthand({ value: { childKey: 'foo' } })
.should.have.property('key', 'foo')
})

it('works with a number', () => {
getShorthand({ value: { childKey: 123 } })
.should.have.property('key', '123')
})

it('works with falsy values', () => {
getShorthand({ value: { childKey: null } })
.should.have.property('key', null)

getShorthand({ value: { childKey: 0 } })
.should.have.property('key', '0')

getShorthand({ value: { childKey: '' } })
.should.have.property('key', '')
})
})
})

Expand Down
14 changes: 14 additions & 0 deletions test/specs/modules/Dropdown/Dropdown-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1497,6 +1497,20 @@ describe('Dropdown', () => {
.find('DropdownItem')
.everyWhere(item => item.should.have.prop('data-foo', 'someValue'))
})

it('handles keys correctly', () => {
const customOptions = [
{ key: 0, text: 'foo', value: 'foo' },
{ key: null, text: 'bar', value: 'bar' },
{ key: undefined, text: 'baz', value: 'baz' },
]
wrapperShallow(<Dropdown options={customOptions} selection />)
const items = wrapper.find('DropdownItem')

items.at(0).key().should.equal('0')
items.at(1).key().should.equal('bar')
items.at(2).key().should.equal('baz')
})
})

describe('selection', () => {
Expand Down