When updating from many-item to single-item array, re-render is forced although item key hasn't changed

[gaearon]

Consider this:

render: function () {
  return <div>{arr.map(this.renderItem)}</div>;
}

Usually, key is respected in this case and elements don't get unmounted and remounted unnecessarily if their keys match between subsequent render calls.

However, if all elements but one are removed, React 0.10.0 for some reason doesn't realize it's the same component (the key hasn't changed) and forces its DOM node to update.

I tried setting a breakpoint inside shouldUpdateReactComponent and _updateChildren to debug the issue and it looks like prevComponentInstance is null because its name in nextChildren and prevChildren is slightly different (in one case it includes leading 0:, in the other it doesn't).

Anyway, the workaround for me was, curiously, not returning an array when there is just one item, and rendering it as is. In this case, React was able to understand it's the same component, and didn't force an unnecessary re-render.

I'm sorry for not providing a workable fiddle or something—jsFiddle hosts an old version of React, and it's 8am where I live so I should go to sleep. This should be fairly simple to reproduce, unless it's already fixed.

[sophiebits]

I can't immediately repro this. You can see from the source of traverseAllChildren.js that the intent is certainly to treat a single child as the same as an array:

https://github.com/facebook/react/blob/master/src/utils/traverseAllChildren.js#L121-L124

If you could post a jsfiddle or jsbin that would be helpful. In either case, it shouldn't be hard to use 0.10.0 or a nightly from http://react.zpao.com/builds/master/latest/.

[syranide]

Tried to repro it myself but can't: http://jsfiddle.net/kb3gN/2231/, however there exists a somewhat unintuitive problem: http://jsfiddle.net/Pa5Ud/, if you wrap all of those 3 cases in another array, logic dictates that the result should be the same, but it isn't. I would assume that this is the problem you're seeing, although I'm thinking there is no possible traverseAllChildren-fix for it (other than removing the one-child special-case in React, which might be unlikely).

[sophiebits]

Closing, but @gaearon if you can repro let me know.

[gaearon]

@syranide You're correct, the problem was due to a nested array (I didn't realize I was actually putting one array inside the other).

The simplest repro: http://jsfiddle.net/EMeQ2/1/

Your repro is exactly how I came to this problem: I accidentally introduced an extra depth to each inner component, and the “one item” special case broke because of it.

@spicyj

This is by design, right? It makes sense that key is only taken into account exactly at the same “depth” inside the array. If we tried to flatten the arrays, nested keys could clash.

But maybe we could document this in Keys section? It's not immediately obvious that key will work with nested arrays, but only when the depth/position matches exactly.

[syranide]

@gaearon Yeah, this is an unfortunate side-effect of an optimization; if a component is given just one child then children is points directly to that child, but if it's given multiple children then children is an array of children.

In-order for this optimization to successfully reuse the first child traverseAllChildren (constructs the node IDs) ignores the outer-most array, if there is one, so that going from one child to many children doesn't affect the ID. However, as you probably can imagine, this can only be applied to the outer-most array if we want array-depth to matter. So as I mentioned, {this.props.children} behaves differently to {[this.props.children]} even though logic says it shouldn't. I haven't looked but I assume the same issue is present for {{items: this.props.children}}.

AFAIK they've made a very intentional decision to go this way as it avoids an unnecessary allocation in cases with single children. But it makes the behavior of arrays and objects as children really rather unintuitive.

Thinking about the problem briefly, the only solution I see (while keeping the optimization) would be to add some simple logic that can detect when an entry goes from being a component to an (array of) array of components or vice versa. Updating the ID in this case is currently not feasible, but given #1570 + another PR or two and might be. But I'm not convinced that this would be a good idea.

[gaearon]

@syranide

I'm not sure I understood you correctly so let me rephrase it.

Can React warn me when the same node definition in JSX “jumps” between levels?
Like here:

render: function () {
    var node = <div />;

    if (this.state.condition) {
      return node;
    } else {
      return <div>node</div>;
      // or return [node]
    }

This is useful for cases when I, as a novice user, expect React to understand the node is being moved up/down a level, whereas React doesn't support this behavior. This would also have catched the previous use case with different level in nested arrays.

If this is what you are suggesting, what are the challenges that make it difficult to implement now? What are potential false positives?

[syranide]

@gaearon Keys in React are only considered unique/reusable within the container they're rendered inside <div>, <span>, [], {}, etc. So <div><Com key="x" /></div> will not see <Com key="x" /> reused anywhere but inside <div></div> at that same spot, it cannot jump and there exists no understanding of things moving around without keys, it's either reused or destroyed. Put them inside another div, span, array or object and it is unmounted + destroyed and a new instance will be created + mounted in the new place.

Key-less components in React are given an implicit key that is equal to its index among the children of the parent. Meaning that <span /><div /> will see none of the two components reused if rendered as <div /><span /> the next time. Since React.DOM.span !== React.DOM.div and their keys (which is their index) has changed, both will be destroyed and recreated. Looking at the data, there exists no information to suggest that they are related and should switch places.

[gaearon]

@syranide

I see, so these are the limitations of the current implementation.

From your PRs I have seen, you are trying to change how React finds node's component, and plan to get rid of data-reactid. Would this eventually help the reconciler to understand if a node is being moved? What kind of “a few more PRs” are necessary to solve this?

It would have been awesome if React not just warned, but also recognized “move up/down an array/container” as a valid operation.

[syranide]

@gaearon I think you misunderstood my intent, what I described above is not a flaw in React, it is how it's designed. <span /><div /> is just as related to <What /><Ever /> as it is to <div /><span />, that is to say, not at all. Any relation is inferred by you, it cannot be derived from the data contained within the previous and next snapshot of the hierarchy that React sees.

If you don't explicitly add a key to a descriptor, React will explicitly set it to the index of that component among the children:
<span key="A" /><span /><span /><span key="B" />
Is behavioraly identical to:
<span key="$A" /><span key="1" /><span key="2" /><span key="$B" />

So considering that we render the following next:
<span key="B" /><span /><span key="C" /><span />
Again, it would be behaviorally identical to:
<span key="$B" /><span key="1" /><span key="$C" /><span key="3" />

So given that we now have a snapshot of the previous and next hierarchy, as React sees it:

Prev: <span key="$A" /><span key="1" /><span key="2" /><span key="$B" />
Next: <span key="$B" /><span key="1" /><span key="$C" /><span key="3" />

The following will happen:

1. 2 and $A will be destroyed (does not exist in next).
2. 1 and $B will be reused (exists in prev and next).
3. 3 and $C will be created (does not exist prev).

When dealing with nesting such as:
<span><span><span key="A" /></span><span key="B" />{[[<span key="C" />]]}</span>
React sees the following:
<span key=".0"><span key=".0.0"><span key=".0.0.$A" /></span><span key=".0.B" /><span key=".0.0:0:C" /></span>

[gaearon]

@syranide

Ah okay. I misunderstood this paragraph, can you elaborate on it?

Thinking about the problem briefly, the only solution I see (while keeping the optimization) would be to add some simple logic that can detect when an entry goes from being a component to an (array of) array of components or vice versa. Updating the ID in this case is currently not feasible, but given #1570 + another PR or two and might be. But I'm not convinced that this would be a good idea.

But still, I have a few questions.

Any relation is inferred by you, it cannot be derived from the data contained within the previous and next snapshot of the hierarchy that React sees.

At the risk of suggesting something very stupid..

React cannot see it, but JSX generator can. Could we possibly solve this in JSX generator, or are we constrained by keeping it a simple syntax transform? I'm talking about

render: function () {
    var node = <div />;

    if (this.state.condition) {
      return node;
    } else {
      return <div>node</div>;
      // or return [node]
    }
}

being translated to something like

render: function () {
    var node = React.DOM.div({ uniqueKey: React.nextUniqueKey() });

    if (this.state.condition) {
      return node;
    } else {
      return React.DOM.div({ uniqueKey: React.nextUniqueKey() }, node);
      // or return [node]
    }
}

The (huge?) downside is sacrificing the simplicity of the transform, the upside is being able to reason about node moving up or down in the hierarchy.

[gaearon]

Oh wait, this is totally wrong. In fact JSX generator would need to generate these IDs itself at compile time. This really looks too much of bookkeeping to place on a now-transparent generator so probably not a good direction.

Still, I wouldn't mind it if a separate (optional?) tool processed my JSX files and amended calls to component constructors with an extra “unique-as-in-source-file” key that React could recognize to better reason about nodes in runtime.

[syranide]

@gaearon

Given <Root><span /></Root>, React sets this.props.children= <span />
Given <Root><span /><span /></Root>, React sets this.props.children = [<span />, <span />]

If alternating rendering one and then the other (without the special-case), only <Root> would be reused, because it would technically look like this:

<Root key=".0"><span key=".0.0" /></Root>
<Root key=".0"><span key=".0.0:0" /><span key=".0.0:1" /></Root>

But we want the first child to be reused, so React must ignore the first array when constructing the ID to compensate for the fact that this.props.children can be either a component or an array of components (the optimization). But this workaround does not work if this.props.children is inserted into an array, array of arrays, etc. Makes any sense?

As for your examples of uniqueKey, inferring the identity of something by it's location in the source does not work well in general, imagine a helper-function in the component that creates and returns a button. All such buttons would then have the same uniqueKey. Although I have proposed something similar some time ago, being able to provide a keyScope of sorts (or perhaps a special key object that also has a scope reference), a component could then move around freely if the scope was used. But the response wasn't very enthustiastic back then. :)

[gaearon]

@syranide

I understand now, thanks for taking time to explain!

imagine a helper-function in the component that creates and returns a button

Yeah, right.

But the response wasn't very enthustiastic back then. :)

Is there some discussion on the web that I can read?

[syranide]

@gaearon No problem :)

IIRC there's no log for the discussion, it was mostly a quick idea on my part and not really thorougly evaluated / detailed. If this is something you feel strongly about and can make a case for, I say it's worth "giving it a shot". I think it makes a lot of sense from a theoretical perspective, it's basically the only feature that "manual construction/destruction of components" can boast, but React can't.

[syranide]

This is actually avoidable after-all #2378