Treat [undefined] as valid PropTypes.renderable

[gasi]

• React.PropTypes.renderable: null vs undefined #1647: React no longer issues a warning when passing in [undefined] as React.PropTypes.renderable, hence treating it the same as [null].

Notes

• Tests pass.
• Lint reports many warnings but not due to newly added code.
• Previously signed CLA.

[chenglou]

@sebmarkbage

[sophiebits]

No need to update the changelog here; we'll do it when we do a release (and organize it in a way that hopefully makes more sense than the individual commit bullets points).

[gasi]

👍 Removed CHANGELOG entry.

[gasi]

Removed CHANGELOG entry and the minimal changes are now ready for review.

[sebmarkbage]

If you start passing undefined values in to a renderable slot you can no longer have default values for that same slot. This is a potential refactoring hazard.

It's also easy to accidentally return undefined by missing a return case in a function.

function render() {
  if (x) return <Foo />;
  if (y) return <Bar />;
  // undefined
}

Failing this type check helps prevent such errors.

I can see that it's a pain to check for the existence of a value. :(

<Foo component={this.props.component || null} />

This does forces you to make the choice if you want to use the default prop or explicitly null.

You can always make a renderable prop optional which is suppose to be the same as undefined.

[mathieumg]

What about: {optionalBoolean && <MyComponent />}?

If optionalBoolean is undefined this will return undefined. Is the "solution" here to do this {optionalBoolean ? <MyComponent /> : null}?

[sophiebits]

@sebmarkbage We should at least make [<a />, <b />, undefined] pass the renderable check, no?

[gasi]

Yes, I agree with @spicyj to allow [<a />, <b />, undefined] and I believe my changes enable exactly that. The case you describe is still covered and a simple undefined (outside of […] array) will still throw an error:

react/src/core/__tests__/ReactPropTypes-test.js

Lines 387 to 398 in 927eb57

	it('should warn for missing required values', function() {
	typeCheckFail(
	PropTypes.renderable.isRequired,
	null,
	'Required prop `testProp` was not specified in `testComponent`.'
	);
	typeCheckFail(
	PropTypes.renderable.isRequired,
	undefined,
	'Required prop `testProp` was not specified in `testComponent`.'
	);
	});

[sebmarkbage]

Ideally it would be the same behavior for top-level and nested.

You can implement default props as ES6 destructuring which could introduce similar issues at the array level.

There's also a problem if you don't have defaults and don't have explicit prop types in intermediate components. This could warn you about undefined values even without explicit propTypes:

<Wrapper>{this.props.left}{this.props.right}</Wrapper>

It's disturbing that it's relaxing the type check when it's nested.

Reopening for proper discussion.

@mathieumg An optional boolean can have an explicit default prop. Makes it a little bit easier to reason about the actual value if it's only true/false.

[sophiebits]

@sebmarkbage Isn't this a reasonable piece of code to write in render?

var left;
if (showLeft) {
  left = <Left />;
}

return (
  <div>
    {left}
    <Right />
  </div>
);

I guess that's not 100% the same as what we're talking about here, but it's close.

[sebmarkbage]

@spicyj Not really, no. It looks like an accidental missing else statement. I'd probably lint against that. var left = null; on the top line would be reasonable.

But even then, that's a mutable structure. I'd probably prefer the const form.

[gasi]

Thanks for reopening for discussion, @sebmarkbage. I definitely want to find a solution that allows for more use cases of renderable without harming existing checks.

You can implement default props as ES6 destructuring which could introduce similar issues at the array level.

Can you give an example?

It's disturbing that it's relaxing the type check when it's nested.

If you believe that, shouldn’t [null] fail the type check as well then?

In the original example I gave in #1647, what style would you recommend to address that undefined in an array is not a valid renderable?

[gasi]

Any update on this?

[sebmarkbage]

I think the most convincing use case for me is the case where you have an optional property.

<Foo child={this.props.childOrUndefined} />

This might be reason enough to allow this.

However the main issue is that this resolves differently for undefined and null:

var { childOrUndefined = <Foo /> } = this.props;

Therefore passing undefined may actually change behavior of underlying classes in confusing ways. Seems like a little bit of boilerplate to ensure which one you mean seems like a decent way to resolve this issue and ensure that refactoring doesn't cause unforeseen bugs.

[sebmarkbage]

Closing this out again because I think that making it optional is enough. Will follow the lead from Flow here and align with Flow once it settles. I still think undefined needs to be treated differently from null here.