null props considered differently in getDefaultProps vs. isRequired

[jeanlauliac]

In the case a null argument is passed to a property marked as isRequired:
http://jsfiddle.net/jeanlauliac/0n6snb6b/1/

We'll get a proper warning in the console: Warning: Required prop namewas not specified inHello. (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's null. 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 against null.

[zpao]

Yes, we intentionally treat undefined and null differently, as does JS. Basically, reading the spec definitions is our explanation.

4.3.9 undefined value
primitive value used when a variable has not been assigned a value

4.3.11 null value
primitive value that represents the intentional absence of any object value

I think the proptypes warning is a bit confusing here. It seems like isRequired should allow null since it has intention. However isRequired is equivalent to saying your type is non-nullable (which means no null nor undefined). So we don't really have a way of saying "any defined value which includes null but not undefined". Maybe we should but I don't really know how to express that.

cc @sebmarkbage who talks to me all the time about types and could surely form an opinion if he doesn't already have one.

[jeanlauliac]

Agreed null should be semantically intentional in opposition to undefined. I think that make sense to guard against null as well though, just as non-nullable types (eg. in Hack); so as to avoid if (foo !== null) boilerplate in render(). There could be some kind of .isRequired.isNonNull but I can guess changing the behavior of isRequired would be too much of breaking change.

[sebmarkbage]

It's true that isRequired should only check for undefined, not null. That way it's possible to explicitly allow a null value as a valid value. This could be important in a union type.

It would open up some other confusing scenarios. E.g. null is a renderable but a required renderable would still allow null (it already allows false which is equally confusing).

[HurricaneJames]

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 null. Also, DOM methods (ex. document.querySelectorAll) return null when they find nothing. This means that we have to include substantial, ugly null-checking code somewhere if we really do not want null to override the default value.

Perhaps isRequired is a good solution. If isRequired is set, then some value is required, period. No nulls, no undefineds, some value must be present (we could argue about empty strings, but that seems a step too far). At some point even React realized null was not a valid value because isRequired throws the warning. If we want to allow null, then we just leave off isRequired.

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.

[HurricaneJames]

It was just pointed out by a colleague that the logic could go the other way just as easily.

isRequired could act as a flag that allows null. Under this logic, the flag specifies that a value is required, that there should not be a default, and that null could fulfill that value. However, when isRequired is not set, then null would trigger the default value.

[jasonkuhrt]

Ha, full circle:

Pretty much echoing what is being said here.

Suggestions:

isRequired

• value may be anything except undefined
• if value is undefined for whatever reason (be it the key does not exist or the value is explicitly undefined) then log the warn message

isRequiredValue

• value may be anything except undefined OR null
• if value is undefined for whatever reason (be it the key does not exist or the value is explicitly undefined) OR is null then log the warn message

getDefaultProps

This accepts a function which returns an object which allows users an escape hatch to specially treat this.props.x === null. Therefore by default React SHOULD overwrite null values with given defaults. If a user really wants to honour null values then this is doable:

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'
  }
})

[jasonkuhrt]

If we care about being backwards compatible my suggestion needs to be tweaked.

isRequired would remain unchanged and the new isX semantic would gain a new name like isRequiredOrNull or whatever.

[jasonkuhrt]

As a work-around for now:

React.PropTypes.oneOfType([React.PropTypes.oneOf([null]), React.PropTypes.any).isRequired

Thanks to Glenjamin on IRC for providing this!

[sebmarkbage]

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 PropTypes.Nullable(PropTypes.string).isRequired.

We could also rename isRequired -> isDefined.

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

[jasonkuhrt]

@sebmarkbage isDefined cool, better than isRequiredOrNull.

[HurricaneJames]

I was curious if there was a decision on @jasonkuhrt 's suggestion to make getDefaultProps override null as well as undefined? As pointed out, it is possible to keep null in the suggested solution if it is really desired, but it is currently not possible to override it if you do not.

[mull]

Is this being discussed anymore?

[HurricaneJames]

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 null with undefined when fetching data.

[mull]

I don't know, to me it's fine if it accepts null - what bothers me is that's the only behaviour if you want to require a prop. Perhaps ugly, but something like PropTypes.string.notNull although with a better name would work just fine. An alternative to isRequired simply.