-
Notifications
You must be signed in to change notification settings - Fork 650
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
Enhancement for best practice prop types. #70
Conversation
… size and allow users that import this package to include prop types in development and optionally remove them using Webpacks UglifyJS + Define plugin.
@jquense Let me know your thoughts. If we can get this merged and a future build cut relatively soon that'd be great. Thanks! |
Ping @jquense |
src/CSSTransitionGroup.js
Outdated
@@ -53,7 +37,21 @@ class CSSTransitionGroup extends React.Component { | |||
} | |||
} | |||
|
|||
CSSTransitionGroup.propTypes = propTypes; | |||
CSSTransitionGroup.defaultProps = defaultProps; | |||
CSSTransitionGroup.propTypes = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really like the propTypes defined down here, is the plugin really not smart enough to extract them as they were?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The plugin traverses the AST using the default parser... I think it's Babylon under the hood. It wraps the direct node and can't reverse engineer because it would require to find all references I believe which is very complicated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its not that complicated, i've written that babel plugin before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could investigate to see whether uglify will find the dead code and resulting file size is same if you'd prefer to keep these as separate variables?
@@ -57,6 +57,7 @@ | |||
"babel-plugin-add-module-exports": "^0.2.1", | |||
"babel-plugin-dev-expression": "^0.2.1", | |||
"babel-plugin-transform-object-assign": "^6.22.0", | |||
"babel-plugin-transform-react-remove-prop-types": "^0.4.5", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this here? you aren't using it anywhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, forgot to add plugin wrap mode to .babelrc
, Updated.
@@ -9,6 +9,7 @@ | |||
"plugins": [ | |||
"dev-expression", | |||
"add-module-exports", | |||
"transform-object-assign" | |||
"transform-object-assign", | |||
["transform-react-remove-prop-types", { mode: "wrap" }] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the idea that wrapping will work with uglify to remove dead code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, correct. Sample output bundle from development build would look as follows:
TransitionGroup.propTypes = process.env.NODE_ENV !== "production" ? {
component: _propTypes2.default.any,
childFactory: _propTypes2.default.func,
children: _propTypes2.default.node
} : {};
In the now production build for this app (as well as end-users who import and build production bundles using either Webpack -p
or equivalently Define + Uglify plugins) will result in:
After Define
TransitionGroup.propTypes = false ? {
component: _propTypes2.default.any,
childFactory: _propTypes2.default.func,
children: _propTypes2.default.node
} : {};
After Uglify
TransitionGroup.propTypes = {};
So small changes; however, allows for smaller code base in this production bundle -- which is a good thing... And optionally allows for end-users to perform the same optimizations when using bundlers like Webpack, browserify, etc, when not using external distribution production build.
This is how a lot of packages help reduce filesize (e.g. material-ui)
does find dead code and remove it.
@jquense The uglify plugin does remove the dead code for prop types when defined in the original location, so I went ahead and restored those. So essentially, this saves a few hundred bytes in production now with the following changes :) Let me know if you need anything else as the sooner we can get the next release cut we can use production dist minified from unpkg. |
04bf0af
to
bba5d44
Compare
ping @jquense |
thanks! |
* Fixes #69: Adding capability to reduce production build output size and allow users that import this package to include prop types in development and optionally remove them using Webpacks UglifyJS + Define plugin. * Updated .babelrc that properly wraps prop-types. * Restoring the original constant declaration for prop types as uglify does find dead code and remove it. * Updating changelog.
* Fixes #69: Adding capability to reduce production build output size and allow users that import this package to include prop types in development and optionally remove them using Webpacks UglifyJS + Define plugin. * Updated .babelrc that properly wraps prop-types. * Restoring the original constant declaration for prop types as uglify does find dead code and remove it. * Updating changelog.
* Initial * clean up prop names * more exploration * fun children * fix tests * some comment docs * Incorporated development + production builds into /dist (#64) (#66) * Incorporated development + production builds into /dist (#64) and updated Readme to include CDN/External information. * Dropping references to Webpack specific build configurations in Readme and adding clarity for CDN/external. * Enhancement for best practice prop types. (#70) * Fixes #69: Adding capability to reduce production build output size and allow users that import this package to include prop types in development and optionally remove them using Webpacks UglifyJS + Define plugin. * Updated .babelrc that properly wraps prop-types. * Restoring the original constant declaration for prop types as uglify does find dead code and remove it. * Updating changelog.
Fixes #69: Reduces output bundles by wrapping prop types allowing for end users to further remove them using the babel plugin in combination with webpack define plugin. Production build file reduced by removing propTypes for static dist build.