Amphtml: Build Size Check should be Differential

[jridgewell]

Rather than having a maximum build size cap (which we have to bump continuously), we could check the build size difference.

The biggest downside to the max cap is that the changes are cumulative, and only the last PR that finally pushes it over the cap will fail.

This, however, would tell each PR how much it increases the size. So, if I add a simple method and it ends up increasing the size by 1kb, I can be informed.

We can discuss whether there should be a cap to the difference (maybe 1kb?), but I think the biggest use of this would just be a PR integration telling the difference.

[rsimha]

/cc @choumx

[choumx]

Probably in the minority opinion here but I like having a hard cap to avoid growth creep AKA "death by a thousand cuts". In other words, a differential size limit of 0. :)

My rationale is that most AMP engs work on extensions, so having some friction when increasing the size of v0.js, although annoying, is a good thing. It may even incentivize some engs to help contribute long-term fixes to these issues, e.g. #14480.

An eng can find out the differential by looking at how much the size cap has been exceeded, though I guess it's not as easy as having Travis do it for you.

[rsimha]

I'm open to either mode (hard cap or differential), but either way, I'd like for the check to natively affect the status of PRs, instead of failing the Travis PR build.

The tool isn't a GitHub app (https://github.com/siddharthkp/bundlesize/issues/219), but I believe there are other ways to achieve this.

[jpettitt]

FWIW I think the overall size cap (now we're looking at compressed size) is the way to go. As @kristoferbaxter has pointed out it's a proxy metric at best anyway. It would be nice the actual delta was displayed in the pr-check but the total size is what matters.

[rsimha]

Showing the delta will involve doing two builds (takes time) or maintaining some kind of server look up table that maps commits on master to a build size.

Keep the ideas coming. I'll look into this soon :)

[jpettitt]

I don't suppose there is a way to hook the merge then build and auto commit a .master-size-<build>.json ... that would also give us a history of size by merge.

[rsimha]

Assigning to @danielrozenberg for more updates on this.

[lannka]

The overall size is the metric we should keep an eye on, but the current travis setup can catch an innocent PR.

Imagine PR A & PR B both increase size by 0.2K, and we we have 0.3K budget. The 2 PRs will both pass Travis test, and get merged.

Then any following PRs will be blocked.

[danielrozenberg]

@lannka that's correct, which is why we have #17043 :)

[danielrozenberg]

Fixed :smile: