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

[Internal] Add bundle size impact reporting #146

Merged
merged 15 commits into from
May 8, 2018

Conversation

liuyenwei
Copy link
Contributor

@liuyenwei liuyenwei commented Apr 28, 2018

This PR adds logic to our dangerfile to report on the bundle size impact of a PR. Currently this is only set as a warning, with the idea that whoever is reviewing the diff can easily see the bundle size impact. If no bundle size change was detected, nothing will be outputted.

The flow is:

  • Get build info for base commit via buildkite API
  • Download stats.json artifact that was uploaded for ^ build.
  • Download stats.json artifact for current build
  • Compare sizes
  • If size difference, report

Example of output (tested this by including moment):
image

@christianvuerings
Copy link
Contributor

christianvuerings commented Apr 28, 2018

Deploy preview for gestalt ready!

Built with commit 6929552

https://deploy-preview-146--gestalt.netlify.com

@chrislloyd
Copy link
Contributor

Is it possible to use Danger for this? Facebook uses it for the same purpose: facebook/react#12682 (comment)

Also, should we possibly be thinking of having a build matrix (dev/prod etc?) Uglify-es will strip all source-maps from the output.

@liuyenwei
Copy link
Contributor Author

@chrislloyd yup, i don't see why not - i'll look into this!

Also, should we possibly be thinking of having a build matrix (dev/prod etc?) Uglify-es will strip all source-maps from the output.

Yup, happy to discuss this more. The way i set up the uglify plugin here was so that it would generate extra .min.js files so that we didn't change the existing bundles. the resulting files here would be:

  • gestalt.js
  • gestalt.min.js
  • gestalt.es.js
  • gestalt.es.min.js

I mainly did this to compare min+gzip size, but I think it would be a good idea to at least generate a minified umd build.

@chrislloyd
Copy link
Contributor

Ah nice. Yeah - excited to see where you go with this :)

@liuyenwei liuyenwei force-pushed the bundlesize branch 23 times, most recently from 45dd4d1 to 5a1c32d Compare May 7, 2018 19:34
Add api key
buildkite-agent maybe??
shrug
mount tmpfs and store stats there
use temp sha
apparently we have to pass buildnumber into artifact download
buildkite artifact downloads using the exact path
@liuyenwei liuyenwei force-pushed the bundlesize branch 2 times, most recently from 08f5fb6 to 79ff8e9 Compare May 8, 2018 00:18
@liuyenwei liuyenwei changed the title WIP: Add bundlesize check Add bundle size impact reporting May 8, 2018
@liuyenwei liuyenwei changed the title Add bundle size impact reporting [Internal] Add bundle size impact reporting May 8, 2018
@liuyenwei liuyenwei requested a review from chrislloyd May 8, 2018 00:43
@@ -7,6 +7,16 @@ steps:
agents:
queue: elastic-builders
- wait
- name: ":hammer_and_wrench: build"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved this here - the build is fast enough

@liuyenwei liuyenwei force-pushed the bundlesize branch 5 times, most recently from c66affe to 148b868 Compare May 8, 2018 08:07
@chrislloyd
Copy link
Contributor

Glad you didn't add moment!

@liuyenwei liuyenwei merged commit 5fc6d84 into pinterest:master May 8, 2018
@liuyenwei liuyenwei deleted the bundlesize branch May 8, 2018 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants