Seems like a lot changed internally during the React 15 upgrade. I've not been able to find an implementation that works across all versions. Using this.type() doesn't work for all use cases, nor does findDOMNode(), so I had to use a conditional.

I think we need to duplicate all of these tests for undefined and false, which are also things that are "rendering nothing" that we would probably want this function to behave identically for. Looking at the code, I don't think this will pass right now.

The other question for me is the name. isNull() is pretty directly indicating that we are testing for null, which might not always be the case (other things can return ReactNodeTypes.EMPTY).

That said, I'm not really sure what the right name is. Maybe it should be isEmpty() or isNothing()?

Perhaps something like hasRenderedEmpty() or hasRenderedNothing()?

Ok, I changed it to isEmptyRender(), to be a bit more explicit. I also added false based tests and tried to add undefined, but supposedly you can't, as React errors with "Invariant Violation: Unexpected node: undefined" or "Error: Invariant Violation: Foo.render(): A valid ReactComponent must be returned. You may have returned undefined, an array or some other invalid object.".

Hey @milesj , you're right. My bad - I guess undefined is not valid here.

We also want to ensure that this returns false in the proper cases though.

false => true
null => true
true => false
0 => false
<div /> => false
<Foo /> => false

At the moment, isEmptyRender() { return true; } would pass all of these tests :)

I think at that point this will be ready to merge. Not super psyched about the name, but can't think of anything better at the moment.... so at least it's explicit.

I was a bit surprised about the undefined thing as well. I could of sworn it allowed it at some point.

I'll look into adding some truthy tests.

Don't forget about NaN and the empty string, just to make sure the other falsy values are covered (both should return false i believe)

@milesj any updates on this? 😄

Oops, thanks for reminding me. Been pretty swamped at work. Will try to get it this week.

No worries, don't feel pressured/rushed! Thanks for the contribution, looking forward to seeing how this turns out 👍

Added a ton of return value checks.

@lelandrichardson I added tests that check null/false, as well as React components/elements. I originally had it testing every possible falsey value, but then it just barfed all over the output, as seen by the screenshot. I don't really see the benefit of testing all the falsey values as React doesn't allow anything besides null/false.

@milesj I think we should only be testing valid "empty" return values 👍

What about NaN, and the empty string? Does React complain about those too?

I tested a ton of values, all of which are: null, false, undefined, NaN, array, empty array, object, empty object, zero, string, empty string, React element/component, DOM element, etc.

All of them threw those warnings excluding null/false/React.

Screenshot I took.

LGTM

@lelandrichardson Anything else you'd like done on this? Besides hitting the "Update Branch" button :P

@milesj can you squash this to get read of all those merge commits? I think we can merge this after that 👍

Wouldn't Github's new merge squash feature suffice here?

I've tried rebasing the history a bit, but it's turned out troublesome since the commits happened at different points in history. I'll mess around some more.

Does Github do that now? I didn't realize, thats neat. I'll merge. Thanks again @milesj!

(merge-squash is fine when the PR author has issues with rebasing, but in general we do strongly prefer rebased and non-squash-merged commits. github's squash-merge breaks the history between the PR and master)

Yeah, GitHub's automatic "Update Branch" is useful, but annoying history wise. Will probably do it manually going forward.