RFC: Open questions around "Collection Shorthand"

[jeffcarbs]

See https://github.com/TechnologyAdvice/stardust/pull/567#discussion_r81444584 for a more detailed description of what I'm calling "Collection Shorthand".

---

I've been thinking about collection shorthand a little bit and have some open questions that I wanted to bring up so we can start figuring it out.

ChildKey

When rendering the items it's necessary to generate a unique key for each item unless one is passed. In some cases it may be possible to do this given the props (see the rendering of each MenuItem here. However, there are issues even with this type of approach

• If the items are string/props shorthand, generally we'll be able to imply a key from those props. However, if the items are elements with only children, this wouldn't work (e.g. in the MenuItem case the content/name props would be blank).
• Certain types of data may not even have uniqueness amongst content. TableRow > TableCell comes to mind. Imagine a row where each cell is the string "n/a" - we'd be unable to generate a key just from the props.

Should we do any of the following:

• Fallback to index-based keys in cases where we can't guarantee uniqueness. This is an anti-pattern but it would work. Perhaps we do this and display a warning in dev like "no key or childKey was passed so we're falling back to index-based keys, which can yield unreliable results. You should specify keys yourself to guarantee uniqueness".
• Don't set a default childKey unless we're certain we guarantee uniqueness and just have the user see the Warning: Each child in an array or iterator should have a unique "key" prop.

When you're missing keys, React needs to re-render the whole collection because it can't be sure. When you generate duplicate keys for different elements React squashes the duplicates and only renders one. So getting it wrong is much worse than just not doing anything.

Interacting with child props

In some components, it's necessary to be able to use the props for each item created by the shorthand. For example, there's an open issue to allow specifying string elements for Dropdown options (#470). In this case, I think it makes sense to make to use shorthand. However, we need to do things like filtering/searching, which rely on turning a literal into a props object or being able to use the props of a passed React element.

I'm thinking we can do something like this:

• whenever props change (e.g. componentWillMount, componentWillReceiveProps), if the items have changed we run something like (e.g. for Dropdown):

this.setState({ items: _.map(this.props.items, (item, index) => DropdownItem.create(item, defaultProps) })

• now whenever we do anything with the items, they will be always be fully "hydrated" react elements so we can interact with them in a consistent way (e.g. item.props.value) instead of needing to do something like _.isObject(item) ? item.value : item.

---

There may be some other issues so feel free to add them here for discussion.

Thoughts?

[levithomason]

ChildKey

Fallback to index-based keys in cases where we can't guarantee uniqueness.

I think we should agree that we'll never use index in keys. It would be better to let the user see the error, and go add their own childKey to the shorthand.

Don't set a default childKey unless we're certain we guarantee uniqueness and just have the user see the Warning...

👍

There are other options like generated ids or WeakMap, but I'm afraid these are frail themselves and require too much tooling than is worth it. It seems we should just do as React has done and require the key if anything.

Interacting with child props

In this case, I think it makes sense to make to use shorthand
👍

we need to do things like filtering/searching, which rely on turning a literal into a props object

We search/filter on text/value of the options object. Those options are currently pulled in from props. All this happens in getMenuOptions. We then use the options from that method to renderOptions which maps over the options and returns DropdownItem elements.

If I'm thinking correctly here, we could update getMenuOptions to use a DropdownItem factory. Then as you've noted, update any usages of options returned by getMenuOptions from opt.val to item.props.val since it will be returning elements now. Probably also rename it to getDropdownItems or some suitable name.

I think we can and should avoid adding the items to state though. If we find that we're creating the DropdownItems several times each render, due to getDropdownItems being called from several methods, then I could see doing this once in the lifecycle methods you noted but I'd suggest we just put the items on the component this.items instead. This way we aren't creating another render.

Note, getMenuOptions is the SSOT for the items. All other methods use this method, including during the render cycle. So, updating that once place takes care of the entire component.

[levithomason]

I believe this is solved in #645. There is now a factory for collection shorthand. It implements a key generator that generates keys based on the final props of the factory. It defers to the key prop first, the childKey prop second (value or function), else it generates a key.

If generation fails to produce a unique key, then we leave it to React warnings to inform the user that need to provide unique keys.