Item: chance of auto-generated key collision when using shorthand props

[noinkling]

Steps

<Item header="1984" meta="1984" />

// or even:
<Item>
  <Item.Content header="1984" meta={1984} />
</Item>

Expected Result

No issues.

Actual Result

Warning: Encountered two children with the same key, `1984`. Keys should be unique so that components maintain their identity across updates. Non-unique keys may cause children to be duplicated and/or omitted — the behavior is unsupported and could change in a future version.

The warning is caused by the keys being automatically set from the value, here:

Semantic-UI-React/src/lib/factories.js

Lines 89 to 92 in 9ef4156

	} else if (valIsString || valIsNumber) {
	// use string/number shorthand values as the key
	props.key = val
	}

Note that there's no need for keys at all in this example.

One workaround that prevents the keys from being set (and therefore the error):

<Item header={{ content: "1984" }} meta={{ content: "1984" }} />
// yuck

Version

0.77.1

Testcase

https://codesandbox.io/s/4r702vq8y4

[levithomason]

If you have duplicated shorthand values, pass props objects and include unique keys. Alternatively, you can use the full markup.

Shorthand is a convenience and it cannot cover every use case.

[noinkling]

Shorthand is a convenience and it cannot cover every use case.

I understand that, however if you're dynamically generating your props you might have no idea that there's a possibility of having duplicate keys until you happen to feed it the right data (or maybe not at all if the warning isn't shown in production, which could potentially result in insidious bugs). It's the fact that it's surprising/not easily predictable that makes it an issue.

I'm sure there's a good underlying reason for createShorthand() to use auto-generated keys, but...

• In this particular case keys don't serve any purpose. The elements are laid out in a static order and it's not like each prop can generate multiple elements.

  Looking at the code:

  Semantic-UI-React/src/views/Item/ItemContent.js

  Lines 46 to 54 in 9ef4156

  	return (
  	<ElementType {...rest} className={classes}>
  	{ItemHeader.create(header)}
  	{ItemMeta.create(meta)}
  	{ItemDescription.create(description)}
  	{ItemExtra.create(extra)}
  	{content}
  	</ElementType>
  	)

  Each of those .create() methods can accept an options object as a second argument that gets passed through to createShorthand() - a flag could be added that disables the autogenerated key (in this particular context and others where it makes sense) like:

  ItemHeader.create(header, { generateKey: false })  // or dontGenerateKey: true

• Regardless of context, using the value itself as a key is brittle. It would seem wise to at least concatenate it with the component name, or a random value, or something. Or use a counter. Sure, I can provide my own key, but in this case it feels like a copout; ensuring your keys are unique is basically React 101, and applies to libraries too.

[levithomason]

TL;DR

👍 PR for a generateKey option to .create() methods, also solves your use case.

👎 All other proposals above.

I will take some time to clarify and recap our history so I can link to it later :)

---

Each of those .create() methods can accept an options object as a second argument that gets passed through to createShorthand() - a flag could be added that disables the autogenerated key

We used to enable generateKey as a createShorthandFactory() option which is not possible as we don't know if/when keys will be needed for a given factory. However, disabling keys at the time of creation seems like a good solution.

I'd gladly merge a PR for this!

Regardless of context, using the value itself as a key is brittle.

Yes, don't use shorthand strings/numbers in cases where it might be brittle. Use props objects or the full markup.

It would seem wise to at least concatenate it with the component name, or a random value, or something. Or use a counter.

Unfortunately, none of these options will work:

• Concat with component name: This would still result in duplicate keys, just with longer names :)
• Random value: Random keys are actually worse than an index/counter. Not only does it break React, it introduces a performance hit :(
• Or something: We also used to generate keys from hashing props, however, if props and position change this also breaks. It is also a perf hit.
• Counter value: These keys are valid, however, there is no way to know which elements to assign them to. Child component state is then lost.

Sure, I can provide my own key, but in this case it feels like a copout; ensuring your keys are unique is basically React 101, and applies to libraries too.

I can see how it would feel that way but I assure you we aren't copping out on you. In React 102 and 103 we learn that keys must also be stable and predictable. Per the reconciliation docs:

"Keys should be stable, predictable, and unique. Unstable keys (like those produced by Math.random()) will cause many component instances and DOM nodes to be unnecessarily recreated, which can cause performance degradation and lost state in child components."

A key must be unique among its siblings, it must not change over time, and you must be able to keep it assigned to the same component between renders even when that component's props and position in the DOM change drastically. In short, there is no automatic way to do this. Hence, React requires user-defined keys.

I'm sure there's a good underlying reason for createShorthand() to use auto-generated keys, but...

Factories have a rich history of iteration. Fix-all solutions to handling keys seem obvious, however, the devil is in the details. We've gone through iterations of not handling keys at all, generating keys for all shorthand values, generating keys only for primitive values, generated keys behind a generateKey flag, and now always generating keys but only for primitive types. Each permutation was prompted by filed issues and sometimes lengthy discussions (search issues/PRs).

Why not generate random keys?

Random keys, like indexes, are an anti-pattern and will break React. https://reactjs.org/docs/reconciliation.html

Why not use a counter for keys?

This ends up being the same as using an index or random key. Though this key is unique and constant, it is not permanent. There is no way to know which position in the DOM a node has moved to in order to reassign it's key to it.

Why remove the createShorthandFactory() option generateKey

There is no way to know how/where a factory will be used. All factories may be used in arrays.
The consumer must determine if they need keys. It cannot be a library concern. #1428 (comment)

Why skip generated keys for objects and elements?

When using an object or element, the user is able to (and should) define their own unique key. If it were possible to generate keys from elements or props, React itself would not require user keys. facebook/react#1342

Why use shorthand strings/numbers as keys?

In this case, the user has no way of passing any additional information to the factory. The value is the only argument. In the majority of use cases, this value is unique (menu items, list of image src values, etc).

Why use keys at all?

Otherwise, shorthand factories could not be used in arrays. Since many factories are used in arrays and all factories can be used in arrays, we then must always have a key.

This is not cool.

Agreed. It is a shortcoming we'd love to not have. Any ingenious solution that satisfies all requirements will be implemented in a heartbeat. Until then, just pass a valid key.

[noinkling]

Thanks for taking the time to write a clear and detailed explanation, that all makes sense.

I'll throw together that PR. It might take a little time to go through and find all the appropriate .create() calls (and write/modify the tests).

[levithomason]

Awesome, that would be very much appreciated! Once the argument is added and tested, we can certainly help identify and update create() calls.

Almost all our usages will be able to take advantage of this. There are very few components that use the factories in arrays.

Just be sure to create a branch on your fork instead of using your master branch. This way, we can push changes to help out.