React: 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 propnamewas 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:

• 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

• 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

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.

[tomatau]

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 doesn't trigger this default and suddenly we have to use defensive programming throughout our components.

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 React.PropTye.__.allowNull so that this can become more clear and explicit behaviour per property with minimal boilerplate and clear semantics. Then getDefaultProps can be triggered by default for any prop that doesn't match the type being validated against...

[jarsbe]

A bit of a different scenario but if anyone finds this useful...

I wanted null to have a default prop.

<Avatar src={user.avatar_url} />

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.

<Avatar src={user.avatar_url || undefined} />

[SillyButt]

@jarsbe Thank you so much! I has problems with default props and I just used undefined as you did.

[glortho]

Published a quick nullable prop type helper here: https://gist.github.com/jedverity/23715563d2b74d0ae5bb

Thanks to @jasonkuhrt and Glenjamin (IRC) and everyone else on this thread.

EDIT: This actually doesn't work. oneOf( [ null ] ) might work on its own but null can't get past isRequired.

[koulmomo]

can I get clarification on whether it is safe to access this.props inside of getDefaultProps as an escape hatch with regards to null as described here?

[sebmarkbage]

No, this.props is not available inside getDefaultProps because it gets called before there even is a this.

[koulmomo]

Is there a recommended workaround for supporting returning default props when an optional prop is null?

I currently have an API that returns nested JSON structures with null values where I ideally would want defaults. To avoid

having to write repetitive and fragile code

— react docs

I can think of two workarounds that do not require changes in how getDefaultProps behaves (if there are no plans to have getDefaultProps handle this use case the workaround would of course become permanent).

1. manually parse the API response replacing null values with undefined where appropriate
2. adding a custom getProps method which I call in my render function to fetch a local props var.

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

[jDeppen]

+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

customProp: (props, propName, componentName) => {
  // allow null but ensure propName is specified
  if ( props[propName] === undefined ) {
    return new Error('Validation failed!');
  }
}

[idMolotov]

+1 for isDefined / isRequired

[milj]

+1. Null value should be treated as defined.

[gaearon]

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

[tmcw]

Interesting - is there a current alternative to getDefaultProps based on Flow types?

[wardpenney]

+1 for isDefined / isRequired

[aweary]

Since PropTypes is no longer part of React core, I'm going to close this out. I've opened an issue for this in repo for the prop-types package: https://github.com/facebook/prop-types/issues/110

[jharris4]

If anyone's still interested in isDefined / isRequired, there's a PR for it here: https://github.com/facebook/prop-types/pull/90