Skip to content

Add React.Children.toArray #3650

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 11, 2015
Merged

Add React.Children.toArray #3650

merged 1 commit into from
Aug 11, 2015

Conversation

sophiebits
Copy link
Collaborator

There's been some interest in this recently (#2956, mailing list). Unlike the other helpers, this strips nulls per #2393 (comment).

@mjurczyk
Copy link

Should it nullify only undefined/null/bool, or anything that isn't a valid React child (regarding the mailing list) ?

@gaearon
Copy link
Collaborator

gaearon commented Apr 16, 2015

I like this!

@jimfb
Copy link
Contributor

jimfb commented Apr 17, 2015

I like this too!

@sophiebits
Copy link
Collaborator Author

Updated, now skipping nulls per #2393 (comment).

@sebmarkbage?

@syranide
Copy link
Contributor

Unlike the other helpers, this strips nulls per #2393 (comment).

But that means it's (kind of) unsuitable for rendering right? Stripping nulls messes up implicit indicies which is not obvious.

@sebmarkbage
Copy link
Collaborator

This doesn't have implicit indices since they are preresolved into explicit keys.

We're looking at ways we can encode the explicit keys in the same ways as if the toArray function wasn't used or as if they were nested.

@natew
Copy link

natew commented May 13, 2015

Had this discussion on twitter recently: https://twitter.com/natebirdman/status/597879879086968832

Is it really impossible to return a single child element directly in React right now? Seems really weird if so. I have the total giving me 1, and just a single element being passed into a component, but I can't return just this.props.children or React.Children.only(this.props.children). The only thing that works is: this.props.children['.0'] and it gives a lot of warning about opaque types.

Is there a way to avoid this warning? Or a blessed way to return a single child?

@sophiebits
Copy link
Collaborator Author

If it's a single child, this.props.children already works. If it's an array containing one element and you want to extract it, your best bet is probably to use forEach and store away the first child that comes out, something like:

var child;
React.Children.forEach(this.props.children, function(c) {
  child = c;
});

@natew
Copy link

natew commented May 13, 2015

@spicyj thanks! We are shimming React.createElement and I think some babel code is turning it into a single-element array. Wups.

@iammerrick
Copy link
Contributor

Another example use case is a Concatenate component...

import React from 'react';

export default React.createClass({
    getDefaultProps() {
      return {
        separator: (key) => {
          return <span key={key}> &sdot; </span>
        }
      };
    },
    render() {
      var children = _.compact(this.props.children);
      var separated = children.reduce((previous, current, index) => {
        previous.push(current);
        if (index !== children.length - 1) {
          previous.push(this.props.separator(index));
        }
        return previous;
      }, []);
      return <div>{separated}</div>;
    }
  });

@iammerrick
Copy link
Contributor

Not sure how to implement concat with empty elements right now...

@iammerrick
Copy link
Contributor

@spicyj @sebmarkbage Is this pull request dead in the water? Looks like its been a while without being merged.

@jimfb
Copy link
Contributor

jimfb commented Jul 10, 2015

@iammerrick: Both @spicyj and @sebmarkbage are out of the country at the moment, and have limited network connectivity. I think everyone involved would still like to see this get merged, but we're trying to prep a 0.14 release and this is not a high priority item at the moment. I imagine we will revisit this after 0.14 ships. Feel free to ping this issue again after 0.14 ships, as that's probably the best time to restart these discussions.

@sophiebits
Copy link
Collaborator Author

(Hi from Venice!)

I think we want this for 0.14. We wanted to make it so switching from this.props.children to React.Children.toArray(this.props.children) when rerendering does not change the keys of all children, which is currently hard.

sophiebits added a commit that referenced this pull request Aug 11, 2015
Add React.Children.toArray
@sophiebits sophiebits merged commit 6160216 into facebook:master Aug 11, 2015
@ibrahima
Copy link

I realize this is a really old PR, but I was just wondering: what's the reasoning behind stripping out only nulls, and not all invalid elements? For example, why strip null but leave undefined, which is the default return value if a function doesn't return a value? I read the linked comment but it doesn't really clarify why nulls are treated specially and not other invalid element types. This doesn't really matter, since we can always chain .filter(React.isValidElement) after toArray, but I was just curious since they seem conceptually the same to me.

@sophiebits
Copy link
Collaborator Author

It does remove undefined. See the test case labeled null/undefined/bool are all omitted. I'd expect every element in the return value of this function to be a string, number, or React element.

@cycomachead
Copy link

I'm seeing the opposite in a component I wrote (which Ibrahim is reviewing)

But reading through the code, it looks like a null element won't be pushed, but undefined and boolean values are still going to make their way to the new array. result.push() is outside the conditional for isValidElement, and I don't see where else such elements would be filtered.

@ibrahima
Copy link

To add some detail, in our react component some of the children are defined by methods that look like the following:

renderFoo(){
  if(someCondition) {
    return <MyChild/>
  }
}

Playing around in the developer console though, that test case returns what it expects to return, but as @cycomachead said, I don't fully understand why.

(Woah, we did not expect a response, let alone one so quickly!, and from spicyj no less. Thanks!)

@sophiebits
Copy link
Collaborator Author

We have a test case checking that it is excluded so I am pretty sure it is. :)

In particular it is treated the same as null here:

if (type === 'undefined' || type === 'boolean') {
. So maybe you have an issue somewhere else in your code.

@ibrahima
Copy link

Oh cool! So traverseAllChildren is normalizing undefined and booleans to null before passing to the iterator function.

Definitely seems possible that there's an error elsewhere in our code. Thanks for your help, and I hope we didn't bother you with this question!

@cycomachead
Copy link

D'oh, yes, i see it now. Thanks!

There's definitely some issue in our code then, but it's odd that manually adding a .filter() over isValidChildren fixed the issue.
React.Children.toArray(this.props.children).filter(React.isValidElement) works, but
React.Children.toArray(this.props.children)

@sophiebits
Copy link
Collaborator Author

Are you sure it was undefined? Perhaps it was a string or number. If you were reading child.props.foo then that would then give you a "undefined has no property .foo" error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.