[Children] filter null values in map function

[yordis]

Currently I am implementing some Dropdown component and I have something like this.

return (
  <div className={classnames(classes)} tabIndex="-1">
    {Children.map(children, this.render_child)}
  </div>
)

...

@autobind
render_child(element, index) {
  return cloneElement(element, {
    key: element.key || index,
    selected: this.props.selected,
    onClick: this.click_handler
  })
}

The business requirement put me in this situation. I created the list of item of the Dropdown component.

// this is using map from some array so I will return an array.
render_menu_item(key) {
  if(!key) { //whatever checking
    return
  }

  return (
    <MenuItem value={key}>{text}</MenuItem>
  )
}

Now, because I use element.key in some case element could be null so give me an error. I propose to remove the null or undefined from the Children.map because then I will be filtering all the time the null values whenever I have this situation.

I understand I can forEach but I think is not the best implementation, in the end you change the map function alright so

[yordis]

this is soft of related to #2956

[jimfb]

There are reasonable situations where users might want to have the nulls when mapping over elements, and it seems bad for map to skip elements. Implementing filter as mentioned in #2956 would be the better solution, if we were to do such a thing, but filter does have its own set of issues as mentioned in the bug. Specifically, the effects on reconciliation performance.

Closing as a duplicate of #2956, since that solves this use case and is tracking the use case.

[yordis]

@jimfb can you put me an example when the user want to have null? because people used a lot the null value for render nothing in some function base on some condition, the null or undefined values doesn't add any value to the render, no?

Children is DOM/React element so I don't see the problem with removing it by default like the react-dom does when you try to render a null value

[sophiebits]

The new toArray removes nulls, btw.

[jimfb]

@yordis See syranide's comment: #2956 (comment)

Basically, by preserving the nulls in the array, you preserve the implicit keys that React uses to track instance identity. This ensures that the state of stateful components gets properly preserved when items are added/removed from the list. It's a fairly advanced topic, but it's a valid use case. The fact that nulls aren't removed from map means it "accidentally" works more often for people who aren't familiar with how implicit keys work.

[sophiebits]

@jimfb That's not a real concern since things get rekeyed by React.Children.map.

[yordis]

@spicyj thanks a lot about the toArray solutions it's good to know but even that I am still without get why the map doesn't do it for me. Again pointing to my comment about rendering null elements. He closed because he think the better implementation is use filter but I am not agree with that. I don't know, I am asking for the same behavior that React render does when you pass an array with null values.

[jimfb]

@spicyj Since React.Children.map rekeys, do we want to reopen? My intuition is we should retain nulls, since that's how array.map works and I don't see adding a null-check as that burdensome, but I did close under the understanding that retaining nulls was advantageous for reconciliation but apparently that's not necessary, so I'm fine with reopening if you think this is something we might do.

[sophiebits]

Sure, we can. It's hard to imagine what could break. I guess it would be kind of weird if you have a component like <LeftRight>{a}{b}</LeftRight> if a/b could be null.

[yordis]

@spicyj In my situation I cloned the children and because I have null values I can't do element.key (example) so I have to check for that element. You alright it do weird stuff with Children.map so I don't see any problem with that. React when you pass some null values it does nothing with it. That's why it's common to se something like:

<div>{this.renderItem()}</div> // could be `null` or an element.

renderItem() {
  if(anyCheckingIsTrue) {

    return <h1>Great</h1>
  }
}

// This is a map that give you some `null` or `undefined` values
let list = this.filters()
// and then
<Test>{list}</Test>

class Test extends from Component {

  render() {
    return (
      <div>
        {Children.map(this.props.children, this.renderItems)}
      </div>
    )
  }

  renderItems(element, index) {

    // we want to do something with that element
    // this is clean in my opinion, but I have to do the check
    return cloneElement(element, {
      key: element.key || index, // this will fail if we dont put the check when element is null
      onClick: this.itemClick
      ...
    })
  }
}

[softberry]

I am agree to @jimfb that 'null' items are valid and should not be taken out through Children.map function. This is not what map function should do. We have Children.map function for looping though a specific Children list and apply a function against each child. So, map is not responsible of rendering something but formatting children list.

Developers (reactjs users) may need to render null children differently other than not null children . I mean null does't always mean "not to be rendered" but may be "rendered differently".
Forexample,

<div class='simple-item'> element is valid </div>
<div class='not-found'> element is null</div>
<div class='simple-item'> element is valid </div>
<div class='simple-item'> element is valid </div>
<div class='not-found'> element is null</div>
<div class='simple-item'> element is valid </div>

In my opinion is that, this is not a bug and "works as intended" . Web Developers should take this account and design their solutions accordingly.

For example for the initial snippet from @yordis ,

@autobind
render_child(element, index) {
if(element===null) { element=emptyElementToBeRendered ; }
  return cloneElement(element, {
    key: element.key || index,
    selected: this.props.selected,
    onClick: this.click_handler
  });
}

[sophiebits]

I agree there is value in removing nulls, but React.Children.toArray does remove nulls already (so you can use that), and I don't believe it's worth the churn to change Children.map and .forEach given that many people may have code that relies on the current behavior.

In general we don't see React.Children as a best practice going forward since it is hard to make use out of it without reimplementing a lot of the rules of React by hand and inevitably making APIs that don't consistently work as expected. Going forward hopefully we can document some of the alternatives (like createContext) more.

Closing this.

[danielo515]

Going forward hopefully we can document some of the alternatives (like createContext) more.

Sorry for the resurrection but, how is createContext an alternative to children inspection?