Assign and reuse element key by child index

[aduth]

This pull request assigns a key to avoid warnings when the array return value of nodeListToReact is rendered as children into a React element. It does so by assigning a key using the index of the child in its parent. Currently this behavior is only applied when using nodeListToReact, but nodeToReact could potentially emulate this by finding its own index within the parent. It also tries to reuse the key by assigning a private-ish property to the DOM node itself. This is in an effort to try to help React reconcile changes more easily, since after all, this is the purpose of the key prop in the first place.

See: https://facebook.github.io/react/docs/lists-and-keys.html#keys

[aduth]

One potential issue is that if we try to render a concatenation of two arrays returned by nodeListToReact, React will collapse them and log a warning:

Warning: flattenChildren(...): Encountered two children with the same key, `0`. Child keys must be unique; when two children share a key, only the first child will be used.
    in div

Minimal proof-of-concept:

ReactDOM.render(
	[
		...nodeListToReact( someNodes, React.createElement ),
		...nodeListToReact( someOtherNodes, React.createElement )
	],
	target
);

Not sure what the best solution here would be. Maybe we prefix with an incrementing or another value guaranteed to be unique for each nodeListToReact call? Might also be a good idea to also make it distinct from other uses (e.g. a key of "1" is likely common).

[aduth]

To address #1 (comment), 7f62a0c updates logic to assign key as a module-specific prefix combined with a globally incrementing counter, e.g. _domReact0, _domReact1, etc.

[aduth]

An incrementing counter could be an issue if the number of processed children were to ever exceed Number.MAX_SAFE_INTEGER. I'm not sure how realistic this is. We could always revisit with a more robust solution that guards against this.

@iseulde Do you have any thoughts on these changes? There's a few framework-level changes in the Gutenberg repository related to treating children as array that are blocked by this.

[ellatrix]

I'm not sure if I fully understand node._domReactKey. So we modify the nodes that are given to us, and next time reuse the keys? How does React do this?

[aduth]

React uses keys in its virtual representation of the DOM, before ever touching the DOM, to help decide what operations needs to be applied.

We're working in the opposite direction though. You're right that the idea with applying a key as a property to the node itself is for reuse. Take an example:

const listItemNodes = document.querySelector( 'ul' ).childNodes;
const app = document.getElementById( 'app' )

function render() {
	ReactDOM.render(
		React.createElement(
			'ul',
			null,
			...nodeListToReact( listItemNodes )
		),
		app
	);
}

setInterval( render, 1000 );

Here, if we'd not reused keys on the DOM nodes, React would outright replace the set of rendered list items every single time render is called because the return value of nodeListToReact would assign an entirely different set of keys each time.

We'd actually abused this behavior to our advantage in the Editable component with tagName changes.

The above documentation link also references this article which might help clarify a bit more: https://facebook.github.io/react/docs/reconciliation.html#recursing-on-children

[ellatrix]

So what if the node did change, but the key is still the same? Will React know that it needs to re-render? Will it detect the changes, even though the key is the same?

[ellatrix]

I can try it :)

[aduth]

Yes, key is more of a hint as a means to address the issue they highlight in the article about how if we were to insert a new item at the beginning of a list without keys, all children would be mutated, when a more efficient approach is to simply prepend the new node. The rest of the reconciliation still occurs, so even on repeated renders using the same key, if the props have in fact changed, they'll be accounted for.

[ellatrix]

Alright, let's merge