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

Components returning undefined rather than empty string for false #74

Closed
ShaneDrury opened this issue Oct 2, 2017 · 13 comments
Closed
Labels

Comments

@ShaneDrury
Copy link

ShaneDrury commented Oct 2, 2017

Hey, I'm not sure if this is a bug but there seems to be some inconsistency between the code and described behaviour.
In the recent version 3.0 upgrade Issue #67, one of the breaking changes is: "Components returning null or any falsy value are now rendered as an empty string in snapshots instead of null"

But that doesn't seem to be what's happening in the code.

A simple failing case:

const MyComponent = ({ foo }) => {
  return (
    foo && <div>Yep</div>
  );
};

it("truthy case", () => {
  const myComponent = shallow(
    <MyComponent foo />,
  );
  expect(toJson(myComponent)).toMatchSnapshot();
});

it("falsey case", () => {
  const myComponent = shallow(
    <MyComponent foo={false} />,
  );
  expect(toJson(myComponent)).toMatchSnapshot();
});

gives

exports[`truthy case 1`] = `
<div>
  Yep
</div>
`;
exports[`falsey case 1`] = `undefined`;

where I would expect falsey case to return the empty string.
I think this is because we're using lodash's isNil to check return values here

if (isNil(node)) {

which "Checks if value is null or undefined." but not false: https://lodash.com/docs/4.17.4#isNil

Is using isNil desired behaviour? Thanks for your time

@adriantoine
Copy link
Owner

Hi @ShaneDrury!

As you say I'm checking for null and undefined values, but not all falsy values. Although it seems weird that it give

exports[`falsey case 1`] = `undefined`;

it should give

exports[`falsey case 1`] = `false`;

I will have a look but I think I will stick to an empty string for null and undefined only as you might want to see if you component returns false sometimes.

@FezVrasta
Copy link

FezVrasta commented Oct 13, 2017

I'm still reproducing something similar (or the same?) with 3.1.2

<div styleName="CarouselDot__inner">
  {!!icon && <Icon styleName="CarouselDot__icon" name={icon} />}
</div>

When icon is evaluated as false the snapshot will read:

<div
  className="CarouselDot__inner"
>
  <undefined />
</div>

If I change it to {icon ? <Icon styleName="CarouselDot__icon" name={icon} /> : null} it works properly.

The weird thing is that my whole project makes use of renders of this kind, and only this component gives me this problem.

Ideas?

@adriantoine
Copy link
Owner

Hey @FezVrasta, that's my bad, I haven't released a new version including the fix. I'll get v3.1.3 out asap.

@adriantoine
Copy link
Owner

@FezVrasta v3.1.3 is out!

@FezVrasta
Copy link

FezVrasta commented Oct 17, 2017

Thank you!

@FezVrasta
Copy link

FezVrasta commented Oct 17, 2017

Nope, no way... I'm getting the same problem of before even with 3.1.3... I double checked it 😓

Note that my component doesn't return just false, it returns something like: <div>{false}</div>

@adriantoine adriantoine reopened this Oct 17, 2017
@adriantoine
Copy link
Owner

@FezVrasta I will have a look now.

@FezVrasta
Copy link

I'm going to send a PR to add a failing test, if I manage to get one.

@adriantoine
Copy link
Owner

Don't worry I just reproduced the issue so I have a failing test.

Thanks for reporting the issue by the way.

@FezVrasta
Copy link

Oh, I just finished the PR lol.

nevermind then 😇

@adriantoine
Copy link
Owner

Haha sorry, good news is that I fixed it already.

@adriantoine
Copy link
Owner

@FezVrasta it should be fixed in v3.1.4 🎉

@FezVrasta
Copy link

It works ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants