Don't wrap values with ReactTextComponent in traverseAllChildren (cloneWithProps)

[syranide]

Fix for #1962

Moves wrapping with ReactTextComponent to flattenChildren instead, where it belongs.

Also very minor code fix-up. Also worth mentioning that the flattening done by cloneWIthProps messes with the reactids, so while the idea is to avoid unnecessary allocations I assume, it does affect outcome (i.e, you can't conditionally use cloneWithProps without all components remounting).

[syranide]

cc @zpao @sebmarkbage, this seems straight-forward to me, unless you have other intentions (perhaps we really should just make cloneWithProps use its own traversal code).

[sebmarkbage]

This is kind of difficult to follow and under-parenthesized. Can we break this apart a bit so that it's easier to read?

Looks logically correct but it would be interesting to see if the order matters for performance here. I.e. if the type string/number/object check matters on real code.

[syranide]

      } else if (type === 'string' || type === 'number' ||
                 children.type && children.type.prototype &&
                 children.type.prototype.mountComponentIntoNode) {

Is a lot more logical perhaps, otherwise a bit more dramatic (but not as fast, not that it should be measurable):

      } else {
        var isComponent = (
          children.type && children.type.prototype &&
          children.type.prototype.mountComponentIntoNode
        );
        if (type === 'string' || type === 'number' || isComponent) {
          ..
        } else if (type === 'object') {
          ..

Any preference? (I've updated according to the first code as it's a lot better at least)

[sebmarkbage]

Love this. @zpao, if this passes our internal test process, can we pull this in?

[syranide]

@sebmarkbage Quick poll on your thoughts of #1989
https://github.com/syranide/react/commit/52d96624c3100c28312c0ccc6982aca87bf284a3#diff-5697274a51c77d8781a739f192ceca25 (check last file)

It's a super-simple fix that merges adjacent ReactTextComponents and neatly isolated in flattenChildren (it's possible to hide the var in traverseContext to make it re-entrant, but that should never ever matter).

[syranide]

Rebased

[sebmarkbage]

Will pull this in on Monday. Ping me if I forget.

[syranide]

@sebmarkbage You forgot to remind me to remind you :)

[zpao]

Failing lint because these names (nextName, nextIndex) are used above. Perhaps just define above the whole conditional.

[syranide]

@zpao Fixed and fixed. Just shout if there's anything else.

I can't run the tests for some reason, so I'll just defer to travis for now.

[zpao]

Thanks. Tests pass and this doesn't seem to have broken any of our other test workflows, so there shouldn't be any more issues :)