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

Closed
jeanlauliac opened this Issue Sep 9, 2014 · 29 comments

Comments

Projects
None yet
@jeanlauliac
Copy link
Contributor

jeanlauliac commented Sep 9, 2014

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.

@jeanlauliac jeanlauliac changed the title null props not considered differently in getDefaultProps vs. isRequired null props considered differently in getDefaultProps vs. isRequired Sep 9, 2014

@zpao

This comment has been minimized.

Copy link
Member

zpao commented Sep 9, 2014

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

This comment has been minimized.

Copy link
Contributor

jeanlauliac commented Sep 10, 2014

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

This comment has been minimized.

Copy link
Member

sebmarkbage commented Sep 10, 2014

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

This comment has been minimized.

Copy link
Contributor

HurricaneJames commented Nov 7, 2014

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

This comment has been minimized.

Copy link
Contributor

HurricaneJames commented Nov 7, 2014

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

This comment has been minimized.

Copy link

jasonkuhrt commented Nov 21, 2014

Ha, full circle:

screen shot 2014-11-21 at 10 35 48 am

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

This comment has been minimized.

Copy link

jasonkuhrt commented Nov 21, 2014

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

This comment has been minimized.

Copy link

jasonkuhrt commented Nov 21, 2014

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

This comment has been minimized.

Copy link
Member

sebmarkbage commented Nov 21, 2014

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

This comment has been minimized.

Copy link

jasonkuhrt commented Nov 21, 2014

@sebmarkbage isDefined cool, better than isRequiredOrNull.

@HurricaneJames

This comment has been minimized.

Copy link
Contributor

HurricaneJames commented Feb 16, 2015

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

This comment has been minimized.

Copy link

mull commented Mar 20, 2015

Is this being discussed anymore?

@HurricaneJames

This comment has been minimized.

Copy link
Contributor

HurricaneJames commented Mar 20, 2015

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

This comment has been minimized.

Copy link

mull commented Mar 20, 2015

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

This comment has been minimized.

Copy link

tomatau commented May 1, 2015

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

This comment has been minimized.

Copy link

jarsbe commented May 7, 2015

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

This comment has been minimized.

Copy link

SillyButt commented Jun 1, 2015

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

@glortho

This comment has been minimized.

Copy link

glortho commented Aug 19, 2015

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

This comment has been minimized.

Copy link

koulmomo commented Oct 30, 2015

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

This comment has been minimized.

Copy link
Member

sebmarkbage commented Oct 30, 2015

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

@koulmomo

This comment has been minimized.

Copy link

koulmomo commented Oct 30, 2015

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

This comment has been minimized.

Copy link

jDeppen commented Nov 4, 2015

+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

This comment has been minimized.

Copy link

idMolotov commented Dec 8, 2015

+1 for isDefined / isRequired

@milj

This comment has been minimized.

Copy link

milj commented Jan 5, 2016

+1. Null value should be treated as defined.

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Jan 5, 2016

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

This comment has been minimized.

Copy link
Contributor

tmcw commented Jan 6, 2016

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

@wardpenney

This comment has been minimized.

Copy link

wardpenney commented Mar 28, 2016

+1 for isDefined / isRequired

@aweary

This comment has been minimized.

Copy link
Member

aweary commented Sep 19, 2017

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: facebook/prop-types#110

@aweary aweary closed this Sep 19, 2017

@jharris4

This comment has been minimized.

Copy link

jharris4 commented Sep 20, 2017

If anyone's still interested in isDefined / isRequired, there's a PR for it here: facebook/prop-types#90

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment