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

Add validation for arrayOf and objectOf in ReactPropTypes #5390

Merged
merged 2 commits into from
Nov 12, 2015

Conversation

chicoxyzzy
Copy link
Contributor

  static propTypes = {
    instrument: PropTypes.object.isRequired,
    data: PropTypes.arrayOf({
      timestamp: PropTypes.object.isRequired,
      value: PropTypes.number.isRequired,
    }).isRequired,
    height: PropTypes.number.isRequired,
    width: PropTypes.number.isRequired,
  };

Notice data property. it should look like

    data: PropTypes.arrayOf(PropTypes.shape({
      timestamp: PropTypes.object.isRequired,
      value: PropTypes.number.isRequired,
    })).isRequired,

instead.

This is one of common mistakes. But error message is very cryptic:
Warning: Failed propType: typeChecker is not a function Check the render method of 'ChartPanel'.

So I wrote some tests and pretty error messages.

now error looks like this
Warning: Failed propType: Invalid prop 'data' supplied to 'ChartContainer', expected valid PropType. Check the render method of 'ChartPanel'.

This is kind of same as #3963 but for arrayOf and objectOf proptypes

@chicoxyzzy chicoxyzzy changed the title Adding validation for arrayOf and objectOf in ReactPropTypes Add validation for arrayOf and objectOf in ReactPropTypes Nov 4, 2015
@@ -144,6 +144,11 @@ function createAnyTypeChecker() {

function createArrayOfTypeChecker(typeChecker) {
function validate(props, propName, componentName, location, propFullName) {
if (typeof typeChecker !== 'function') {
return new Error(
`Invalid argument \`${propFullName}\` supplied to \`${componentName}\`, expected valid PropType.`
Copy link
Member

Choose a reason for hiding this comment

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

This message isn't awesome. You're actually trying to say that the component author wrote a bad propType spec, but the way this error will show sounds like the user provided an invalid prop value. Can we tune it so that's more clear?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it more clear now?

@zpao
Copy link
Member

zpao commented Nov 5, 2015

Thanks for doing this, I think it's a great idea. I sort of wish I had pushed for making #3963 actually throw an error (since it would throw at require time, not component creation time). We have less contextual information we can provide but the error would actually be thrown earlier, preventing bad proptype definitions from getting committed… maybe we can do that in another pass.

@facebook-github-bot
Copy link

@chicoxyzzy updated the pull request.

@zpao
Copy link
Member

zpao commented Nov 12, 2015

Alright, let's do it. Now that I'm thinking about proptypes again, I might have some followup ideas to try but this puts us in a better spot.

zpao added a commit that referenced this pull request Nov 12, 2015
Add validation for arrayOf and objectOf in ReactPropTypes
@zpao zpao merged commit 8104262 into facebook:master Nov 12, 2015
@chicoxyzzy chicoxyzzy deleted the arrayof-objectof-tests branch November 12, 2015 00:53
@slorber
Copy link
Contributor

slorber commented Dec 14, 2015

hey @zpao I also got a case where I had this error. I'm not exactly sure where it came from nor where I fixed it but a better error message would have helped :)

Actually we register all of our Api payloads as proptypes. I think at some point the declaration order of proptypes lead to something like PT.arrayOf(undefined), or maybe PT.shape({attribute: undefined}

Maybe it's something else because I can't get this "typeChecker" error back.

@chicoxyzzy
Copy link
Contributor Author

@slorber feel free to ping me if you'll meet your issue again

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.

4 participants