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

strip proptypes in production build (fixes #1882) #2003

Merged

Conversation

jochenberger
Copy link
Contributor

@jochenberger jochenberger commented Sep 14, 2017

File size changes:

react-select.es.js: 82822 -> 82970
react-select.js: 84232 -> 84380
react-select.min.js: 43966 -> 41700

See also #1876.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 92.056% when pulling fb33957 on jochenberger:strip-proptypes-in-production into 49b12b0 on JedWatson:master.

@jochenberger
Copy link
Contributor Author

I guess we need to replace 'process.env.NODE_ENV' in the other two bundles too. But I'm not sure what we should replace it with in the es6 build.

@jochenberger jochenberger changed the title strip proptypes in production build (fixes #1882) WIP: strip proptypes in production build (fixes #1882) Sep 14, 2017
@jochenberger
Copy link
Contributor Author

jochenberger commented Sep 14, 2017

And what should happen to lib/*.js? With this PR merged, they would contain the if (process.env.NODE_ENV !== 'production')wrappers. Are those files always used in a Node environment?
Maybe we should rather just add the https://github.com/oliviertassinari/babel-plugin-transform-react-remove-prop-types plugin to the .min.js build.

@jochenberger jochenberger force-pushed the strip-proptypes-in-production branch from fb33957 to a6b023a Compare September 14, 2017 12:08
@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 92.056% when pulling a6b023a on jochenberger:strip-proptypes-in-production into 49b12b0 on JedWatson:master.

@agirton
Copy link
Collaborator

agirton commented Sep 14, 2017

Hi @jochenberger thank you for the PR, instead of putting the propTypes behind a flag, we could probably just use babel-plugin-transform-react-remove-prop-types. This would make it much cleaner. Thoughts?

@JedWatson
Copy link
Owner

we could probably just use babel-plugin-transform-react-remove-prop-types

yes please 🙂

@jochenberger
Copy link
Contributor Author

I'm not so sure. Don't we want to have the checks in the src and lib files? I don't do node apps myself, but I imagine that people who do want the propTypes checks removed in production. If we only remove them in the bundles via babel-plugin-transform-react-remove-prop-types, they will always end up with the propTypes checks. However, if the if (process.env.NODE_ENV !== 'production') are present in the src and lib files, they will also effect the users' apps/bundles. Unless they use babel-plugin-transform-react-remove-prop-types too. I don't really know what's the best practice here, since I don't have much experience with node.
What do others do? react-router uses babel-plugin-transform-react-remove-prop-types for instance.

@JedWatson JedWatson self-requested a review September 21, 2017 01:36
Copy link
Owner

@JedWatson JedWatson left a comment

Choose a reason for hiding this comment

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

I think that it's not safe to have if (process.env.NODE_ENV !== 'production') anywhere other than in src. We can't guarantee anything about the environment the code may be run in now or in the future, which doing so breaks.

I don't think there's any concern having proptypes in place in a node environment, if the consumer requires the lib entry. React ignores proptypes in production anyway so this is only an optimisation for client-side package size.

Given the consistency of other major packages using babel-plugin-transform-react-remove-prop-types I think that's the way to go, and let's not introduce environment specific code in our source that we don't need to (I'm happy to follow react-router's configuration for this in terms of which builds to apply the plugin to)

@JedWatson
Copy link
Owner

JedWatson commented Sep 21, 2017

Please update to use babel-plugin-transform-react-remove-prop-types

@jochenberger jochenberger force-pushed the strip-proptypes-in-production branch from a6b023a to 5da115a Compare September 21, 2017 07:11
@jochenberger
Copy link
Contributor Author

Done. Now the only file size change is react-select.min.js: 44246 -> 41927.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 92.344% when pulling 5da115a on jochenberger:strip-proptypes-in-production into eb2dfba on JedWatson:master.

@jochenberger jochenberger changed the title WIP: strip proptypes in production build (fixes #1882) strip proptypes in production build (fixes #1882) Sep 21, 2017
@JedWatson
Copy link
Owner

Neat, thanks @jochenberger!

@JedWatson JedWatson merged commit 7523c20 into JedWatson:master Sep 24, 2017
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

Successfully merging this pull request may close these issues.

4 participants