Skip to content
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

Can we have React.Children.filter? #2956

Closed
gaearon opened this issue Jan 27, 2015 · 34 comments
Closed

Can we have React.Children.filter? #2956

gaearon opened this issue Jan 27, 2015 · 34 comments

Comments

@gaearon
Copy link
Member

@gaearon gaearon commented Jan 27, 2015

My use case: wrapping all non-empty children into divs.

  render() {
    return (
      <div style={rootStyle} onWheel={this.handleWheel}>
        {Children.map(this.props.children, this.wrapChild)}
      </div>
    );
  },

  wrapChild(child, index) {
    if (!child) { // I know this is probably wrong kind of comparison; don't care
      return (
        <div style={this.getChildStyle(index)}>
          {child}
        </div>
      );
    }
  },

This is all nice but I also need to know how many valid children I have wrapped.
Children.count will return the number of children including the “empty” children. I want to strip them out.

Can we have Children.filter?

@gaearon
Copy link
Member Author

@gaearon gaearon commented Jan 27, 2015

Another way to solve this is to give us Children.toArray.

@gaearon
Copy link
Member Author

@gaearon gaearon commented Jan 27, 2015

Solved my problem with react/lib/flattenChildren. Wish it were exposed/supported though.

@syranide
Copy link
Contributor

@syranide syranide commented Jan 29, 2015

I may be wrong, but I think this is generally problematic unless React.Children.filter would replace filtered children with null or undefined rather than compact the array (which you can/should do with map). If the filter is in any way dynamic and the children aren't keyed then the reconciliation will suffer badly.

@gaearon
Copy link
Member Author

@gaearon gaearon commented Jan 29, 2015

Right, I see. What about exposing flattenChildren?

@syranide
Copy link
Contributor

@syranide syranide commented Jan 29, 2015

flattenChildren returns a keyed object, this is deprecated (keyed objects) and will not be supported soon AFAIK (Map will be, but then you'd have to polyfill or wait for ES6).

@brigand
Copy link
Contributor

@brigand brigand commented Jan 29, 2015

Also see #2932 which would solve this.

@gaearon
Copy link
Member Author

@gaearon gaearon commented Jan 29, 2015

So the proper way of counting non-empty children is doing a Children.forEach and incrementing a counter?

@brigand
Copy link
Contributor

@brigand brigand commented Jan 29, 2015

If you do need to implement extra functions like this you can bend map to implement other functions due to how JS works. Here's an example in jsbin/gist.

var cutils = utilsFromMap(React.Children.map);
var countNonEmpty = function(children){
    return cutils.reduce(function(acc, x){
        // assumes non-empty means 'truthy'
        return x ? acc + 1 : acc;
    }, 0, children);
}
@ofersadgat
Copy link

@ofersadgat ofersadgat commented Feb 7, 2015

You can implement your own Children.toArray:

var children = [];
React.Children.forEach(this.props.children, function(child){
    if (!child){
        children.push(child);
    }
});
@arendjr
Copy link

@arendjr arendjr commented Apr 7, 2015

syranide wrote:
I may be wrong, but I think this is generally problematic unless React.Children.filter would replace filtered children with null or undefined rather than compact the array (which you can/should do with map). If the filter is in any way dynamic and the children aren't keyed then the reconciliation will suffer badly.

Maybe I'm missing something, but I don't see the problem exactly. Could you elaborate?

FYI: I want to implement a Tabs component, and I thought it would be nice idea to define the individual tabs using nested Tab components. But besides tabs, the tabs header can also contain buttons. So I'd get a structure like this:

<Tabs>
    <Tab key='tab1' label='First tab'>/* tab content */</Tab>
    <Tab key='tab2' label='Second tab'>/* tab content */</Tab>
    <TabButton /* props */ />
</Tabs>

In the Tabs component itself I would now like to know which children are tabs and which are buttons. So it'd be great it I could do this:

var tabs = React.Children.filter(this.props.children, function(child) { /* test if it is a tab */ });
@syranide
Copy link
Contributor

@syranide syranide commented Apr 7, 2015

@arendjr I'd call that bad practice, Tab and TabButton are unrelated and should be provided through separate props, not as a mixed soup of children.

@jimfb
Copy link
Contributor

@jimfb jimfb commented Apr 7, 2015

@syranide I'm not sure I agree that this is bad practice. It seems perfectly reasonable to me that a parent component might want to have arbitrary behaviors based on their children. In particular, if components want to preserve the absolute ordering of the child components, there isn't really any other way to do it (do the tabs come before/after the tab buttons? or do they alternate? or are they otherwise mixed?). I agree that's not what he is doing here (he doesn't appear to be preserving order), but I think it's a completely valid use case. You could imagine someone implementing a "smart div" that does fancy layout things, and treats some components differently from others (obviously it's natural to have a mixed bag of children).

Given the simplicity of @ofersadgat's implementation, I'm not convinced it makes sense to pollute the API space by providing a filter function.

@syranide I agree reconciliation would suffer. Maybe a React.Children.filter would warn if children do not have keys? That is almost a reason to provide this function, to add a warn hook.

It's probably a separate issue, but as @gaearon suggested, it seems like there should be a natural way to get children as a (potentially empty) array. Children.toArray might be a nice solution to this issue.

cc @spicyj

@syranide
Copy link
Contributor

@syranide syranide commented Apr 7, 2015

I'm not sure I agree that this is bad practice. It seems perfectly reasonable to me that a parent component might want to have arbitrary behaviors based on their children

@JSFB A very broad subject it seems to me, processing children is not inherently bad, I think it depends on what you're trying to accomplish. If children need to be separated out it's likely that they should be separate props to begin with or the should be API redesigned somehow,

You could imagine someone implementing a "smart div" that does fancy layout things, and treats some components differently from others (obviously it's natural to have a mixed bag of children).

Sure I can imagine that, but I'm not quite sure when that actually makes practical sense and if there aren't better solutions to the problem. Inventing non-standard behaviors on a per-component basis seems like it should be a last resort, to be used only after you've concluded that established practices isn't a good fit. It shouldn't be all-or-nothing, just because children can be used and looks pretty, doesn't mean they should.

The solution could imaginably be <Tabs tabs={[...]} />, it's no longer an opaque structure and the special behavior likely apparent to the user as well (just saying). If you ask me, each tool should be used only for its intended purpose, children has a purpose too and I think it's a mistake to expand that definition unless we're sure it makes sense.

@jimfb
Copy link
Contributor

@jimfb jimfb commented Apr 7, 2015

I agree that it depends on what you're trying to accomplish and that it's likely they should be separated out if you're going to try to filter them anyway. But if someone desires a heterogeneous list of components (usually due to ordering concerns), I think that's a perfectly valid use case, and 'children' may be a natural place for those to be defined. We should be providing reasonable support for that use case.

At Facebook, it's common to branch based on the type of a child. Popularity at Facebook doesn't necessarily imply that it's a best practice, but it is a common use case that we do support. I don't know what percentage of those use cases are doing a filter operation (I imagine it's not a large percentage, because presumably the reason to use a heterogeneous list is to maintain order).

Having said that, I'm still not entirely convinced this feature (React.Children.filter) makes sense / justifies the API creep. The only real advantage I see is that it gives us an opportunity to warn about unkey'd children.

@sophiebits
Copy link
Collaborator

@sophiebits sophiebits commented Apr 8, 2015

It's true that Facebook has existing components that branch based on the type (or position) of a child, but it's usually a better idea to pass separate components explicitly. Instead of

<LeftRight>
  {left}
  {right}
</LeftRight>

you can do

<LeftRight left={left} right={right} />

which is more explicit in addition to being easier to implement. Going back to the original issue, we were considering implementing some sort of React.Children.toArray which clones children and re-keys them appropriately. For example

React.Children.toArray([
  [<div key="monkey" />, <span key="gorilla" />],
  <input key="gorilla" />
])

would return something like

[
  <div key="0.$monkey" />,
  <span key="0.$gorilla" />,
  <input key="$gorilla" />,
]

which is more or less how React flattens children internally. At that point you could manipulate the array in whatever way is most convenient.

@arendjr
Copy link

@arendjr arendjr commented Apr 8, 2015

React.Children.toArray() sounds good!

@kidwm
Copy link

@kidwm kidwm commented Dec 27, 2015

const {children} = this.props;
children.slice(-1).concat(children, children[0]).map(
 (item, index) => <li key={index}>{item}</li>
) 

BTW, how to do children concat like this in proper way?

@sophiebits
Copy link
Collaborator

@sophiebits sophiebits commented Dec 27, 2015

We have React.Children.toArray now.

@kidwm Not sure I understand what you're trying to do there.

@sophiebits sophiebits closed this Dec 27, 2015
@kidwm
Copy link

@kidwm kidwm commented Dec 28, 2015

@spicyj I don't know how to solve the same key problem after the use React.Children.toArray to concat, so I keep it in this way and assign key by index.

@sophiebits
Copy link
Collaborator

@sophiebits sophiebits commented Dec 28, 2015

I don't know what key problem you're trying to solve. If you explain it maybe I can help.

@kidwm
Copy link

@kidwm kidwm commented Dec 28, 2015

@spicyj Thank you very much!

I've tried to do in this way:

const {slides} = React.Children.toArray(this.props.children);
React.Children.map(slides.slice(-1).concat(slides, slides[0]),
 (item) => <li>{item}</li>
)

but didn't know how to address this problem: Warning: flattenChildren(...): Encountered two children with the same key

@sophiebits
Copy link
Collaborator

@sophiebits sophiebits commented Dec 29, 2015

Something like this would work:

const slides = React.Children.toArray(this.props.children);
const slidesA = slides.map((el) => <li key={'A-' + el.key}>{el}</li>);
const slidesB = slides.map((el) => <li key={'B-' + el.key}>{el}</li>);
const wrappedSlides = slidesA.slice(-1).concat(slidesB, slidesA.slice(0, 1));

though exactly which keys you should assign to which elements depends on how you want to preserve each child's state/identity.

@kidwm
Copy link

@kidwm kidwm commented Dec 29, 2015

@spicyj if it was possible to alter the key with React.cloneElement, maybe that would be simpler?

const cloneChildren = React.Children.map(this.props.children, (child, index) => React.cloneElement(child, {key: index}));
@sophiebits
Copy link
Collaborator

@sophiebits sophiebits commented Dec 29, 2015

Yes, that also works. But you rarely want the index as key.

@kidwm
Copy link

@kidwm kidwm commented Dec 29, 2015

@spicyj Thanks again, this is my final code:

const slides = React.Children.map(this.props.children, (child) => React.cloneElement(child, {key: child.key + '_clone'}));
React.Children.map(slides.slice(-1).concat(this.props.children, slides.slice(0, 1)),
 (item) => <li>{item}</li>
)
@fernandopasik
Copy link

@fernandopasik fernandopasik commented Apr 28, 2016

I'm doing this by wrapping the Children utility:

import { Children } from 'react';

Children.filter = function (children, filterFn) {
  return Children
    .toArray(children)
    .filter(filterFn);
};

export default Children;

And using it like this so it's a bit shorter in my component code

import Children from '../utils/children';
Children.filter(props.children, item => 'header' === item.type)
@sophiebits
Copy link
Collaborator

@sophiebits sophiebits commented Apr 28, 2016

I'd suggest copying the Children object instead of mutating it so it's clearer when you're using your util and when you're not.

@fernandopasik
Copy link

@fernandopasik fernandopasik commented Apr 28, 2016

Thanks for the advice @spicyj !

Here's the code updated:

import { Children } from 'react';

export default Object.assign({}, Children, {
  filter(children, filterFn) {
    return Children
      .toArray(children)
      .filter(filterFn);
  }
});
@catamphetamine
Copy link
Contributor

@catamphetamine catamphetamine commented Mar 1, 2017

@spicyj

It's true that Facebook has existing components that branch based on the type (or position) of a child, but it's usually a better idea to pass separate components explicitly.

Not always.
For example, I've got a dropdown menu which wraps each child into an <li/> and when encountering null it just throws an exception (presumably in React.Children.map).
Why does React.Children.map even have those empty children lol.
Broken design as for me.
And React.Children.toArray(children).map() doesn't work because is somehow messes keys

@fernandopasik
Copy link

@fernandopasik fernandopasik commented Mar 1, 2017

Hi!

I've made a quick small library with more functions for Children structure here

https://github.com/fernandopasik/react-children-utilities/blob/master/src/index.js

There are also recursive ways to do find, map and more stuff. Check them out, and let me know if there are more I can add.

Thanks!

@Sweater-Baron
Copy link

@Sweater-Baron Sweater-Baron commented Oct 19, 2017

I know this issue is old and closed, but I got here while googling how to do this, and thought I'd leave a note for posterity, since I found an easy way:

You can use React.Children.map to approximate filter if you just return nothing when the condition is false, and return the child when the condition is true:

filterChildren(children, conditionTester) {
    return React.Children.map(children, child => {
        if (!conditionTester(child)) {
            return;
        }
        return child;
    });
}
instructure-gerrit pushed a commit to instructure/instructure-ui that referenced this issue Dec 20, 2017
See this for more details: 
facebook/react#2956 (comment)

Test plan:
* Try to render:
<List>
 {someFalsyCondition &&
   <ListItem>List item 1</ListItem>
 }
 {anotherFalsyCondition
    ? <ListItem>List item 2</ListItem>
    : null
 }
 <ListItem>List item 3</ListItem>
</List>
* it should not blow up

Change-Id: Ib854a7fc207067ab24baf387496c954a9a6b5874
Reviewed-on: https://gerrit.instructure.com/136263
Tested-by: Jenkins
Reviewed-by: Jennifer Stern <jstern@instructure.com>
Product-Review: Jennifer Stern <jstern@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
instructure-gerrit pushed a commit to instructure/instructure-ui that referenced this issue Jan 3, 2018
See this for more details: 
facebook/react#2956 (comment)

Test plan:
* Try to render:
<List>
 {someFalsyCondition &&
   <ListItem>List item 1</ListItem>
 }
 {anotherFalsyCondition
    ? <ListItem>List item 2</ListItem>
    : null
 }
 <ListItem>List item 3</ListItem>
</List>
* it should not blow up

Change-Id: Ib854a7fc207067ab24baf387496c954a9a6b5874
Reviewed-on: https://gerrit.instructure.com/136263
Tested-by: Jenkins
Reviewed-by: Jennifer Stern <jstern@instructure.com>
Product-Review: Jennifer Stern <jstern@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
@maciej-gurban
Copy link

@maciej-gurban maciej-gurban commented Feb 27, 2018

You can always just do:

const children = React.Children.map(this.props.children, child => child);

// and then

children.filter(someCondition);
children.some(anotherCondition);

or attach these directly on React.children().

AEgan pushed a commit to AEgan/reactjs.org that referenced this issue Aug 16, 2018
This adds to the `React.Children.toArray` documentation to mention that
it will filter out invalid elements.

I needed to filter out invalid child elements in an application I am
working on. When searching for the best appraoch, I came across an old
github issue facebook/react#2956, and found
that the discussion led to the creation of `React.Children.toArray`. I
didn't realize that `toArray` would do this, even after reading the docs
for it, and thought it would be worthwhile to include for the next
developer looking for it.
AEgan pushed a commit to AEgan/reactjs.org that referenced this issue Aug 16, 2018
This adds to the `React.Children.toArray` documentation to mention that
it will filter out invalid elements.

I needed to filter out invalid child elements in an application I am
working on. When searching for the best appraoch, I came across an old
github issue facebook/react#2956, and found
that the discussion led to the creation of `React.Children.toArray`. I
didn't realize that `toArray` would do this, even after reading the docs
for it, and thought it would be worthwhile to include for the next
developer looking for it.
@jlurena
Copy link

@jlurena jlurena commented Aug 15, 2019

I know this issue is old and closed, but I got here while googling how to do this, and thought I'd leave a note for posterity, since I found an easy way:

You can use React.Children.map to approximate filter if you just return nothing when the condition is false, and return the child when the condition is true:

filterChildren(children, conditionTester) {
    return React.Children.map(children, child => {
        if (!conditionTester(child)) {
            return;
        }
        return child;
    });
}

Wouldn't this give nulls in your array?

@dmitrirussu
Copy link

@dmitrirussu dmitrirussu commented May 20, 2020

import MyComponent from './..'

filter(children, (child) => { child.type === MyComponent; })

Also if Children is FC component then there you can find out the Chilld.type.displayName also could be used for filtring.... the exmaple above works if Wrapper is a React.Component for children...

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

Successfully merging a pull request may close this issue.

None yet