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

PropTypes.node forbids true but flow typing accepts it, causing runtime warning #310

Open
ypresto opened this issue Jan 15, 2020 · 18 comments
Assignees

Comments

@ypresto
Copy link

ypresto commented Jan 15, 2020

PropTypes.node does not accept true according to #109.

But flow typing in React declares export type ReactEmpty = null | void | boolean;
https://github.com/facebook/react/blob/b6173e643a4311b9b1cf039824b2f3d7b974b8cf/packages/shared/ReactTypes.js#L18 so it accepts true without error for type checking, but prop-types shows warning for it.

(Actually I'm using equivalent TypeScript type definition along with material-ui library which uses prop-types.)

@ljharb
Copy link
Contributor

ljharb commented Jan 15, 2020

Sounds like the flow types (which are maintained in a separate package) have a bug. I’d suggest filing this issue there :-)

@ljharb ljharb closed this as completed Jan 15, 2020
@ypresto
Copy link
Author

ypresto commented Jan 15, 2020

ReactEmpty flow type is in facebok/react repo and which is just following React's spec to include true. So I'm filling issue on facebook/react to change that spec, is it right?

@ljharb
Copy link
Contributor

ljharb commented Jan 16, 2020

Ah, I misunderstood. You're saying that react itself changed to allow true at some point, and PropTypes.node does not properly allow it?

It'd be great if we could figure out in which version that changed.

@ljharb ljharb reopened this Jan 16, 2020
@ypresto
Copy link
Author

ypresto commented Jan 16, 2020

Perhaps the type has not been changed since 2014: (the first commit which contains ReactEmpty (in the doc))
facebook/react@d36d26a#diff-191cf9292ca3ab67015443b913e019cbR179

type ReactEmpty = null | undefined | boolean;

@ljharb
Copy link
Contributor

ljharb commented Jan 16, 2020

Perhaps the first step then is to file an issue on react to ask them to update the flow type. Once that's updated, and once it's clear when that change in React proper happened, it should be straightforward to update it here.

@ypresto
Copy link
Author

ypresto commented Jan 19, 2020

AFAIK current state is:

  • React: accepts true and render empty at least since 2017/11
  • React's flow type: accepts true at least since 2014
  • @types/react: accepts true
  • PropTypes.node: rejects true

So React and its flow type are already in sync for true.
My guess is that React accepts true from the beginning. But I'm in doubt, so filled facebook/react#17871 .

@ljharb
Copy link
Contributor

ljharb commented Jan 21, 2020

Did some testing; looks like prior to React 16, true was disallowed. I'll update this package.

@ljharb ljharb self-assigned this Jan 21, 2020
@Hypnosphi
Copy link

Hypnosphi commented Jan 25, 2020

@ljharb

Did some testing; looks like prior to React 16, true was disallowed. I'll update this package.

Can you please share your test method? If you were trying to return true from component, it would fail just because React 15 didn't allow primitives as component return values. So true isn't different from 'foo' in that aspect. A proper test would be to pass true as children, and it was possible with React 15 AFAIR: <div>{true}</div>

UPD: it works in React 15 without any warnings: https://codesandbox.io/s/boring-goldberg-6qgnh?fontsize=14&hidenavigation=1&theme=dark

@ljharb
Copy link
Contributor

ljharb commented Jan 25, 2020

(similar to #109)

@Hypnosphi i just loaded up a bunch of react versions in a code sandbox and verified which ones gave a runtime warning or not. I was using true as a child of a div.

In other words, what i said was “prior to react 16, it was disallowed” which does not conflict with “works in 15”.

@Hypnosphi
Copy link

Sorry, but how is 15 not prior to 16?

@Hypnosphi
Copy link

verified which ones gave a runtime warning or not

So which versions gave warnings?

@ljharb
Copy link
Contributor

ljharb commented Jan 25, 2020

“disallowed < 16” means “does not work on 15”, I’m not sure where the confusion is.

Every version 15 and below gave warnings.

@Hypnosphi
Copy link

Hypnosphi commented Jan 25, 2020

Every version 15 and below gave warnings.

Can you please share a link to codesandbox? I have posted one above, and it gives no warnings

“does not work on 15”

does not conflict with “works in 15”

It doesn't make sense to me, sorry. Did you mean "does conflict" or something?

@ljharb
Copy link
Contributor

ljharb commented Jan 25, 2020

I discarded the link, so i have no link to share.

No, i meant does NOT conflict. React 16 worked, react 15 didn’t, with true as a child. I see that you are claiming that react 15 works, but your own sandbox has an invariant warning in the console, so react 15 seems to not work.

@Hypnosphi
Copy link

Hypnosphi commented Jan 25, 2020

Oh, looks like I had to fork the shared sandbox before playing with it, my bad. I reverted the changes, please check again

https://codesandbox.io/s/boring-goldberg-6qgnh

UPD: also, React 0.14: https://codesandbox.io/s/relaxed-fermi-7kgc4

@ljharb
Copy link
Contributor

ljharb commented Jan 25, 2020

Fair, those seem to work. Can you provide a react 0.13 sandbox with a class component to verify if that one fails?

@Hypnosphi
Copy link

Hypnosphi commented Jan 25, 2020

Can you provide a react 0.13 sandbox with a class component to verify if that one fails?

Nope

0.13 with class: https://codesandbox.io/s/sad-haze-yykbq
0.12 with createClass: https://codesandbox.io/s/weathered-waterfall-q8fm7

@ljharb
Copy link
Contributor

ljharb commented Jan 25, 2020

Then I’m very confused.

If true has always been allowed, then i don’t know why the node propType ever disallowed it.

I’m still happy to loosen it to accept true, but before i do id like to understand the disparity.

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

No branches or pull requests

3 participants