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

Reduce build size by 33% #1756

Merged
merged 7 commits into from
Feb 23, 2017
Merged

Reduce build size by 33% #1756

merged 7 commits into from
Feb 23, 2017

Conversation

ebidel
Copy link
Contributor

@ebidel ebidel commented Feb 21, 2017

R: all

In this spirit of #1690, I've managed to get the build down to 1.6MB. 33% savings!

screen shot 2017-02-19 at 4 09 33 pm

Other low hanging fruit is stripping out the html/js license comments in the inlined files (readFileSync).

This PR introduces uglify-harmony into the extension gulpfile. gulp build:production will build lighthouse-extension.js and also compress it. gulp build continues to only broswerify. I felt that we may still want to keep that around for speedlier development when making tweaks to the crx. Happy to consolidate though.

@ebidel ebidel changed the title Smallbinary Reduce build size by 33% Feb 21, 2017
@@ -155,6 +162,33 @@ gulp.task('browserify', cb => {
runSequence('browserify-lighthouse', 'browserify-other', cb);
});

gulp.task('compilejs', () => {
const opts = {
mangle: false, // Preserve array notation property access. Needed for artifact name lookups.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Interesting how does this disrupt array notation specifically? I had thought mangle was fine unless Function.name/with/eval though I'm not familiar with uglify-harmony gotchas

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's pretty tricky to track down where things break down, but without mangle: false, the audit names get rewritten:

screen shot 2017-02-21 at 8 55 57 am

screen shot 2017-02-21 at 8 56 25 am

Hard to say if it also creates other errors.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh right because we do use Function.name there with the gatherer class name, maybe update that comment then? The accessors seems like the secondary issue to the primary problem of the gatherer being renamed :)

@paulirish
Copy link
Member

In build-traceviewer-module.js we use babel to do some lightweight minification:

transformed = babel.transform(transformed, {
plugins: ['transform-es2015-destructuring'],
comments: false, // Output comments in generated output.
compact: true // Do not include superfluous whitespace characters and line terminators.
}).code;

are you open to using that? this way we wouldn't have to deal with disabling a bunch of options like mangle and sideEffects.

@ebidel
Copy link
Contributor Author

ebidel commented Feb 22, 2017

For the life of me, I couldn't get babel, typescript, or closure to play nicely.

  • Closure - I wanted closure because it produces the smallest output, but it is really slow on 2.4MB of JS and does not support a non-transpiled option.
  • Babel - https://github.com/babel/babili looked the most promising (to avoid transpilation) but it choked after a certain amount of code and stopped minifying it.
  • Typescript - removes comments but does not minify.

In terms of filesize reduction, I didn't find disabling a few compressor settings to adversely effect file size that much.

compress: { // turn off a bunch stuff so we don't create unexpected errors.
unused: false,
properties: false,
dead_code: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't dead code supposed to be caught by ESLint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even if it does, we're not running eslint over the browserified bundle.

@paulirish
Copy link
Member

I put up #1765 which replaces uglify with babel. It has one benefit of not adding any new dependencies at all. :)

@paulirish paulirish merged commit 7c03886 into master Feb 23, 2017
@paulirish paulirish deleted the smallbinary branch February 23, 2017 06:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants