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

Framework: Allow debug everywhere #12841

Merged
merged 1 commit into from
Jul 12, 2017
Merged

Conversation

samouri
Copy link
Contributor

@samouri samouri commented Apr 5, 2017

Stop replacing debug with debug-noop in in production.

Thinking:

  1. its helpful to be able to look at debug statements in every environment
  2. debug is a tiny library that doesn't add much to our bundle size
  3. debug wont' hurt performance in any way for those uninterested since it is opt-in via localStorage anway.

To Test:

  • debug statements should still work in development
  • debug statements should now work in docker

@matticbot
Copy link
Contributor

@matticbot matticbot added [Size] S Small sized issue [Size] M Medium sized issue and removed [Size] S Small sized issue labels Apr 5, 2017
@samouri samouri mentioned this pull request Apr 5, 2017
4 tasks
@samouri samouri force-pushed the update/framework/debug-runtime-check branch from 2456497 to 2776e47 Compare April 5, 2017 21:06

export default ( ...args ) => {
if ( config( 'env_id' ) === 'production' ) {
return () => {};
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe just use lodash/noop or a single noop function to cut down on the number of functions we create?

@matticbot matticbot added [Size] S Small sized issue and removed [Size] M Medium sized issue labels Apr 5, 2017
@aduth
Copy link
Contributor

aduth commented Apr 5, 2017

The default cache key implementation for createSelector could serve as reference for how to optimize module loading via Webpack dead code detection to avoid importing the debug module:

https://github.com/Automattic/wp-calypso/blob/b5b487b/client/lib/create-selector/index.js#L38-L61

@samouri
Copy link
Contributor Author

samouri commented Apr 5, 2017

@aduth, that only works for differentiating between docker vs. dev server.
Since launching #11352, anything that is marked with if ( 'production' !== process.env.NODE_ENV ) will be removed from both wpcalypso and staging as well as production.

With these constraints, my current thought is that it could be best to use the regular debug in all environments

@aaronyan
Copy link
Member

aaronyan commented Jul 6, 2017

I believe this PR would fix #12277

@samouri samouri force-pushed the update/framework/debug-runtime-check branch from f64b680 to 45240b3 Compare July 7, 2017 17:11
@samouri samouri added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Jul 7, 2017
@samouri samouri changed the title Framework: always replace debug with a wrapper that does a runtime check for production Framework: Allow debug everywhere Jul 7, 2017
@samouri
Copy link
Contributor Author

samouri commented Jul 7, 2017

I just tested locally + in docker. both w4m

@samouri
Copy link
Contributor Author

samouri commented Jul 7, 2017

I tested out the difference in bundle size. The vendor chunk grew from 672K --> 676K.
Everything else remained equivalent

Copy link
Member

@dmsnell dmsnell left a comment

Choose a reason for hiding this comment

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

Seems fair to me. I would like some cake for this review.

@dmsnell
Copy link
Member

dmsnell commented Jul 11, 2017

The vendor chunk grew from 672K --> 676K.
Everything else remained equivalent

this suggests to me that we made a poor replacement: we were almost certainly leaving in the strings which were the largest part of debug() 🤦‍♂️

I wonder what would happen to the size if we not only removed debug() but also removed anything that chains on a call to debug. for instance, use a Babel transform to remove all code which depend on debug; not only replace it with a noop, but actually pull out the function calls

@samouri
Copy link
Contributor Author

samouri commented Jul 11, 2017

@dmsnell: yep! That would make a much larger difference for build size

Just did a quick check and it looks like we have something on the order of 1.5 thousand debug calls across calypso. When i grepped all the lines and zipped them it was ~ 25Kb.

@samouri
Copy link
Contributor Author

samouri commented Jul 11, 2017

Thanks for the review @dmsnell! I'll merge this in tomorrow afternoon unless there are objections that come up.

@dmsnell
Copy link
Member

dmsnell commented Jul 11, 2017

Just did a quick check and it looks like we have something on the order of 1.5 thousand debug calls across calypso. When i grepped all the lines and zipped them it was ~ 25Kb.

when I asked about the build size difference this is about what I expected to find. that could also easily cut down our build size. now I'm wondering why we ever disabled debug because it wasn't helping us on size. we should have done it for realz and gotten that 25 KB

@samouri samouri merged commit e069350 into master Jul 12, 2017
@samouri samouri deleted the update/framework/debug-runtime-check branch July 12, 2017 20:56
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jul 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework [Size] S Small sized issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants