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

Prevent falsy nodes from being counted as children #251

Merged
merged 1 commit into from
Mar 14, 2016
Merged

Prevent falsy nodes from being counted as children #251

merged 1 commit into from
Mar 14, 2016

Conversation

nishitaniyuki
Copy link
Contributor

Like #151

I have a problem like below.

test('component should have no children', t => {
    const component = shallow(
        <div>
            {undefined}
            {undefined}
        </div>
    );
    t.same(component.children().length, 0) // This should be success, but fail since length is 1.
});

I expect null, undefined and false should not be counted as a child node.
What do you think??

const result = [];
React.Children.forEach(maybeArray, child => result.push(child));
React.Children.forEach(filtered, child => result.push(child));
Copy link
Collaborator

Choose a reason for hiding this comment

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

a simpler solution here might be to just check and see if child is false, null, or undefined before pushing it into the result? The recursive nature of children will be taken care of in React.Children.forEach in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly.
Can I rewrite & send new PR?(I'm new to github...)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@nishitaniyuki yep! Update your code and then push the changes to github. this PR should update automatically.

When you update your code, it's best for a change like this if you can make all of your changes into a single atomic commit. Let me know if you need any extra help with this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI, you can change the code and then use git commit --amend to combine the changes into the latest commit so that it stays as a single commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've combined the changes, thank you!
Please check it.

@lelandrichardson
Copy link
Collaborator

Thanks for working on this. I believe we omit these nodes from being passed into methods such as .findWher(), so omitting them here feels more consistent.

@ljharb
Copy link
Member

ljharb commented Mar 13, 2016

We omit null nodes, but not usually all falsy nodes - what about the empty string and NaN?

@lelandrichardson
Copy link
Collaborator

@ljharb i'm not too worried about NaN, but we should have defined behavior for all things that React considers non-renderable: false, null, and undefined.

We explicitly don't traverse false and null. It looks like we might (incorrectly) handle undefined though:

https://github.com/airbnb/enzyme/blob/master/src/ShallowTraversal.js#L32

@nishitaniyuki
Copy link
Contributor Author

i'm not too worried about NaN, but we should have defined behavior for all things that React considers non-renderable: false, null, and undefined.

I agree.

We explicitly don't traverse false and null. It looks like we might (incorrectly) handle undefined though: https://github.com/airbnb/enzyme/blob/master/src/ShallowTraversal.js#L32

I expect this also should skip undefined.

@lelandrichardson
Copy link
Collaborator

@nishitaniyuki great, thanks!

lelandrichardson added a commit that referenced this pull request Mar 14, 2016
Prevent falsy nodes from being counted as children
@lelandrichardson lelandrichardson merged commit c711f13 into enzymejs:master Mar 14, 2016
@nishitaniyuki nishitaniyuki deleted the patch-1 branch November 10, 2016 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants