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

Warning for defining propTypes in production #6402

Closed
brigand opened this issue Apr 3, 2016 · 6 comments
Closed

Warning for defining propTypes in production #6402

brigand opened this issue Apr 3, 2016 · 6 comments

Comments

@brigand
Copy link
Contributor

brigand commented Apr 3, 2016

A production only warning for having propTypes defined as a non-empty object could spark a change in the community to have propTypes conditional on NODE_ENV. This could be automated with a babel plugin, or similar for other build systems.

This would result in all react apps being significantly smaller in production.

Warning: you have defined propTypes on the {ComponentName} component in a production build. If this is your own code, consider using a plugin such as babel-plugin-{plugin-name}. If it's a third party library, open an issue.

Or alternative save-all-bytes text.

Warning: propTypes defined on {ComponentName} http://fb.me/react-prod-proptypes

@mijamo
Copy link

mijamo commented Apr 3, 2016

Naive question here: why should it trigger a warning ? As far as I know propTypes have no influence in production so there is no negative consequence of having them, except that it takes some lines of code.

So I really don't see why a warning should be triggeredfor something that cannot possibly have any impact at all.

@brigand
Copy link
Contributor Author

brigand commented Apr 3, 2016

Just to warn people that there's unneeded code in their production builds. I only suggest it because impacts almost every react app in existence. Maybe it's a stupid idea, but I wanted to bring it up in case other people agree.

It follows the general rule of warnings in that they indicate something that should be fixed. I imagine it'd be hard to find someone arguing that propTypes should take up space in production builds.

@jimfb
Copy link
Contributor

jimfb commented Apr 3, 2016

This would put a pretty heavy burden on component authors. They would need to distribute two versions of their components (prod vs. dev). It's also not super actionable if you're using a third-party component that defines proptypes. An automated transform wouldn't necessarily be able to eliminate all proptypes (eg. on stateless function components) without making a bold assumption that assigning proptypes to a function means that function must be a React component.

Anyway, I'm not actually oppose to the idea, but I think we'd need to have the transforms in existence before we could enable such a warning. My guess is that it's not worth the headache. But if someone writes such a transform, and people start using it and find that the byte savings justify the complexity, we could consider enabling such a warning. Let's re-evaluate if/when such a transform becomes mainstream.

@jimfb jimfb closed this as completed Apr 3, 2016
@brigand
Copy link
Contributor Author

brigand commented Apr 3, 2016

Thanks for the good feedback; those are some issues I didn't think of.

@mijamo
Copy link

mijamo commented Apr 6, 2016

@brigand You will be pleased by #6401. After this pull (for v16), Uglify should delete the proptypes for production :)

@jimfb
Copy link
Contributor

jimfb commented Apr 6, 2016

@mijamo Yeah, that solves some of, but not all of, the overhead that @brigand mentioned. React components will still have a proptype object defined with a bunch of empty function references. But yes, it's an improvement :).

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

No branches or pull requests

3 participants