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

[ci] Use dangerJS to report bundle size changes #14587

Merged
merged 35 commits into from
Feb 21, 2019

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Feb 19, 2019

Inspired by facebook/react#11865

Example eps1lon#4 (comment) (browse through versions to see different views)

Abstract

report on any change in bundle size instead of failing on exceeding an arbitrary limit.

Motivation

CI will only ever inform us if a change exceeded a given size limit. This ensures that we keep track of every size increase via version control. However no reduction is explicitly reported. Not only should this be part of the review anyway. It can also hide future increases. Imagine feature A and B reducing the size limit. Now feature C will pass even if it increases the bundle. Now a feature D is proposed that exceeds the limit and now any historic information regarding the bundle size is gone. In addition to that it will mark that build as "failed" even if the change is considered "good".

Therefore I'm proposing to track bundle size for every change committed to this repo and simply adding a report to PRs how this would affect the bundle size.

Strategy

  1. create a size snapshot
    • snapshot has the following shape: { [bundleKey]: { parsed: number, gzip: number } }
    • two snapshots can be compared by comparing their values for a given key. The entries in those values are pairwise subtracted. If the old value is missing the difference is +inf%, if the new value is missing the difference is -100%)
    • example snapshot
  2. Persist that size snapshot for every commit on branches in the mui-org/material-ui repo i.e. every build that is not a PR.
  3. On every PR compare that snapshot to the snapshot of the merge base and report that comparison.

Implementation

Each point in the implementation corresponds to a bullet-point in #Strategy.

  1. We use the same strategy as was used before to calculate bundle size.
    • size-limit has a programmatic API that creates webpack bundles for a given file and reads that size
  • We include the size snapshot of the umd prod build too
  • by depending on test_build we can recycle the build output via CircleCI workspaces
  1. use a s3 bucket
    CircleCI has an obvious solution for persisting a size snapshot via build artifacts. However they state that those are not stored permanently and are only intended to be used around build time. We therefore have to use a different service.
    We can't use any storage system because we couldn't control explicitly who would've access to it. CircleCI exposes environment variables either to every build or only non-fork builds. I didn't find another solution to expose write access to some storage without implicitly giving this away to any PR.
    Which is why I ended up building my own solution on aws.
    1. Expose a public API gateway that can receive notifications about a specific CircleCI build number
    2. That API triggers a lambda function (hosted on https://github.com/eps1lon/persist-size-snapshot)
      1. check if the build is a build of mui-org/material-ui
      2. Determine branch and commit of that build
      3. copy the size-snapshot.json build artifact from that build to a s3 bucket following the pattern root-bucket-name/artifacts/$branch/$commit/size-snapshot.json
    3. The s3 bucket has public read-only access to artifacts/* (for debugging)
    4. The lambda function has write-only access to artifacts/*
    5. API has the lowest rate-limit possible and requires a API key
    6. key is exposed in the CircleCI environment. Mallory can potentially read this token and spam the API. However we are immediately aware of that (the build shows the code that reads the key) and the lambda only reads from our build anyway. Mallory would need to upload a malicious size-snapshot to CircleCI to inject a malicious snapshot into the s3 bucket.
  2. use dangerJS to report the size comparison (same strategy as react)
    1. uses a bot to update a single comment right under the PR
    2. access token to that bot is an environment variable in CircleCI (react has that token even in version control)
    3. the bot only has public_repo access following the docs. It should not be part of the org.

Changes

  • removed the .size-snapshot.json from version control

CI pipeline

  • moved everything related to size limit to a new job size_snapshot that requires test_build
  • The visual is a little bit misleading. The animated gif shows that it actually only depend on test_build
    before
    screenshot from 2019-02-20 10-44-44

after
mui-danger-pipeline

public tokens

All tokens belong to private services. For testing things out it can stay that way. In the future we should move those to accounts that are handle by either board-members or whitelisted core-members (not every core-contributor needs access to those)

React has the token for the sizebot directly in source control and no issues as far as I know. By having this as an environment variable we can at least know when a token has been made public (code change that read the var).

The aws token is for an API gateway that has the lowest rate limit, free tier usage alerts and a 0.01$ limit. Like the bot token we can know if the token was read publicly and can revoke the token at this point.

material-size-bot comment breakdown

The comment includes changes to the main package bundles if the exceed parsed size by 300B or gzip size by 100B. A detailed breakdown is listed in a collapsible table.

The comment in this PR lists every bundle as +Infinity because no bundles are found previously. This displays that this approach is robust to new bundles that are introduced. Once this gets merged the first snapshot will be persisted and any PR that has a merge base after that will include a proper report.

TODO

  • will not fail once proper environment variables are set up
    • CIRCLE_AWS_API_KEY (¨and rename this one to *_TOKEN for consistency it's passed via x-api-key and they're managed as Keys)
    • DANGER_GITHUB_API_TOKEN
  • cleanup diff
  • review own code
  • collect note into readable description
  • change lambda function to consider org builds to be on mui-org/material-ui instead of eps1lon/material-ui

tmp/ was not created on ci
Use circle ci workspaces to persist build output. attach
that workspace in a size_snapshot job. dangerJS will later run in
that job and report size changes. An additional persist_size_snapshot
will later upload the size-snapshot permanently. CircleCI
artifacts are not supposed to be used permanently.
Only "around build time".
createSizeSnapshot script takes care of this
was only removed to speed up CI
- improves code docs
- move some generic utils to utils module
@mui-pr-bot
Copy link

mui-pr-bot commented Feb 20, 2019

This PR introduced some changes to the bundle size.

@material-ui/core: parsed: +Infinity%, gzip: +Infinity%
@material-ui/styles: parsed: +Infinity%, gzip: +Infinity%
@material-ui/system: parsed: +Infinity%, gzip: +Infinity%

Details of bundle changes.

Comparing: 23e10fa...24a3f80

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core/Paper +Infinity%:small_red_triangle: +Infinity%:small_red_triangle: 0 83149 0 21462
@material-ui/core/Paper.esm +Infinity%:small_red_triangle: +Infinity%:small_red_triangle: 0 76728 0 20465
@material-ui/core +Infinity%:small_red_triangle: +Infinity%:small_red_triangle: 0 484828 0 130131
@material-ui/core/styles/createMuiTheme +Infinity%:small_red_triangle: +Infinity%:small_red_triangle: 0 16235 0 5159
@material-ui/styles +Infinity%:small_red_triangle: +Infinity%:small_red_triangle: 0 63856 0 18142
@material-ui/system +Infinity%:small_red_triangle: +Infinity%:small_red_triangle: 0 15915 0 3938
colorManipulator +Infinity%:small_red_triangle: +Infinity%:small_red_triangle: 0 2290 0 838
Button +Infinity%:small_red_triangle: +Infinity%:small_red_triangle: 0 212888 0 61959
Modal +Infinity%:small_red_triangle: +Infinity%:small_red_triangle: 0 211086 0 61685
@material-ui/core/Popper +Infinity%:small_red_triangle: +Infinity%:small_red_triangle: 0 146815 0 46826
@material-ui/core/useMediaQuery +Infinity%:small_red_triangle: +Infinity%:small_red_triangle: 0 9386 0 3563
docs.main +Infinity%:small_red_triangle: +Infinity%:small_red_triangle: 0 676352 0 205143
docs.landing +Infinity%:small_red_triangle: +Infinity%:small_red_triangle: 0 48975 0 10282
packages/material-ui/build/umd/material-ui.production.min.js +Infinity%:small_red_triangle: +Infinity%:small_red_triangle: 0 320462 0 84731

Generated by 🚫 dangerJS against 24a3f80

allows for more useful comparison:
- add means current.size abs change, +inf rel change
- remove means -prev.size abs change, -100% rel change
looks "weird" otherwise
@eps1lon eps1lon marked this pull request as ready for review February 20, 2019 12:32
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

@eps1lon This is significantly better than the current size-limit usage we have 😍! cc @ai. Danger updates the first comment as more work is done on the pull request, so we won't be spammed, perfect. I'm glad we could keep using size-limit :). This change significantly reduces the overhead of keeping track of 1 specific module. We can consider keeping track of all our components 💥! It's not something I would have tried before as one single change to a highly dependent module would have mean updating dozen of values by hand. It would have been too time-consuming.
This change will also save us time, we do no longer have to update the .size-limit.js, then wait for the build to pass to merge a pull request. It was frustrating for our new contributors:
"It's red 😱. What have I done wrong?" cc @TrySound

I would be cautious with this architectural choice. The last time I have tried this strategy, CircleCI was losing track of the workspace when rebuilding. We have a few flaky tests, we need to rebuild from time to time.

dangerfile.js Outdated Show resolved Hide resolved
dangerfile.js Outdated Show resolved Hide resolved
* @param {string[][]} body
* @returns {string}
*/
function generateMDTable(headers, body) {
Copy link
Member

Choose a reason for hiding this comment

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

Note for self: we could have used this abstraction the API markdown generation logic.

dangerfile.js Outdated Show resolved Hide resolved
scripts/utils/index.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@@ -42,6 +42,7 @@
"prettier:all": "yarn babel-node ./scripts/prettier.js write",
"size": "size-limit",
"size:overhead:why": "size-limit --why ./test/size/overhead.js",
"size:snapshot": "node scripts/createSizeSnapshot",
"size:why": "size-limit --why packages/material-ui/build/index.js",
Copy link
Member

Choose a reason for hiding this comment

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

The best command ever! We might be able to kill 1KB by removing the usage of dom-helpers/styles :) cc @joshwooding

capture d ecran 2019-02-21 a 10 55 01

@eps1lon
Copy link
Member Author

eps1lon commented Feb 21, 2019

The last time I have tried this strategy, CircleCI was losing track of the workspace when rebuilding. We have a few flaky tests, we need to rebuild from time to time.

Workspaces are not persisted between workflows (I think this is what you're referring to with builds). They can only be shared between jobs. I found the data flow image really helpful understanding all the different persistence objects.

If we have problems with workspaces between jobs then we can always run the builds before the size snapshot.

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