Integrate `bundlesize` and cap main binary size #14405

dreamofabear · 2018-04-03T23:33:11Z

Checks amp.js on PR builds with max size of 315 KB (current 314.65 KB)
Checks v0.js on push builds with max size of 75 KB (current 74.9 KB)

@rsimha @erwinmombay Needs some extra work for GitHub integration: https://github.com/siddharthkp/bundlesize#2-build-status

rsimha

Two more changes, in addition to the comments below:

In order to run this during PR checks, you'll need to call checkBundleSize after buildRuntime @ line 603.
Were you going to set up CI integration as described in https://github.com/siddharthkp/bundlesize#2-build-status?

rsimha · 2018-04-04T00:31:04Z

package.json

@@ -59,6 +59,7 @@
    "body-parser": "1.18.2",
    "browserify": "16.1.1",
    "browserify-istanbul": "3.0.1",
+    "bundlesize": "^0.17.0",


This should be an exact version. Use:

yarn add --dev --exact bundlesize

Done, thanks.

rsimha · 2018-04-04T00:44:26Z

build-system/pr-check.js

+    const args = compiled
+      ? '-f "./dist/v0.js" -s "75 kB"'
+      : '-f "./dist/amp.js" -s "315 kB"';
+    timedExecOrDie(`npx bundlesize ${args}`);


Based on https://github.com/siddharthkp/bundlesize#cli, shouldn't this be...

timedExecOrDie(`bundlesize ${args}`);

... without the npx?

That only works if bundlesize is installed as a global package. Check out npx: https://medium.com/@maybekatz/introducing-npx-an-npm-package-runner-55f7d4bd282b

dreamofabear · 2018-04-04T21:48:07Z

Weird, Travis appears to build amp.js with a different size vs. my local build on macOS.

Travis:

FAIL  ./dist/amp.js: 332.28KB > maxSize 315KB (gzip)

Local:

 PASS  ./dist/amp.js: 315KB < maxSize 315KB (gzip)

rsimha · 2018-04-04T21:56:34Z

Weird. Travis runs on Linux VMs. I ran this locally on Linux, and the size is 315 KB.

npx bundlesize -f "./dist/amp.js" -s "315 kB"
 PASS  ./dist/amp.js: 315KB < maxSize 315KB (gzip)

rsimha · 2018-04-04T22:05:15Z

@choumx maybe do a debug Travis build where you print the contents of dist/amp.js if it's over the limit, and diff it against your local build?

dreamofabear · 2018-04-05T05:38:24Z

There are actually many differences e.g. how class functions are declared:

It's because we have a different browsers target for Babel on Travis:

amphtml/gulpfile.js

Lines 1100 to 1104 in 970642f

    
           if (process.env.TRAVIS) { 
        
             browsers.push('last 2 versions', 'safari >= 9'); 
        
           } else { 
        
             browsers.push('Last 4 Chrome versions'); 
        
           }

TIL...

dreamofabear · 2018-04-05T05:41:22Z

@erwinmombay Any problems if we also use browsers: ['last 2 versions', 'safari >= 9'] for local dev?

erwinmombay · 2018-04-05T19:29:54Z

@choumx nope. LGTM

rsimha · 2018-04-05T20:44:05Z

gulpfile.js

@@ -797,9 +798,27 @@ function performBuild(watch) {
      buildExtensions({bundleOnlyIfListedInFiles: !watch, watch}),
      compile(watch),
    ]);
+  }).then(() => {
+


Remove this block?

rsimha · 2018-04-05T20:48:15Z

gulpfile.js

+  if (p.status != 0) {
+    log(cyan('bundlesize: ') + red('The main bundle (amp.js/v0.js) has ' +
+        'exceeded its size cap.') + ' This is part of a new effort to reduce ' +
+        'bundle size (see #14392). Please contact @choumx if this blocks you.');


Style nit: Standard warning and error formats used elsewhere in this file are:

log(red('ERROR:'), 'Something bad happened while doing', cyan('foo'), 'to', cyan('bar'), '. Talk to @choumx.'); log(yellow('WARNING:'), 'Something not so bad happened while doing', cyan('foo'), 'to', cyan('bar'), '. Talk to @choumx.');

dreamofabear · 2018-04-05T21:02:08Z

@rsimha Thanks for the quick reviews.

rsimha · 2018-04-05T21:30:48Z

Looks like this tool needs r/w access to commit statuses in order to enable GitHub PR integration. Unfortunately, if we store BUNDLESIZE_GITHUB_TOKEN as a secure variable on Travis, it won't be visible to PR builds, and PR integration is useless on push builds.

See https://github.com/siddharthkp/bundlesize#2-build-status

/cc @cramforce

rsimha · 2018-04-05T21:33:24Z

gulpfile.js

+  const size = compiled ? '75.1kB' : '332.3kB';
+  const cmd = `npx bundlesize -f "${file}" -s "${size}"`;
+  log(green('Running ') + cyan(cmd) + green('...\n'));
+  const p = exec(cmd);


This will result in a warning being printed to advertise the tool's GH integration feature. Not sure it's worth trying to silence it.

https://travis-ci.org/ampproject/amphtml/jobs/362305001#L663-L666

siddharthkp/bundlesize#158 I think it's harmless.

dreamofabear · 2018-04-05T22:04:33Z

Unfortunately, if we store BUNDLESIZE_GITHUB_TOKEN as a secure variable on Travis, it won't be visible to PR builds, and PR integration is useless on push builds.

Huh, so how can PR integration on Travis work in the first place?

rsimha · 2018-04-05T22:24:41Z

I think it's possible on repositories that don't use forks for pull requests.

See the text in the blue box here: https://docs.travis-ci.com/user/environment-variables/#Defining-encrypted-variables-in-.travis.yml

dreamofabear · 2018-04-05T22:27:19Z

Right-o. 😄 Thanks for the link.

* Add bundlesize dep. * Move config from package.jsoon to CLI. * Use exact version. * Also run in integration shard in PR builds. * Debugging amp.js. * Revert debugging, use same Babel browser target in Travis and local dev. * Move logic from pr-check.js to gulpfile.js. * Raghu's comments.

rsimha · 2018-05-02T17:37:30Z

@choumx I've created siddharthkp/bundlesize#219 to track the feasibility of GH integration for AMP, given that we accept PRs from forks.

dreamofabear requested a review from rsimha April 3, 2018 23:33

googlebot added the cla: yes label Apr 3, 2018

rsimha suggested changes Apr 4, 2018

View reviewed changes

rsimha reviewed Apr 4, 2018

View reviewed changes

rsimha approved these changes Apr 4, 2018

View reviewed changes

William Chou added 4 commits April 4, 2018 15:33

Add bundlesize dep.

36ad628

Move config from package.jsoon to CLI.

7711b53

Use exact version.

c0d0031

Also run in integration shard in PR builds.

9a146d9

dreamofabear force-pushed the bundlesize branch from 0894a7e to 9a146d9 Compare April 4, 2018 19:34

dreamofabear force-pushed the bundlesize branch from 9576ce6 to 042fd28 Compare April 5, 2018 00:08

Debugging amp.js.

a1a5a2e

dreamofabear force-pushed the bundlesize branch from 042fd28 to a1a5a2e Compare April 5, 2018 00:37

Revert debugging, use same Babel browser target in Travis and local dev.

79eb7e6

dreamofabear requested a review from erwinmombay April 5, 2018 19:26

Move logic from pr-check.js to gulpfile.js.

82558bd

rsimha approved these changes Apr 5, 2018

View reviewed changes

Raghu's comments.

a8f79ff

rsimha reviewed Apr 5, 2018

View reviewed changes

dreamofabear merged commit 3abc401 into ampproject:master Apr 5, 2018

dreamofabear deleted the bundlesize branch April 5, 2018 22:03

dreamofabear mentioned this pull request Apr 6, 2018

Bundlesize follow-up #14459

Merged

rsimha mentioned this pull request May 4, 2018

Leaner AMP: Stop the bleeding (feeding?) #14394

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate `bundlesize` and cap main binary size #14405

Integrate `bundlesize` and cap main binary size #14405

dreamofabear commented Apr 3, 2018

rsimha left a comment •

edited

Loading

rsimha Apr 4, 2018

dreamofabear Apr 4, 2018

rsimha Apr 4, 2018

dreamofabear Apr 4, 2018

dreamofabear commented Apr 4, 2018 •

edited

Loading

rsimha commented Apr 4, 2018

rsimha commented Apr 4, 2018

dreamofabear commented Apr 5, 2018

dreamofabear commented Apr 5, 2018 •

edited

Loading

erwinmombay commented Apr 5, 2018

rsimha Apr 5, 2018

rsimha Apr 5, 2018

dreamofabear commented Apr 5, 2018

rsimha commented Apr 5, 2018 •

edited

Loading

rsimha Apr 5, 2018

dreamofabear Apr 5, 2018

dreamofabear commented Apr 5, 2018

rsimha commented Apr 5, 2018

dreamofabear commented Apr 5, 2018

rsimha commented May 2, 2018

Integrate bundlesize and cap main binary size #14405

Integrate bundlesize and cap main binary size #14405

Conversation

dreamofabear commented Apr 3, 2018

rsimha left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dreamofabear commented Apr 4, 2018 • edited Loading

rsimha commented Apr 4, 2018

rsimha commented Apr 4, 2018

dreamofabear commented Apr 5, 2018

dreamofabear commented Apr 5, 2018 • edited Loading

erwinmombay commented Apr 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dreamofabear commented Apr 5, 2018

rsimha commented Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dreamofabear commented Apr 5, 2018

rsimha commented Apr 5, 2018

dreamofabear commented Apr 5, 2018

rsimha commented May 2, 2018

Integrate `bundlesize` and cap main binary size #14405

Integrate `bundlesize` and cap main binary size #14405

rsimha left a comment •

edited

Loading

dreamofabear commented Apr 4, 2018 •

edited

Loading

dreamofabear commented Apr 5, 2018 •

edited

Loading

rsimha commented Apr 5, 2018 •

edited

Loading