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

Don’t bundle DebugTool in production #7189

Merged
merged 1 commit into from
Jul 5, 2016
Merged

Don’t bundle DebugTool in production #7189

merged 1 commit into from
Jul 5, 2016

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Jul 5, 2016

Most of this diff is indentation.

I am removing all __DEV__ checks from ReactDebugTool and ReactDOMDebugTool code. Instead, I am conditionally requiring them—in production, ReactInstrumentation.debugTool and ReactDOMInstrumentation.debugTool would now be null after this change.

Why I am doing this:

  1. Debug tools are not currently used in production at all. All calls to .debugTool.* are guarded by __DEV__.
  2. When we add a __PROFILE__ build, we can change the same condition to include __PROFILE__. There’s nothing here that would make it harder in the future.
  3. We are currently bundling unnecessary noops with method names and strings in production, and as we add more events, they will keep growing:

screen shot 2016-07-05 at 18 40 10

In the future, we might allow attaching devtools in production. Let’s do it when we actually have a use case for it. Right now it’s weird because we have these constraints that don’t actually end up getting used. This makes debug tool architecture more confusing to whoever who will be adding the __PROFILE__ build.

Even when we allow this, it is likely that the actual DebugTool would be external code. There is no need for it to live inside React. The default “broadcasting” DebugTools are good for development but I don’t see why they are useful in prod.

Let’s just keep it simple for now and let the code reflect our real usage. Debug tools are only called in development environment now.

This reduces build size a bit:

     =      = build/react-dom-server.js                                        
     =      = build/react-dom-server.min.js                                    
     =      = build/react-dom.js                                               
     =      = build/react-dom.min.js                                           
  -471     -4 build/react-with-addons.js                                       
 -2732   -620 build/react-with-addons.min.js                                   
  -471     -9 build/react.js                                                   
 -2733   -583 build/react.min.js 

If we are concerned about stray .debugTool.* calls that would blow up in production, we can lint against using .debugTool. outside a __DEV__ block. We don’t add new calls very often though so I’m not sure if it’s worth it.

@gaearon gaearon merged commit 5d31ebc into facebook:master Jul 5, 2016
@gaearon gaearon deleted the disable-debugtool-in-prod branch July 5, 2016 18:41
@gaearon gaearon added this to the 15-next milestone Jul 5, 2016
zpao pushed a commit that referenced this pull request Jul 8, 2016
@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
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 14, 2016
I caused it with #7189.
We generally don’t recommend running TestUtils in production environment but this is technically a regression.

Fixes #7231.
zpao pushed a commit that referenced this pull request Jul 22, 2016
I caused it with #7189.
We generally don’t recommend running TestUtils in production environment but this is technically a regression.

Fixes #7231.
(cherry picked from commit 27d7592)
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.

3 participants