Skip to content
This repository has been archived by the owner on Dec 16, 2021. It is now read-only.

fix: children.map when new child return null #461

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ishenli
Copy link

@ishenli ishenli commented Feb 6, 2018

ref: #419

When the new child is null, should ignore , like React

let Children = {
map(children, fn, ctx) {
if (children == null) return null;
children = Children.toArray(children);
if (ctx && ctx!==children) fn = fn.bind(ctx);
return children.map(fn);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not return children.map(fn).filter(item => item !== null);

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when the children has true or false, React will ignore them, see the demo

// demo
let instance = (
    <div>
      {'123'}
      {true}
      {false}
    </div>
);
class Hello extends React.Component {
  render() {
    console.info(instance.props.children); //  ["123", true, false]
    const ChildrenMap = React.Children.map(instance.props.children, item => item);
    console.info(ChildrenMap); // ["123"]
    return <div>
     {ChildrenMap}
    </div>
  }
}
ReactDOM.render(<Hello/>, document.getElementById('test'));    

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, i see we need to skip boolean and null, undefined

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't it only skip rendering?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, react does much more in React.Children.Map, but I think skip rendering is the the main difference between react-compat and react. The main purpose of this pull request is to improve compatibility.

Copy link
Member

@developit developit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm good merging this, but I'd like the loop converted to a for loop for size reasons. I can do it after merge though!

@developit developit added this to To Do in Backlog via automation Feb 23, 2018
@developit developit moved this from To Do to In progress in Backlog Feb 23, 2018
@developit developit added this to the 3.19 milestone Feb 23, 2018
@likezero
Copy link

likezero commented May 10, 2018

Hi,@developit can you merge this PR?

@developit
Copy link
Member

developit commented May 25, 2018

I'm still confused how it's possible to end up with props.children containing values like null or undefined. Preact strips those out - see this demo:
https://jsfiddle.net/developit/z97ua3m3/

So it would seem that, regardless of whether this PR is merged or not, Preact will still have removed the empty values prior to map() running, right?

Setting that aside, I think this could be simplified a bit? Something like:

	map(children, fn, ctx) {
		if (children == null) return null;
		children = Children.toArray(children);
		if (ctx) fn = fn.bind(ctx);
		return children.filter(Boolean).map(fn).filter(Boolean);
	}

@maxsbelt
Copy link

@developit children property can contain null values. I created simple example for preact@8.3.1: https://codesandbox.io/s/wwnx5q1277?expanddevtools=1

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Backlog
  
In progress
Development

Successfully merging this pull request may close these issues.

None yet

5 participants