Skip to content
This repository has been archived by the owner on Sep 1, 2024. It is now read-only.

Add isDefined and isNotNull parallels to isRequired #90

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jharris4
Copy link

Adds the ability to do the following:

PropTypes.string.isDefined // allows null but not undefined
PropTypes.string.isNotNull // allows undefined but not null

closes #57 and duplicates some work from #59

@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

return null;
if (props[propName] === null) {
return allowNull ? null : new PropTypeError('The ' + location + ' `' + propFullName + '` is marked as required ' + ('in `' + componentName + '`, but its value is `null`.'));
} else if (props[propName] == null) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

== null is the same as === undefined in this case.

I used == null because that's what the old code used, and it avoids referencing the undefined value directly.

I think it's a holdover best practice from older browsers that allow undefined to be assigned a different value:

https://stackoverflow.com/questions/8783510/how-dangerous-is-it-in-javascript-really-to-assume-undefined-is-not-overwritte

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no worries, I make the same mistake all the time. I even use a font that helps me see the difference: https://github.com/tonsky/FiraCode

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

== null also matches both null and undefined, which is the convention in propTypes, so it's not the same as === undefined, by design.

@jharris4
Copy link
Author

@aweary any chance of getting some feedback on this PR?

I'd love to see this merged & released if possible, since there isn't a good workaround for supporting null values without this PR...

@ljharb
Copy link
Contributor

ljharb commented Aug 30, 2017

@jharris4 explicitNull in https://npmjs.com/airbnb-prop-types is a pretty good workaround :-)

@jharris4
Copy link
Author

jharris4 commented Aug 30, 2017

@ljharb thanks for the suggestion, I just had a quick look at the source and tests for explicitNull...

I'm curious, how would you suggest using explicitNull to handle something like PropTypes.String.isDefined (which from this PR would allow "abc" or null but NOT 123 or undefined) ?

@ljharb
Copy link
Contributor

ljharb commented Aug 30, 2017

Using or from airbnb-prop-types also, or([explicitNull, PropTypes.string]).isRequired should do it.

@aweary aweary self-assigned this Aug 30, 2017
@aweary
Copy link
Contributor

aweary commented Aug 30, 2017

@jharris4 I'll try and review soon, thanks for doing this btw!

@th0th
Copy link

th0th commented Sep 14, 2017

Any updates on this? I really don't want to use a whole new package for nullable required props.

@aweary
Copy link
Contributor

aweary commented Sep 15, 2017

@th0th I'm currently the only person working on this repo, and it's not something I can prioritize right now, so I'm sorry that it's been rather slow.

@ljharb if you could give this a review and let me know how it looks to you I would feel better about merging this soon.

@ljharb
Copy link
Contributor

ljharb commented Sep 19, 2017

I'm not really sure this is a good approach; I'd prefer adding something like PropTypes.allOf, and then adding PropTypes.null and PropTypes.undefined, and then PropTypes.allOf(PropTypes.string, PropTypes.null) etc.

@jharris4
Copy link
Author

jharris4 commented Sep 19, 2017

@ljharb Things like allOf or oneOf could be nice additions to PropTypes validation but that's a more complicated feature that's way beyond the scope of this PR.

This PR encompasses the minimal change possible to give people the ability to distinguish between null vs undefined.

This PR is also a non breaking change and keeps the PropTypes package completely backwards compatible with previous versions.

With the current state of the code most of the change that I made in this PR would be required anyway, since there is currently no way to allow null values with isRequired:

if (props[propName] == null) {
  if (isRequired) {
    if (props[propName] === null) {
      return new PropTypeError('The ' + location + ' `' + propFullName + '` is marked as required ' + ('in `' + componentName + '`, but its value is `null`.'));
          }

@ljharb
Copy link
Contributor

ljharb commented Sep 19, 2017

oneOf already exists; I was suggesting allOf to complement it.

I realize it's out of scope of this PR; I'm suggesting that this PR's premise is flawed and it should be closed.

@jharris4
Copy link
Author

I'd argue that the premise of the original code to conflate null with undefined was flawed, and that this PR provides a simple, backwards compatible, non-breaking change, fully tested change to work around that flaw.

I get the sense that there are other users out there who could also benefit from this change to allow them to continue using prop-types with sometimes null prop values. But if nobody else chimes in we can close this PR.

It'd be unfortunate though, because without this PR prop-types can't meet all of my prop validation needs, and I'll have to look for another solution.

I wonder whether the core React team have mostly switched to using Flow for prop type validation (https://flow.org/en/docs/react/components/) and whether that'll be the most officially supported solution moving forward.

@jharris4
Copy link
Author

jharris4 commented Sep 19, 2017

Also with regards to oneOf, without this PR, there would be no way to accomplish this:

PropTypes.oneOf(PropTypes.null, PropTypes.string).isRequired

because of the strict handling done for isRequired in this code:

return createChainableTypeChecker(validate);

However it would work just fine as:

PropTypes.oneOf(PropTypes.null, PropTypes.string).isDefined

@MatthewHerbst
Copy link

@ljharb I'm kind of in the same boat as @jharris4. I agree that your solution makes more sense long-term, but it also requires a major breaking change to how isRequired works. The proposed solution is pretty clean, and doesn't per-se break any existing code patterns. In fact, if I'm reading what you're thinking fully through, I would argue that you support removing isRequired all together in favor of oneOf/allOf with a PropTypes.undefined as well.

@ljharb
Copy link
Contributor

ljharb commented Sep 20, 2017

I'm not sure why it requires a major change at all - I'm not proposing changing how .isRequired works whatsoever.

I'm suggesting solely two semver-minor changes: adding PropTypes.allOf, and PropTypes.explicitNull.

If you insist on wanting to exclude null, then a third semver-minor change: adding PropTypes.noneOf as well.

From those primitives, everything this PR wants can be achieved, with composition.

@jharris4
Copy link
Author

The trouble is that the current implementation of isRequired will not let you do any validation whatsoever if the prop value is null or undefined.

I have dozens of props in some of my apps that I'd like to validate to ensure that their values are always a string or null but never undefined.

The composition idea is a good one, so I'll run with it for a minute.

PropTypes.allOf([PropTypes.not.undefined, PropTypes.String])

Assuming that PropTypes supported negation (which it doesn't), I can't quite get what I want using allOf (correct me if I'm wrong).

PropTypes.noneOf([PropTypes.undefined, PropTypes.not.String])

Similarly with noneOf, I'm not quite seeing how to compose things together to get what I want.

PropTypes.oneOf([PropTypes.null, PropTypes.String])

This seems like the closest composition solution to what I actually want. The trouble is, it allows undefined. But then if I add .isRequired to it, it won't allow null. (i.e. this was the whole motivation for this PR lol).

@ljharb
Copy link
Contributor

ljharb commented Sep 20, 2017

PropTypes.oneOf([PropTypes.null, PropTypes.string.isRequired])

@jharris4
Copy link
Author

That could work, so long as PropTypes.null exists.

I would perhaps generalize it a bit to PropTypes.value so that you can do PropTypes.value(null) or whatever else you want.

@jharris4
Copy link
Author

I just became aware of a detailed issue filed a while back on React that discusses a lot of these points! facebook/react#2166

It's pretty funny to read the comment by @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

@ljharb
Copy link
Contributor

ljharb commented May 22, 2019

Even adding a new top-level propType is a bit of a big deal, since this package gets so much usage - adding a new method to every top-level propType is much much riskier.

I've only been maintaining this package for a few months, and the limited time I've had to invest in it has been spent cleaning up bugfixes and cutting long-awaited releases - which naturally pushes risky additions to the bottom of the list, even if I'd personally enjoy using them.

I hope you, and everyone else, can be patient.

@jharris4
Copy link
Author

Hey @ljharb, first of all thanks for donating your time to maintaining open source packages like this one. It can be a thankless job at times...

If you feel that there might be a way to move this PR forward, could you elaborate on the risks you're referring to?

The existing tests for this library all pass with this PR, and I took the trouble to add new tests to mitigate any risks.

If you feel that more tests would be required to increase your confidence level, I could probably find some time to invest in that.

Also, in case it helps inspire confidence we've got 3 different products using this PR (via forked package) with a combined total of several hundred components, deployed on-premise to about 300 clients, and haven't had a single issue (we use sentry for error tracking).

@damianlajara
Copy link

How is this still open?! LGTM 🚀

@macandcheese
Copy link

macandcheese commented Sep 9, 2019

This is a great PR! A consideration - does a failing PropType for either
isDefined or isNotNull default to a provided DefaultProp? That would be 💯.

@icopp
Copy link

icopp commented Nov 7, 2019

This change is still necessary to cover the simple use case of InferProps with Typescript and being able to pass through props to children that are strictly typed as X | undefined, thus rejecting X | null | undefined from InferProps.

The current workaround looks like this (assuming X is boolean) and is both silly and verbose:

PropTypes.oneOfType([
  PropTypes.bool.isRequired,
  PropTypes.oneOf([undefined]).isRequired,
]).isRequired

@lukescott
Copy link

@icopp The workaround should't work because null and undefined are never passed to a validator. isRequired is a flag (not a validator) that changes the internal behavior of prop-types to emit an error rather than eat nullish values. If it was a validator we wouldn't need this PR and we could solve it ourselves.

The PR doesn't change prop-types behavior (for backwards compatibility), but it adds isNotNull and isDefined as companion flags to isRequired to treat null and undefined separately.

At this point is there a way to completely remove prop-types from React and use our own callbacks? If this package is no longer being maintained and the notion is to "use TypeScript" (which we do), should prop-types import just be removed from core and replaced with something a little more simplistic? Something that can be better extended?

On our team we really don't use prop-types at all anymore and use TypeScript, but I personally see the benefit of having runtime checking of props. The dream for me would be able to just define TypeScript types and have runtime checking in development to guard against badly typed endpoints.

@ljharb
Copy link
Contributor

ljharb commented Jan 16, 2020

@lukescott this package is absolutely still being maintained - a new feature not being added, or taking a long time to be added, quite simply never means a package is unmaintained.

PropTypes and TypeScript can not replace each other; PropTypes is incapable of checking function argument types or return values; TypeScript is incapable of determining basic things like "is it an integer". They should be used together, in concert.

@adamchenwei
Copy link

Why can't this just be added as an "Experimental" feature?

@ljharb
Copy link
Contributor

ljharb commented Jan 18, 2020

That's not a concept that exists in semver. If it's published, it's supported until at least the next major, and that's effectively the same as forever.

@adamchenwei
Copy link

I am more referring to some sort repos feature system in accepting features that community want but the owner of the repo do not want (for whatever reason) and see how this feature is been used and etc. Experimental features even in React library itself... Why can't it be also in prop-type repo? Just thought it can give some oxygen in this tight grip......

@seniath
Copy link

seniath commented Jul 28, 2020

This change is still necessary to cover the simple use case of InferProps with Typescript and being able to pass through props to children that are strictly typed as X | undefined, thus rejecting X | null | undefined from InferProps.

just chiming in to say i also ran into issues with this and InferProps, so it'd be great to get this merged in

@jdhorner
Copy link

I ended up here after realizing that I needed to support a prop that could be both null but also a value, but not undefined. I just realized that the PR has been open for over 3 years! I guess I'll keep my fingers crossed. :)

@ljharb
Copy link
Contributor

ljharb commented Sep 18, 2020

@jdhorner in the meantime, https://npmjs.com/airbnb-prop-types has explicitNull.

@jharris4
Copy link
Author

@jdhorner The explicitNull from airbnb-prop-types only allows null or undefined values (shown by tests here) so likely won't meet your requirements.

The fork of this package with this PR merged is available here if you'd like to use it: https://www.npmjs.com/package/prop-types-defined

@ljharb
Copy link
Contributor

ljharb commented Sep 18, 2020

@jharris4 you'd use explicitNull.isRequired with or or PropTypes.oneOf, so it absolutely would meet everyone's requirements.

@jharris4
Copy link
Author

@ljharb ah yes, combing or with explicitNull from airbnb-prop-types would work.

So for anyone looking for a way to do what this PR is about you have two options:

import PropTypes from 'prop-types-defined';

const propTypes = {
  aBoolOrNullValue: PropTypes.bool.isDefined
};

or

import PropTypes from 'airbnb-prop-types';

const propTypes = {
  aBoolOrNullValue: PropTypes.or([PropTypes.bool.isRequired, PropTypes.explicitNull().isRequired]).isRequired
};

@ljharb any reason not to close this PR now? Seems like it's never going to be merged, and it might be better to just point people to another solution...

@ghost
Copy link

ghost commented Dec 17, 2020

Is there any movement on this PR? It would be nice to be able to do the following:

PropTypes.oneOfType([PropTypes.string, PropTypes.oneOf([null])]).isRequired

As others have probably already mentioned, null and undefined are two different constructs and should not be treated as the same thing...

@davidje13
Copy link

@kesupile-kesupile it's looking increasingly likely that this will never be merged.

A while ago I made a tiny plugin to allows nulls (it's a plugin rather than a fork like some of the other solutions so you don't need to worry about it going stale): https://www.npmjs.com/package/prop-types-nullable — it's tested, supports TypeScript, and the chosen syntax is compatible with common linter rules.

npm install --save prop-types-nullable

Usage:

const PropTypes = require('prop-types');
const nullable = require('prop-types-nullable');
 
MyReactClass.propTypes = {
  foobar: nullable(PropTypes.func).isRequired,
};

(or just copy the 10-or-so lines of code for nullable into your own codebase if you don't want an extra dependency)

@ljharb
Copy link
Contributor

ljharb commented Dec 25, 2020

@davidje13 https://npmjs.com/airbnb-prop-types has had an explicitNull propType for years; i'd suggest using that since that package also has many other propType composition helpers.

As to whether this will be merged or not, it's a relatively impactful change, so it's unlikely to be merged soon, at least.

@lukescott
Copy link

Also React no longer has prop-types as a dependency. Even though though you could use other libs before, it still included it, but not anymore. Better to use an alt lib like the Airbnb one or a fork of this one.

@matuszeg
Copy link

Any updates on when/whether this will be reviewed? Its a small PR that offers a large amount of value. It would tremendously improve the project. All the other alternatives suggested are all very verbose. Seems odd to not move forward with this when it provides a simple solution with very little changes.

Copy link
Contributor

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still not sure whether this is viable to land, but to be landable, it'd have to add similar tests to all four test files.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

disallow null