Skip to content
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

Returning an error if a PropType is passed to ReactPropTypes.oneOf. #7526

Closed
wants to merge 5 commits into from

Conversation

ventuno
Copy link
Contributor

@ventuno ventuno commented Aug 19, 2016

I took where @philipshurpik left off with #6940 to fix #1919, as that PR can't be merged anymore and is more than a month old.

@philipshurpik, if you meant to keep on working on #6940, please let me know and I'll close this one.

The idea is to return a PropTypeError whenever one of ReactPropTypes is passed to ReactPropTypes.oneOf. The error message, hints that the user might be meaning to use ReactPropTypes.oneOfType instead.

  1. If createChainableTypeChecker is called with an expectedType we add the expectedType flag to the checkType function (see [1])
  2. In createEnumTypeChecker.validate, if expectedValues[i] is of type function and it has the expectedType flag, we know that a ReactPropTypes has been passed, so we throw the error described above
  3. As per Improve proptypes oneof warnings - Fixes #1919 #6940, if a generic function is passed to ReactPropTypes.oneOf we improve the previous output to display "Unexpected function" and the function converted to string

As a side note, I noticed that if getPropType is called with undefined, we get the following exception:
Cannot read property '@@toStringTag' of undefined
Is this expected?

[1] https://github.com/facebook/react/pull/6940/files#r68511156

@ventuno ventuno changed the title Returning error if a PropType is passed. Returning an error if a PropType is passed to ReactPropTypes.oneOf. Aug 19, 2016
@ghost ghost added the CLA Signed label Aug 19, 2016
values[i] = 'Unexpected function: ' + values[i].toString();
}
}
return values;
Copy link

Choose a reason for hiding this comment

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

maybe something like this instead?

return values.map(value => (
  getPropType(value) === 'function' ? `Unexpected function: ${value}` : value
));

You shouldn't have to specially call toString on value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, @donavon. Thanks a lot, I implemented your recommended change :-).

@ventuno
Copy link
Contributor Author

ventuno commented Feb 12, 2017

Dear @aweary and @gaearon, I just noticed there was some activity about this PR [1]. So I decided to fix the merging issues. Can you kindly review?

[1] #8274 (comment)

@gaearon
Copy link
Collaborator

gaearon commented Oct 4, 2017

Sorry about this! We've been busy with a React rewrite this year, and haven't really paid attention to PRs. :-(

This code has moved to https://github.com/facebook/prop-types so you can make a PR there and we could review.

@gaearon gaearon closed this Oct 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PropTypes.oneOf warning does not include expected types
5 participants