fix(breadcrumb): do not apply index to component keys

[dvdzkwsk]

Merge or deny at will, just encountered this while perusing the codebase and figured I'd submit a PR rather than resorting to a slack discussion :).

[levithomason]

There is no guarantee that the breadcrumb text is unique. The index provides the extra assurance.

[codecov-io]

Current coverage is 92.87% (diff: 100%)

No coverage report found for master at 64f145a.

Powered by Codecov. Last update 64f145a...ba631fa

[dvdzkwsk]

By applying the index to the key we are preventing React from reusing an unchanged component instance in the case that it moved positions. Index as a key is an antipattern, and while this sidesteps the core of that antipattern, it does so by negating the benefit provided in the reconciliation stage.

Text should be a sufficient key for bread crumbs (since each crumb should be unique, otherwise it's nonsensical), but, just in case, I adjusted it to allow for an optional key property should a user require this use case.

[levithomason]

This is a happy medium i think. All generated component props should allow passing an object for props. If a user duplicates text in their breadcrumb (or any other component), then they will have to specify their own unique keys.

[dvdzkwsk]

If that's the case, then this is a 👎 for generating components for users since we can't reliably provide a reusable key (and thus sidestep reconciliation benefits). Again, deny at will, took me all of 2 mins to submit it. I'd just like to see us work more with React than against it.

[levithomason]

I think the utility outweighs the perf/reconciliation benefits. Let's go with the key prop ability you've added and keep the best of both worlds.

[layershifter]

@davezuko I'm agree with you about key, but how about this? It's correct?

[dvdzkwsk]

Oh interesting. I was under the impression that we were restricting text to a string value and not a React node. However, this same issue would have occured even before this change, would it not? Either way, I can see 4 3 ways we handle this:

1. Restrict text to a string
2. Revert the change (ignore this, I don't think it's correct)
3. If text is not a string, we fall back to the index.
4. If you do not provide a string text value, you must provide a key

@levithomason ? I still think this would have been an issue before this change, but it's a good point you've made @layershifter .

[levithomason]

For initial v1, let's favor simplification. Limit the props to strings only. The sub component markup is available if user's need to use nodes.