Remove key-collision invariant.

[cpojer]

Keys often use dynamic data that are out of the control of the person creating the React component so key-collisions might happen in user code. While it is still important to avoid that from happening, we can still keep rendering with limited user impact (only impacts perf). This change makes it so that key collisions are logged the same as missing key warnings and it assigns fallback IDs for collisions.

Not sure if this is the best solution and whether it might break something - everything seemed to work fine from my testing.

Fixes #566.

cc @spicyj

[syranide]

Are you sure? It really seems like it should break on duplicate keys, if not immediately then possibly later when the wrong node is removed, and it really ought to, you f-ed up.

However, if both nodes have identical sub-hierarchy it could probably "adopt" the wrong one without breaking, but in other cases it should break.

[cpojer]

@syranide The collision case should be treated just like missing keys, in which we continue rendering just fine. There is no reason the whole app has to break (assuming your whole app is built using react).

[sophiebits]

You can't rely on traverseContext being an object holding the children. In ReactChildren.mapChildren, traverseContext is a MapBookKeeping object.

[cpojer]

Right, my initial version used a separate map to keep track of collisions but @sebmarkbage was heavily opposed because of perf implications - would it be reasonable to force contexts to provide a getResultMap function or something like that?

[sophiebits]

The best approach may be to put the logic in flattenChildren, though there you only have the full key, not the parts. (Also your code here is wrong because you want to be checking for nameSoFar + (nameSoFar ? SUBSEPARATOR : SEPARATOR) + key, not just key.)

[cpojer]

In flattenChildren I have no way to reassign the key.

[syranide]

@cpojer @spicyj What is the intended solution to this problem? I mean, so duplicate keys are unacceptable.

So one solution is to "unassign" conflicting keys, but that will wreak havoc on child behavior, also, what should happen when the component with the still assigned key is removed, should the "unassigned" component recreate itself to get that key? Or should it remain "unassigned" indefinitely? While neither sounds great, having it recreate itself and get the key sounds like the best action. However, conflicting keys would be a major no-no performance-wise (but perhaps it just has to work).

The best solution is possibly that in addition to their keys, keys may also have a key-index, which basically translates to 1$key. As you compute the rootNodeIDs you also keep a map key -> index, and when there's a conflict you increment index, and then you discard the map. Intuitively, it seems that this would present correct behavior as long as components with conflicting keys never swap places or are removed/inserted in-front, which sounds kind of acceptable to me. It doesn't sound very expensive to keep, but perhaps it is in this context?

Theoretically, it seems that components could somehow keep track of their own key-index as well/instead, allowing removes and inserts to have correct behavior, or at least better behavior (as in, they get the wrong props, but they are not recreated). But I'm not sure right now at what cost that would be. Intuitively it seems like just keeping the above map and if a component has an index, that is used to set key -> index instead, otherwise index is incremented and set on the component.

Or?

[zpao]

Needs some @sebmarkbage up in here

[sebmarkbage]

Duplicate keys is always incorrect. This is just trying to avoid a hard crash and try to keep going as long as possible. However, this can always lead to incorrect state. The solution should be whatever is fastest in the default case with no collisions. Perf in the broken case doesn't really matter.

Can't we just prefix all collisions with a counter format in flattenChildren?

if (collision) {
  result[(collisionCounter[name]++) + 'collision:' + name] = ...;
}

It doesn't resolve conflicts for you in map etc. and other helpers but you may not want to.

[syranide]

@sebmarkbage Isn't this best done in traverseAllChildren? I guess it makes sense to do it in flattenChildren, but you get the "key path" there and not just the key. So you would have to prepend (and thus add another character to escape) unless you want to dig into and parse the "key path". Or so it seems to me at least.

Doing it in traverseAllChildren the key-format could then simply be 1$key, no escaping needed, and it would extend to all implementations of flattenChildren (if that ever matters). Also, if we absolutely don't care about performance, we could just keep a single counter and skip the map, but perhaps that is taking it too far?

[sophiebits]

We could potentially also ignore everything after the first child with a given key, so

<div>
  <div key={1}>A</div>
  <div key={1}>B</div>
  <div key={3}>C</div>
</div>

renders A C only.

[syranide]

@spicyj I've thought about that, and it seems like the cleanest approach to me (right now at least), it's almost like a feature (keys are unique) as opposed to the "emergency survival" of trying to patch it up. I'm assuming it would be even cheaper as we would just not call callback or something like that?

[sebmarkbage]

@syranide Putting it in traverseAllChildren would require a lookup map within traverseAllChildren which is otherwise not necessary.

@spicyj Ignoring would work too. Simpler to implement. As long as we have warning hook.

[cpojer]

@balpert implemented ignoring the keys in #1364