cloneWithProps key/ref, what is the intended behavior?

[syranide]

@petehunt @sebmarkbage I've had a number of people ask this on IRC now, and I'm not really sure what the answer is myself. But the current situation seems illogical to me:

// key OK, ref OK
<div><Item key="a" ref="a" />

// key OK, ref OK
<div><Item key="a" ref="a" /><Item key="b" ref="b" />...

// key OK/ERR (possibly unexpected conflicts), ref is OK
<PlainList><Item key="a" ref="a" />

// key OK (may conflict), ref is OK
<PlainList><Item key="a" ref="a" /><Item key="b" ref="b" />...

// key OK, ref OK
<WrappedPlainList><Item key="a" ref="a" />

// key OK, ref OK
<WrappedPlainList><Item key="a" ref="a" /><Item key="b" ref="b" />...

// key ERR (discarded), ref ERR (discarded)
<CloneWithPropsList><Item key="a" ref="a" />

// cloneWithProps ERR (does not support arrays/objects)
<CloneWithPropsList><Item key="a" ref="a" /><Item key="b" ref="b" />...

// key OK, ref OK
<WrappedCloneWithPropsList><Item key="a" ref="a" />

// key OK, ref OK
<WrappedCloneWithPropsList><Item key="a" ref="a" /><Item key="b" ref="b" />...

// key ERR (discarded), ref ERR (discarded)
<ForEachCloneWithPropsList><Item key="a" ref="a" /><Item key="b" ref="b" />...

I would expect them all to work like <PlainList> (I think?). Or perhaps even more preferable, like <WrappedPlainList>. <Wrapped* is implemented as children = <div>{this.props.children}</div>, so cloneWithProps won't actually add props to the children as-is, but imagine that it did.

Am I missing something? Because the current situation seems fundamentally plain broken to me, using this.props.children as-is is dangerous and cloneWithProps destroys ownership.

I do understand that it's because of the change of ownership to the local instance, but what's the intended use-case for this behavior? But I'm not 100% sure what the expected behavior would be either... but it seems like keys should concatenate and all refs should be kept (i.e, multiple owners).

Related #1867 #1274

[syranide]

Updated OP a bit more to further clarify certain cases.

[sebmarkbage]

Could you expand on the actual implementation of the wrappers?

It should be noted that the intended usage of children and cloneWithProps is this:

children = Children.map(child => cloneWithProps(child, someProps));
// or
children = Children.map(child => <div>{cloneWithProps(child, someProps)}</div>);

In which case the true key would be taken from the resulting object and not really the child's key.

cloneWithProps is a mess because of the magic property manipulation. It should essentially just be a two level deep clone. { ...child, props: { ...child.props, ...someProps } }

Refs are broken partially because of the string/owner API. First class refs #1373 should make it easier to implement shared refs.

However, it's a little dangerous to get a shared ref in that case since you're overriding properties that the owner might be relying upon for it's imperative hooks to work.

In general both the children and cloneWithProps API is just a patch work. This is just the tip of the complexity iceberg if you start reasoning about children and their keys.

I think that it's probably better to have a separate pipeline to pass properties from parents to children. For example through a context-like system that implicitly passes properties through the parent->child chain. That should eliminate most valid use cases for cloneWithProps.

Reasoning about children is often about wrapping something like with map. It's easy to screw up the keys though. So a more restrictive API would be nice.

[syranide]

// <WrappedCloneWithProps />
children = cloneWithProps(<div>{child}</div>, someProps));
// <ForEachCLoneWithProps />
children = Children.map(child => cloneWithProps(child, someProps));

The <WrappedCloneWithProps /> does not make practical sense, but it was just to illustrate that it did overall behave as I would expect for keys/refs.

Anyway, my primary intention with this was just to "confirm" my uneasy feeling regarding all this, as it seemed (theoretically) quite broken. Apparently I wasn't entirely wrong and it seems you guys have thought about it. So all is good, we shall make do with what we have for now. :)

Many thanks for the detailed reply!

[sebmarkbage]

What is <PlainList>?

[syranide]

@sebmarkbage Ah, yes that wasn't very intuitive perhaps... simply inserting the children as-is into the DOM, i.e <div><div key="potentiallyConflictingKey" />{this.props.children}</div> (inside a composite component).

<WrappedPlainList> is simply the equivalent of <div><div key="noLongerPotentiallyConflictingKey" /><div>{this.props.children}</div></div>.

(It's not really a great examination, I was simply trying to wrap my head around the "problem")

[sebmarkbage]

I see. I suspected you might be on to something and now I understand. The fact that this could cause a conflict is bad:

<div><div key="potentiallyConflictingKey" />{this.props.children}</div>

Normally it works out because you didn't need to put keys on single items so you don't. It was exclusively used to track identity in sets.

The fact that we added the key check in https://github.com/facebook/react/blob/master/src/core/shouldUpdateReactComponent.js changed that. So now it is a hazard. cc @spicyj

[sophiebits]

It was always true that <div><div key={x} /></div> would cause a remount when x changed; it was just the case that at the root the key was ignored. The behavior here now isn't any different from the previous key behavior.

[sebmarkbage]

Ah yes. True. It was not intended to be used as a cache breaker but I guess it already was.

[syranide]

@sebmarkbage Ah, hmm... for that simple case, I suppose keys could be bound to the owner as well (like ref), so keys would be prefixed with an owner GUID of sorts, then it should behave as one would expect. Anyway...

The more I think about how "key" really should work, the more interesting it becomes... it seems to me that the purpose of "key" is two-fold; managing life-time and stateful reordering. Skipping all my mind exercises in trial-and-error...

I'm (in this instant) feeling quite convinced that only the creator should be able to specify a true "key" for a descriptor (for stateful reordering) and it would be prefixed with a creator GUID (preventing any unforseen conflicts). Derivative descriptors can only append to an imaginary "keyPrefix" (for managing life-time) which does not have to be unique, derivate descriptors are inherently statefully reordered by simply rendering them in a different order.

In my mind, that currently makes a lot of sense, but perhaps I'm missing something obvious (or am failing to explain myself/too tired to think straight)? :)

[sebmarkbage]

The original keys were prefixed with the ownership level which ensured that owner was taken into account. There was a problem with that but I don't remember what.

Owner is already taken into account for life-time but it could collide for the reordering case.

[syranide]

I was probably somewhat off yesterday, anyway, stepping one step further back and looking at the problem from a more technical/data perspective. It seems that ref is straight-forward unless I missed something important and I would define it as:

descriptorRefs = [
  {instance: ReactInstance, key: InstanceLocalRefKey},
  ...
]

Simply an array of all instances that should receive a reference (actual implementation may vary ofc). Anyway, on to the meat:

However, key seems to be really quite complicated (to me at least), what I'm having most trouble wrapping my head around is "index as implicit key". It should seemingly be "computed" by the creator (in-case the children are filtered, etc, further on), but you can't do that since it's just an array/object of children and we can't/don't want to apply/require any immediate/special processing as children can be passed in any number of ways, not just as children... or have I missed something obvious?

The only immediate solution I can think of that appeals to me is only allowing filtering/sorting/interaction with lists of children through a dedicated API. Or ensuring that index is maintained after filtering/sorting, but that requires Map (can be polyfilled) or if you avoid numeric indices for objects = fragile? That way the index is maintained until rendered and we have our solution to "implicit index as key".

Then I think key (or rather, an instance-identifying key) could simply look like: (imagine all instances being stored in a global map)

instanceKey = {
  creator: ReactInstance
  key: null (CreatorLocalImplicitIndexKey) | CreatorLocalKeyString | ReParentableGlobalUID
}

The only thing missing from that definition (if you can parse my intention) I think is that derivative descriptors wouldn't be able to control the life-time of children, at all, but would that actually amount to added value? I'm not entirely sure, but I don't think so. The cool thing however is that instances are re-parentable by default, even when using CreatorLocalKeyString, regardless of which instance actually renders them in the end. When using ReParentableGlobalUID the descriptor would also be re-parentable.

(EDIT: Forgot to mention, by that definition, keys are creator/instance-local, not "parent-local")

I need to cool off a bit and think about this some more, but again, this makes sense this instant, so that's always something. :)

...

I also think it would be interesting to not only consider re-parenting, but the idea of being able to keep non-rendered instances from being destroyed. Imagine: being able to return/wrap descriptors so that they are not actually mounted, but not destroyed either. Perhaps always better solved by visually hiding though (but some form of it may make sense for re-parenting, e.g., when waiting for a re-parenting target to "become" ready)... maybe. :)