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.

Sign up for GitHub

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

Jump to bottom

Strip React.PropTypes type checking code in production #7651

Closed
wants to merge 3 commits into from
Closed

Strip React.PropTypes type checking code in production #7651

wants to merge 3 commits into from

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Sep 3, 2016

This is #6401 revived.
It’s a major change that can go in 16 now that we warn for calls in 15.

This gives us -1K from isomorphic React post gzip.

   raw     gz Compared to last run                                             
     =      = build/react-dom-fiber.js                                         
     =      = build/react-dom-fiber.min.js                                     
     =      = build/react-dom-server.js                                        
     =      = build/react-dom-server.min.js                                    
     =      = build/react-dom.js                                               
     =      = build/react-dom.min.js                                           
 +1200   +213 build/react-with-addons.js                                       
 -3703   -999 build/react-with-addons.min.js                                   
 +1200   +216 build/react.js                                                   
 -3703   -988 build/react.min.js

@gaearon gaearon added this to the 15-next milestone Sep 3, 2016
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 87.244% when pulling f4c9a04 on gaearon:noop-proptypes-prod into 7b247f3 on facebook:master.

@gaearon
Copy link
Collaborator Author

gaearon commented Sep 3, 2016

Also pushed a revert for warning added in #7132.

@gaearon gaearon mentioned this pull request Sep 3, 2016
Closed
@ghost ghost added the CLA Signed label Sep 3, 2016
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.007%) to 87.225% when pulling 9539d71 on gaearon:noop-proptypes-prod into 7b247f3 on facebook:master.

fson
fson reviewed Sep 3, 2016
View reviewed changes
src/isomorphic/classic/types/ReactPropTypes.js
};
productionTypeChecker.isRequired = productionTypeChecker;
var getProductionTypeChecker = () => productionTypeChecker;
// Keep in sync with production version above
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this should say "Keep in sync with the development version above."

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.007%) to 87.225% when pulling aeabce2 on gaearon:noop-proptypes-prod into 7b247f3 on facebook:master.

@gaearon
Copy link
Collaborator Author

gaearon commented Sep 3, 2016

The PR is fine, don’t be fooled by 0.007% decrease in coverage..

@zpao
Copy link
Member

zpao commented Sep 3, 2016

FWIW, I just changed the threshold for coveralls to -1% so hopefully we don't get too much noise from failures there.

@zpao zpao modified the milestones: 16.0, 15-next Sep 3, 2016
@ghost ghost added the CLA Signed label Sep 3, 2016
vjeux
vjeux reviewed Sep 3, 2016
View reviewed changes
src/isomorphic/classic/types/ReactPropTypes.js
var getProductionTypeChecker = () => productionTypeChecker;
// Keep in sync with development version above
var ReactPropTypes = {
array: getProductionTypeChecker(),
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the point of calling it here? Why not just put productionTypeChecker ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm trying to mirror the structure above exactly so it's easy to keep them in sync.

@mbrevda mbrevda mentioned this pull request Oct 19, 2016
Merged
@gaearon
Copy link
Collaborator Author

gaearon commented Oct 23, 2016

Closing, will resubmit as another PR.

@gaearon gaearon closed this Oct 23, 2016
@gaearon gaearon mentioned this pull request Oct 23, 2016
Merged
@gaearon
Copy link
Collaborator Author

gaearon commented Oct 23, 2016

#8066

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@sophiebits sophiebits

Labels
CLA Signed
Projects
None yet
Milestone
16.0
Development

Successfully merging this pull request may close these issues.
7 participants
@gaearon @coveralls @zpao @vjeux @fson @sophiebits @facebook-github-bot