Skip to content

Report the actual type when PropTypes.instanceOf fails#4622

Merged
zpao merged 1 commit intofacebook:masterfrom
jorrit:instanceofreporttype
Aug 14, 2015
Merged

Report the actual type when PropTypes.instanceOf fails#4622
zpao merged 1 commit intofacebook:masterfrom
jorrit:instanceofreporttype

Conversation

@jorrit
Copy link
Copy Markdown
Contributor

@jorrit jorrit commented Aug 13, 2015

When PropTypes.instanceOf fails, the error text does not report what the type of the actual prop is. This PR adds the type of the actual prop.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How is this not throwing when props[propName] === null?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh right, null doesn't actually come into this check because it does the isRequired check first.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If the value is Object.create(null), then we'll throw here. We should guard against that.

@zpao
Copy link
Copy Markdown
Member

zpao commented Aug 13, 2015

Sorry I talked to myself in there… I think this seems reasonable. There could still be some false positives - eg, two different c'tors that both have Cat as their name, or more likely problems with primitives (eg 5 instanceof Number will fail the proptype check but then the message will say that it expected Number and got Number) but that's why we have the other primitive proptype checkers.

@jorrit
Copy link
Copy Markdown
Contributor Author

jorrit commented Aug 14, 2015

Yes, the nature of Javascript is such that explaining the type checks is hard. In cases that someone has two different classes with the same name it could be the case that 'Cat' is not 'Cat', but someone using class name multiple times should expect something like that, in my opinion.

I'll update the code.

@jorrit
Copy link
Copy Markdown
Contributor Author

jorrit commented Aug 14, 2015

I have updated the commit of my branch to incorporate the check you recommended.

@zpao
Copy link
Copy Markdown
Member

zpao commented Aug 14, 2015

Thanks!

zpao added a commit that referenced this pull request Aug 14, 2015
Report the actual type when PropTypes.instanceOf fails
@zpao zpao merged commit b4b028d into facebook:master Aug 14, 2015
@jorrit
Copy link
Copy Markdown
Contributor Author

jorrit commented Aug 14, 2015

Thanks for merging!

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.

3 participants