-
Notifications
You must be signed in to change notification settings - Fork 46.5k
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
null props considered differently in getDefaultProps vs. isRequired #2166
Comments
Yes, we intentionally treat
I think the proptypes warning is a bit confusing here. It seems like cc @sebmarkbage who talks to me all the time about types and could surely form an opinion if he doesn't already have one. |
Agreed |
It's true that It would open up some other confusing scenarios. E.g. |
Hey, I just ran across this problem today and found this issue. Is this really the logic we want to use for React? I frequently hit JSON APIs that return Perhaps Otherwise, we need to use state to guarantee a default value for a lot of things that should have been props. Which is fine, but a bit heavy. Am I completely off base here? It would not surprise me if I am. |
It was just pointed out by a colleague that the logic could go the other way just as easily.
|
Ha, full circle: Pretty much echoing what is being said here. Suggestions:
This accepts a getDefaultProps: function() {
return {
x: ( this.props.x === null ? null : 'foo-default' )
}
} Just use a functional utility when the above becomes too tedius: getDefaultProps: function() {
return allowNull(this.props)({
x: 'foo-default'
}
}) |
If we care about being backwards compatible my suggestion needs to be tweaked.
|
As a work-around for now:
Thanks to Glenjamin on IRC for providing this! |
I agree that null should be a valid value even if it's required. I don't think that the breaking change is too bad since it's more lenient. It wouldn't spam you. We could introduce nullable types We could also rename Our goal is to align with Flow, which doesn't really model this distinction clearly right now. So any change in the type system should also be brought to http://flowtype.org |
@sebmarkbage |
I was curious if there was a decision on @jasonkuhrt 's suggestion to make |
Is this being discussed anymore? |
I do not believe it is. Back in February I tried patching a fork of React.js to make components treat null the same as undefined. The change is fairly simple, but there are several tests saying they specifically want null to override the defaults. Personally, I think this one tiny part of React.js is crazy, but I'm certain FB has an internal use case. In the end I found it easier to rewrite my internal ajax/json library to automatically replace |
I don't know, to me it's fine if it accepts |
Bump! To specify the absence of a property should be explicit per property... E.g. a func would prefer to specify a noop, a string might want either null or an empty string, a number may even want to specify NaN as an absence.. This is easily possible with getDefaultProps... until Null is a complicated language feature... The semantics around this need to be made more clear as debugging against null when getDefaultProps are specified is a pain point - especially when you don't have full control over the data source. I would prefer to see a |
A bit of a different scenario but if anyone finds this useful... I wanted null to have a default prop.
If null is passed in null gets used rather than the default prop. That makes sense since null is not an undefined value. To get the default value I just did the following, explicitly send in undefined if the url is null.
|
@jarsbe Thank you so much! I has problems with default props and I just used undefined as you did. |
Published a quick Thanks to @jasonkuhrt and Glenjamin (IRC) and everyone else on this thread. EDIT: This actually doesn't work. |
can I get clarification on whether it is safe to access |
No, |
Is there a recommended workaround for supporting returning default props when an optional prop is I currently have an API that returns nested
I can think of two workarounds that do not require changes in how
module.exports = React.createClass({
displayName: 'Foo',
getProps: function getProps() {
return {
bar: this.props.bar || 'baz'
};
},
render: function render() {
var props = this.getProps();
return (<div>{props.bar}</div>);
}
}); |
+1 for isRequiredOrNull (or similar) I'm using a custom validator because I want to ensure a prop is specified but I don't care if it's null
|
+1 for isDefined / isRequired |
+1. Null value should be treated as defined. |
I wouldn't expect there to be further changes to PropTypes. Flow has become much more mature recently, and from what I heard from the React team, it is the longer term solution to type checking. This puts PropTypes into the same "compatibility" bucket in terms of priorities—like createClass or React addons, they are still supported, but only with bugfixes and performance improvements, without adding new features or changing the API. (Note: this is not an official position, but my impression of it based on conversations in other threads.) |
In the case a
null
argument is passed to a property marked asisRequired
:http://jsfiddle.net/jeanlauliac/0n6snb6b/1/
We'll get a proper warning in the console:
Warning: Required prop
namewas not specified in
Hello.
(though it should probably be "prop was null" and not "prop was not specified", but anyway)On the other hand, the default value is only used when the prop is
undefined
, but not when it'snull
. Is this the explicitly wanted behavior? If it is, then we should probably make the documentation explicit about it (http://facebook.github.io/react/docs/reusable-components.html), giving the entire responsibility to component callers of safeguarding againstnull
.The text was updated successfully, but these errors were encountered: