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

Don't check propTypes ahead of time #3591

Merged
merged 1 commit into from
Jun 30, 2016

Conversation

gaearon
Copy link
Contributor

@gaearon gaearon commented Jun 30, 2016

We are adding a warning to React that warns when you call PropTypes validation functions by yourself. This is going to be unsupported as we want to make them behave more like black boxes so people stop calling them, and it would become safe for us to make them no-ops in production, saving some code size.

I tried the new code on some projects, and it appears that React Router calls propTypes directly generating the warnings: facebook/react#7132 (comment).

I’m not sure how valuable that check was so I just removed it. If it’s super valuable, we can replace it with React.createElement call that would make React check the prop types, but in this case I’d need to have a test case verifying the behavior you wanted because I’m not sure I fully understand why this call is necessary.

I’d appreciate if you could merge this and cut a release (especially if you could also backport it to 2.x) because otherwise React is start emitting warnings about React Router in a month or so.

@timdorr
Copy link
Member

timdorr commented Jun 30, 2016

Yeah, we don't need to handle this if React can take care of it. 👍 from me.

@taion
Copy link
Contributor

taion commented Jun 30, 2016

This check is just bizarre – by assumption the input to this is a React element, so the propTypes check would already have been made in React.createElement.

It looks like the check was originally added in c5a24a5. Was there some point in the past in which React validated prop types at mount time rather than at element creation time? cc @mjackson

@gaearon It will still be possible for a custom prop type validator to call into a built-in one, right?

@taion taion merged commit c115a8b into remix-run:next Jun 30, 2016
@gaearon
Copy link
Contributor Author

gaearon commented Jun 30, 2016

It will still be possible for a custom prop type validator to call into a built-in one, right?

Yea as long as you pass in all the arguments. There's a new "secret" argument we check against. Using spread on argument list and passing it on is enough.

@taion
Copy link
Contributor

taion commented Jun 30, 2016

I'll release 2.5.2 tomorrow with this fix unless a reason comes up to not do this.

@gaearon
Copy link
Contributor Author

gaearon commented Jun 30, 2016

Thanks!

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

Successfully merging this pull request may close these issues.

3 participants