Allow iterables in traverseAllChildren

[leebyron]

This lets you use any kind of iterable as a container of react child elements. It also preserves keys when possible if the iterable is keyed.

• Use an ES6 Map or Set to hold children.
• Use an Immutable-js collection to hold children.
• Use a function* which yields children.

Concretely, this removes the necessary conversion to JS objects if you're mapping over Immutable-js collections to get children, i.e.:

<div>
  {myImmutable.map(val => <span>{val}</span>).toJS()
</div>

Now we can remove the toJS() and the intermediate allocation along with it.

[leebyron]

cc @sebmarkbage @zpao @spicyj

[syranide]

Testing to ensure that map keys and implicit index keys work as intended would be great IMHO... a while since I last looked at traverseAllChildren so my memory is fuzzy, at first glance it isn't obvious to me that the final key is actually computed correctly for iterables.

[syranide]

value: 0 is ignored. NVM, this is the entry, but why do we need this test? .done should be all that is needed.

[leebyron]

This is really just a defensive safety check for bad iterators. By the spec, entry should always be a [k, v] tuple unless done is true, however I don't want React reconciliation to die if someone passes in a shitty non compliant entries iterator.

Since this check is cheap, I figured it was better to err on defensive.

[syranide]

Sounds sane, but then the same safety check should be applied for the "set iterables" below right? PS. Or perhaps it just doesn't matter because it will be undefined anyway... so yeah, ignore me :)

[leebyron]

No, because if the iterable is not returning entries then its returning any value, and traverseAllChildren will do the right thing with it. The value iteration below should mirror the Array loop above.

[syranide]

Yeah I realized that just after I wrote it 👍

[syranide]

@sebmarkbage Does "drop support for Objects" include Maps, or was it just Objects because those are a bad idea?

[leebyron]

I just updated this diff to fix a problem (that I added a test for).

Strings are iterable, so the order of conditionals was off in my original diff, it would have iterated over the characters in a string. - I needed to check for iterable only after every other specific case has failed, but before doing object key iteration.

Also added in suggestions from @syranide

[sebmarkbage]

You just made it impossible for future specs to use the name .entries on Array.prototype ever. Hm... Actually this might be a good feature test... Not sure...

[leebyron]

A couple reasons I think this is safe.

1. The Array.isArray() happens before this, so I'm not sure Array.prototype.entries is a concern here.

2. ES6 does add Array.prototype.entries and it would behave correctly if for whatever reason we removed the Array.isArray check above for ES6 browsers.

> a = [1,2,3]
[1, 2, 3]
> a[Symbol.iterator] === a.entries
false
> a[Symbol.iterator]().next()
Object {value: 1, done: false}

3. I have no other ideas for differentiating entry iterators of [k,v] tuples from iterators where the values happen to be arrays.

Example:

> a = [[div,div],[div,div]]
[Array[2], Array[2]]
> a[Symbol.iterator]().next()
Object {value: Array[2], done: false}

vs Map:

> m = new Map([['foo', div], ['bar', div]])
Map {"foo" => "div", "bar" => "div"}
> m[Symbol.iterator] === m.entries
true
> m[Symbol.iterator]().next()
Object {value: Array[2], done: false}

[sebmarkbage]

@syranide Objects should be replaced by Maps.

[sebmarkbage]

Actually, this also needs to work with the key validation. Can you add something here?

react/src/core/ReactElementValidator.js

Line 166 in 5d3b12b

function validateChildKeys(component, parentType) {

[leebyron]

Updated to include ReactElementValidator

[sebmarkbage]

A test for the validator would be nice:

react/src/core/__tests__/ReactElement-test.js

Line 180 in 631b6a0

it('warns for keys for arrays of elements in rest args', function() {

[leebyron]

will do

[leebyron]

Moar tests

[worklez]

Very nice to have this implemented, thanks. But there are still places that expect Array:

• React.PropTypes.node validation will fail if passed an Iterable (e.g. Immutable.List),
• ReactFragment.extract will be called and produce a warning when rendering Iterable.

Are these something to be fixed?

[sophiebits]

@worklez Feel free to file issues or PRs for those.

[aruberto]

I ran into same issue as @worklez and I've submitted #4209 to fix the issue with React.PropTypes.node validation when using iterable as children.