Add an `isEmptyRender()` method to both `ShallowWrapper` and `ReactWrapper` #339

milesj · 2016-04-20T19:02:10Z

Why

Added an isNull() method to both ShallowWrapper and ReactWrapper,
making it easier in tests to detect when a component renders nothing;
either a null or undefined value.

I chose the name isNull() since isEmpty() was used for other situations.
It's not as descriptive as it could be, so open to suggestion.

Tested

Unit tests!

sjdemartini · 2016-04-20T19:04:31Z

src/ReactWrapper.js

+   *
+   * @returns {boolean}
+   */
+  isNull() {


I feel like this name too specifically implies that the component did return null in its render method, even though return undefined (either explicitly or implicitly) should also be fine.

milesj · 2016-04-20T19:54:17Z

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.

lelandrichardson · 2016-04-21T01:27:37Z

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()?

milesj · 2016-04-21T06:26:07Z

Perhaps something like hasRenderedEmpty() or hasRenderedNothing()?

milesj · 2016-04-21T17:19:26Z

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.".

* 'master' of github.com:airbnb/enzyme: Update docs to externalize react/addons for newer versions of React (#338)

lelandrichardson · 2016-04-22T02:18:40Z

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.

milesj · 2016-04-22T03:15:53Z

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.

ljharb · 2016-04-22T05:28:50Z

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)

aweary · 2016-05-16T18:49:56Z

@milesj any updates on this? 😄

milesj · 2016-05-16T19:07:26Z

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

aweary · 2016-05-16T19:41:07Z

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

milesj · 2016-05-16T22:47:19Z

Added a ton of return value checks.

* 'master' of github.com:airbnb/enzyme: (30 commits) Update Jest docs. (#399) DOC: Clarify that .simulate() calls prop (#354) Beef up tests for SFCs and optimize npm test scripts Use public API for batchedUpdates ShallowRenderer supports batched updates grammar.. so trivial, sorry Id selector not working when combined with a tag selector (#314) Fix mount::simulate() signature (#381) Fix documentation code example mistake [lgtm] Fix capitalization of maintainer name, drop approvals to 1 v2.3.0 Add lgtm.co config and MAINTAINERS file. added error checking for state, setState, context, instance methods on ShallowWrapper (#373) equals should skip null and undefined nodes fix #151 (#192) Fix incorrect docs for type add docs related methods for .at() and .get() (#372) [Tests] remove io.js exclusions Add `name` method (#335) Test on node v6 Add loose equals and loose contains functions on ShallowWrapper (#362) ...

making it easier in tests to detect when a component renders nothing; either a null or undefined value. Fixed inconsistencies between React 0.14 and 15. Renamed isNull() to isEmptyRender(). Added tests for false checks. Added itWithData() and a better way to test possible falsey values for React render(). Added tests for components and element return types. Wrapped empty render data in a function. Simplify the empty render data checks.

milesj · 2016-05-19T20:39:25Z

@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.

aweary · 2016-05-19T20:44:27Z

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

ljharb · 2016-05-19T21:10:00Z

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

milesj · 2016-05-19T21:20:43Z

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.

milesj · 2016-05-19T21:22:41Z

Screenshot I took.

aweary · 2016-05-19T23:02:59Z

LGTM

milesj · 2016-05-24T23:01:18Z

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

aweary · 2016-05-24T23:15:03Z

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

milesj · 2016-05-25T00:19:19Z

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.

aweary · 2016-05-25T01:09:37Z

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

ljharb · 2016-05-25T07:32:55Z

(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)

milesj · 2016-05-25T17:42:39Z

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

sjdemartini reviewed Apr 20, 2016
View reviewed changes

ljharb added enhancement semver: minor New stuff. labels Apr 20, 2016

milesj changed the title ~~Add an isNull() method to both ShallowWrapper and ReactWrapper~~ Add an isEmptyRender() method to both ShallowWrapper and ReactWrapper Apr 21, 2016

Merge branch 'master' of github.com:airbnb/enzyme

d9ca7b9

* 'master' of github.com:airbnb/enzyme: Update docs to externalize react/addons for newer versions of React (#338)

Miles Johnson added 2 commits May 19, 2016 12:41

milesj added 2 commits May 24, 2016 10:15

Merge branch 'master' into add-isnull-check

dc1dc1c

Merge branch 'master' into add-isnull-check

411bee2

aweary merged commit 6845529 into enzymejs:master May 25, 2016

milesj deleted the add-isnull-check branch May 25, 2016 03:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an `isEmptyRender()` method to both `ShallowWrapper` and `ReactWrapper` #339

Add an `isEmptyRender()` method to both `ShallowWrapper` and `ReactWrapper` #339

milesj commented Apr 20, 2016

sjdemartini Apr 20, 2016

milesj commented Apr 20, 2016

lelandrichardson commented Apr 21, 2016

milesj commented Apr 21, 2016

milesj commented Apr 21, 2016

lelandrichardson commented Apr 22, 2016

milesj commented Apr 22, 2016

ljharb commented Apr 22, 2016

aweary commented May 16, 2016

milesj commented May 16, 2016

aweary commented May 16, 2016

milesj commented May 16, 2016

milesj commented May 19, 2016

aweary commented May 19, 2016

ljharb commented May 19, 2016

milesj commented May 19, 2016

milesj commented May 19, 2016

aweary commented May 19, 2016

milesj commented May 24, 2016

aweary commented May 24, 2016

milesj commented May 25, 2016

aweary commented May 25, 2016 •

edited

Loading

ljharb commented May 25, 2016

milesj commented May 25, 2016

Add an isEmptyRender() method to both ShallowWrapper and ReactWrapper #339

Add an isEmptyRender() method to both ShallowWrapper and ReactWrapper #339

Conversation

milesj commented Apr 20, 2016

Why

Tested

sjdemartini Apr 20, 2016

Choose a reason for hiding this comment

milesj commented Apr 20, 2016

lelandrichardson commented Apr 21, 2016

milesj commented Apr 21, 2016

milesj commented Apr 21, 2016

lelandrichardson commented Apr 22, 2016

milesj commented Apr 22, 2016

ljharb commented Apr 22, 2016

aweary commented May 16, 2016

milesj commented May 16, 2016

aweary commented May 16, 2016

milesj commented May 16, 2016

milesj commented May 19, 2016

aweary commented May 19, 2016

ljharb commented May 19, 2016

milesj commented May 19, 2016

milesj commented May 19, 2016

aweary commented May 19, 2016

milesj commented May 24, 2016

aweary commented May 24, 2016

milesj commented May 25, 2016

aweary commented May 25, 2016 • edited Loading

ljharb commented May 25, 2016

milesj commented May 25, 2016

Add an `isEmptyRender()` method to both `ShallowWrapper` and `ReactWrapper` #339

Add an `isEmptyRender()` method to both `ShallowWrapper` and `ReactWrapper` #339

aweary commented May 25, 2016 •

edited

Loading