-
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
Changes from 2 commits
c0aadb4
b33b0dd
5321582
13a727b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Good catch, forgot to add plugin wrap mode to |
||
"babel-preset-latest": "^6.24.0", | ||
"babel-preset-react": "^6.23.0", | ||
"babel-preset-stage-2": "^6.18.0", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,22 +5,6 @@ import TransitionGroup from './TransitionGroup'; | |
import CSSTransitionGroupChild from './CSSTransitionGroupChild'; | ||
import { nameShape, transitionTimeout } from './utils/PropTypes'; | ||
|
||
const propTypes = { | ||
transitionName: nameShape.isRequired, | ||
|
||
transitionAppear: PropTypes.bool, | ||
transitionEnter: PropTypes.bool, | ||
transitionLeave: PropTypes.bool, | ||
transitionAppearTimeout: transitionTimeout('Appear'), | ||
transitionEnterTimeout: transitionTimeout('Enter'), | ||
transitionLeaveTimeout: transitionTimeout('Leave'), | ||
}; | ||
|
||
const defaultProps = { | ||
transitionAppear: false, | ||
transitionEnter: true, | ||
transitionLeave: true, | ||
}; | ||
|
||
class CSSTransitionGroup extends React.Component { | ||
|
||
|
@@ -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 commentThe 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 commentThe 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 commentThe 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 commentThe 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? |
||
transitionName: nameShape.isRequired, | ||
|
||
transitionAppear: PropTypes.bool, | ||
transitionEnter: PropTypes.bool, | ||
transitionLeave: PropTypes.bool, | ||
transitionAppearTimeout: transitionTimeout('Appear'), | ||
transitionEnterTimeout: transitionTimeout('Enter'), | ||
transitionLeaveTimeout: transitionTimeout('Leave'), | ||
}; | ||
|
||
CSSTransitionGroup.defaultProps = { | ||
transitionAppear: false, | ||
transitionEnter: true, | ||
transitionLeave: true, | ||
}; | ||
|
||
export default CSSTransitionGroup; |
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:
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
After Uglify
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)