-
Notifications
You must be signed in to change notification settings - Fork 47k
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
Add Symbol prop type #6387
Add Symbol prop type #6387
Conversation
@@ -83,6 +83,7 @@ var ReactPropTypes = { | |||
oneOf: createEnumTypeChecker, | |||
oneOfType: createUnionTypeChecker, | |||
shape: createShapeTypeChecker, | |||
symbol: createSymbolTypeChecker(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be symbol: createSymbolTypeChecker,
(without ()
at the end)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling the createSymbolTypeChecker
returns the chainable type checker, right?
As you would do something like: React.PropTypes.symbol.isRequired
and not React.PropTypes.symbol().isRequired
(am I wrong?)
There is #6377 though. Maybe you should collaborate 😉 |
@gaearon @puradox Should we continue the PR here? We can backport my commits to its PR or I can backport its commits to my PR. Anyway, I'm very interested in improving the test suite! |
@RaitoBezarius We have a bit more conversation over on my PR. You should take a look and we can collaborate together on which approach is best. Take a look at my changes and compare it with yours. From what it looks like, you treated |
Hi there,
As it seemed that no one took #4917, here is my pull request for it.
Some rationale behind the choice as Symbols are essentially polyfilled / transpiled:
Is there any case that I could have missed? Let me know if I need to work on it to get it accepted!
Thanks!