-
-
Notifications
You must be signed in to change notification settings - Fork 148
fix: children.map when new child return null #461
base: master
Are you sure you want to change the base?
Conversation
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); |
There was a problem hiding this comment.
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);
There was a problem hiding this comment.
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'));
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this 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!
Hi,@developit can you merge this PR? |
I'm still confused how it's possible to end up with So it would seem that, regardless of whether this PR is merged or not, Preact will still have removed the empty values prior to 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);
} |
@developit children property can contain |
ref: #419
When the new child is null, should ignore , like React