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

Inline dev-only requires #7188

Merged
merged 2 commits into from
Jul 5, 2016
Merged

Inline dev-only requires #7188

merged 2 commits into from
Jul 5, 2016

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Jul 5, 2016

This is inspired by @keyanzhang’s work in #7178 but without Rollup.
He noticed ReactComponentTreeDevtool and some other devtools get included into prod build.
This fixes it:

   raw     gz Compared to master @ 7d0801e1a085ef7c52b0cc0212404335fa75c717    
     =      = build/react-dom-server.js                                        
     =      = build/react-dom-server.min.js                                    
     =      = build/react-dom.js                                               
     =      = build/react-dom.min.js                                           
  +133    +22 build/react-with-addons.js                                       
 -3715  -1088 build/react-with-addons.min.js                                   
  +133    +18 build/react.js                                                   
 -3694  -1133 build/react.min.js  

Unfortunately it’s way too easy to regress on this. Maybe we should have a way of marking certain files as dev-only and then failing the build if they end up in production bundle?

@gaearon gaearon added this to the 15-next milestone Jul 5, 2016
This reduces the production bundled build size.
@vjeux
Copy link
Contributor

vjeux commented Jul 5, 2016

Maybe we should have a way of marking certain files as dev-only and then failing the build if they end up in production bundle?

Should be easy to make it happen

// file.js
require('assertOnlyIncludedInDev');

// assertOnlyIncludedInDev.js
if (!__DEV__) {
  throw 'This file should only be included in __DEV__';
}

@zpao
Copy link
Member

zpao commented Jul 5, 2016

@vjeux that doesn't fail the build process, just causes the final bundle to break in the browser. It will break FB though (except when inline-requires is on then it should be ok).

@vjeux
Copy link
Contributor

vjeux commented Jul 5, 2016

@zpao hopefully there's at least going to be one test failing if the entire React bundle instant fatal :)

This fixes the tests which were broken due to inlining some requires.
@gaearon
Copy link
Collaborator Author

gaearon commented Jul 5, 2016

If we add a check for the production bundle, there is still a potential pitfall: any lazy requires (which could be in prod as well) would not be caught.

@gaearon gaearon merged commit 8fe6b5f into facebook:master Jul 5, 2016
@gaearon gaearon deleted the inline-requires branch July 5, 2016 17:20
@zpao
Copy link
Member

zpao commented Jul 5, 2016

hopefully there's at least going to be one test failing if the entire React bundle instant fatal :)

There isn't. We don't have anything run tests on the bundles. Best chance would be running the examples but they all use the dev build.

@keyz
Copy link
Contributor

keyz commented Jul 5, 2016

Haha, you beat me to it. Ideally we won't have lazy requires any more.

zpao pushed a commit that referenced this pull request Jul 8, 2016
* Inline dev-only requires

This reduces the production bundled build size.

* Use new references after resetting module registry in tests

This fixes the tests which were broken due to inlining some requires.

(cherry picked from commit 8fe6b5f)
@zpao zpao modified the milestones: 15-next, 15.2.1 Jul 8, 2016
@keyz keyz mentioned this pull request Jul 8, 2016
2 tasks
usmanajmal pushed a commit to usmanajmal/react that referenced this pull request Jul 11, 2016
* Inline dev-only requires

This reduces the production bundled build size.

* Use new references after resetting module registry in tests

This fixes the tests which were broken due to inlining some requires.
keyz pushed a commit to keyz/react that referenced this pull request Jul 13, 2016
gaearon added a commit that referenced this pull request Jul 16, 2016
* Eagerly evaluate inline requires in Jest

I inlined some requires in #7188 to fix the build size regression.
However this caused an issue with Jest due to it resetting module registry between tests.

This is a temporary fix to #7240.
It should be reverted as part of #7178.

* Make the hack work in all environments
zpao pushed a commit that referenced this pull request Jul 22, 2016
* Eagerly evaluate inline requires in Jest

I inlined some requires in #7188 to fix the build size regression.
However this caused an issue with Jest due to it resetting module registry between tests.

This is a temporary fix to #7240.
It should be reverted as part of #7178.

* Make the hack work in all environments

(cherry picked from commit 15ae585)
keyz pushed a commit to keyz/react that referenced this pull request Jul 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants