diff --git a/bundle-size/api.js b/bundle-size/api.js index 6d83ed8bc..533217f9e 100644 --- a/bundle-size/api.js +++ b/bundle-size/api.js @@ -17,7 +17,12 @@ const minimatch = require('minimatch'); const sleep = require('sleep-promise'); -const {formatBundleSizeDelta, getCheckFromDatabase} = require('./common'); +const { + getCheckFromDatabase, + formatBundleSizeItem, + formatFileItem, + sortBundleSizeItems, +} = require('./common'); const RETRY_MILLIS = 60000; const RETRY_TIMES = 60; @@ -127,12 +132,20 @@ function extraBundleSizesSummary( if (bundleSizeDeltasRequireApproval.length) { output += '\n## Bundle size changes that require approval\n' + - bundleSizeDeltasRequireApproval.join('\n'); + // If files require approval, we're firstly interested in the largest + // delta, so we sort desc + sortBundleSizeItems(bundleSizeDeltasRequireApproval, 'desc') + .map(formatBundleSizeItem) + .join('\n'); } if (bundleSizeDeltasAutoApproved.length) { output += '\n## Auto-approved bundle size changes\n' + - bundleSizeDeltasAutoApproved.join('\n'); + // If files don't require approval, we're firstly interested in the + // smallest delta, so we sort asc + sortBundleSizeItems(bundleSizeDeltasAutoApproved, 'asc') + .map(formatBundleSizeItem) + .join('\n'); } if (missingBundleSizes.length) { output += @@ -341,15 +354,11 @@ exports.installApiRouter = (app, router, db, githubUtils) => { const missingBundleSizes = []; const allPotentialApproverTeams = new Set(); - // When examining bundle-size check output for a PR, it's helpful to see - // sizes at the extremes, so we sort from largest increase to largest - // decrease. This ordering is reflected in the ordering of each subsection. - const sortedBundleSizes = Object.entries(mainBundleSizes).sort( - (a, b) => a[1] - b[1] - ); - for (const [file, baseBundleSize] of sortedBundleSizes) { + for (const [file, baseBundleSize] of Object.entries(mainBundleSizes)) { if (!(file in prBundleSizes)) { - missingBundleSizes.push(`* \`${file}\`: missing in pull request`); + missingBundleSizes.push( + formatFileItem(file, `missing in pull request`) + ); continue; } @@ -360,9 +369,7 @@ exports.installApiRouter = (app, router, db, githubUtils) => { const bundleSizeDelta = prBundleSizes[file] - baseBundleSize; if (bundleSizeDelta !== 0) { if (fileGlob && bundleSizeDelta >= fileApprovers[fileGlob].threshold) { - bundleSizeDeltasRequireApproval.push( - `* \`${file}\`: ${formatBundleSizeDelta(bundleSizeDelta)}` - ); + bundleSizeDeltasRequireApproval.push({file, bundleSizeDelta}); // Since `.approvers` is an array, it must be stringified to maintain // the Set uniqueness property. @@ -370,9 +377,7 @@ exports.installApiRouter = (app, router, db, githubUtils) => { JSON.stringify(fileApprovers[fileGlob].approvers) ); } else { - bundleSizeDeltasAutoApproved.push( - `* \`${file}\`: ${formatBundleSizeDelta(bundleSizeDelta)}` - ); + bundleSizeDeltasAutoApproved.push({file, bundleSizeDelta}); } } } @@ -380,7 +385,10 @@ exports.installApiRouter = (app, router, db, githubUtils) => { for (const [file, prBundleSize] of Object.entries(prBundleSizes)) { if (!(file in mainBundleSizes)) { missingBundleSizes.push( - `* \`${file}\`: (${prBundleSize} KB) missing on the main branch` + formatFileItem( + file, + `(${prBundleSize} KB) missing on the main branch` + ) ); } } diff --git a/bundle-size/common.js b/bundle-size/common.js index 7db2d7f3e..6c3c2e27b 100644 --- a/bundle-size/common.js +++ b/bundle-size/common.js @@ -49,6 +49,66 @@ exports.getCheckFromDatabase = async (db, headSha) => { * @param {number} delta the bundle size delta in KB. * @return {string} formatted bundle size delta. */ -exports.formatBundleSizeDelta = delta => { +const formatBundleSizeDelta = delta => { return 'Δ ' + (delta >= 0 ? '+' : '') + delta.toFixed(2) + 'KB'; }; + +/** + * @param {string} file + * @param {string} description + * @return {string} + */ +const formatFileItem = (file, description) => `* \`${file}\`: ${description}`; + +exports.formatFileItem = formatFileItem; + +/** + * @param {{file: string, bundleSizeDelta: number}} item + * @return {string} + */ +exports.formatBundleSizeItem = ({file, bundleSizeDelta}) => { + return formatFileItem(file, formatBundleSizeDelta(bundleSizeDelta)); +}; + +/** + * @param {string} file + * @return {string} + */ +const noExtension = file => { + const parts = file.split('.'); + parts.pop(); + return parts.join('.'); +}; + +/** + * @param {{file: string, bundleSizeDelta: number}[]} items + * @param {'asc'|'desc'} sizeOrder + * @return {{file: string, bundleSizeDelta: number}[]} + */ +function sortBundleSizeItems(items, sizeOrder = 'desc') { + const bySize = (a, b) => { + const factor = sizeOrder === 'desc' ? -1 : 1; + return factor * (a.bundleSizeDelta - b.bundleSizeDelta); + }; + // group by filename without extension, so that '.mjs' is always next to its + // equivalent '.js' + const groups = {}; + for (const item of items) { + const name = noExtension(item.file); + const group = (groups[name] = groups[name] || { + items: [], + bundleSizeDelta: item.bundleSizeDelta, + }); + group.items.push(item); + group.bundleSizeDelta = + sizeOrder === 'desc' + ? Math.max(group.bundleSizeDelta, item.bundleSizeDelta) + : Math.min(group.bundleSizeDelta, item.bundleSizeDelta); + } + return Object.values(groups) + .sort(bySize) + .map(({items}) => items.sort(bySize)) + .flat(); +} + +exports.sortBundleSizeItems = sortBundleSizeItems; diff --git a/bundle-size/test/api.test.js b/bundle-size/test/api.test.js index b6aebe78c..e1adb10f1 100644 --- a/bundle-size/test/api.test.js +++ b/bundle-size/test/api.test.js @@ -362,9 +362,9 @@ describe('bundle-size api', () => { title: 'No approval necessary', summary: expect.stringContaining( '## Auto-approved bundle size changes\n' + - '* `dist/v0/amp-truncate-text-0.1.js`: Δ +0.28KB\n' + - '* `dist/v0/amp-ad-0.1.js`: Δ +0.03KB\n' + '* `dist/v0/amp-date-display-0.1.js`: Δ -1.67KB\n' + + '* `dist/v0/amp-ad-0.1.js`: Δ +0.03KB\n' + + '* `dist/v0/amp-truncate-text-0.1.js`: Δ +0.28KB\n' + '## Bundle sizes missing from this PR\n' + '* `dist/v0/amp-anim-0.1.js`: missing in pull request\n' + '* `dist/amp4ads-v0.js`: (11.22 KB) missing on the main branch' diff --git a/bundle-size/test/common.test.js b/bundle-size/test/common.test.js new file mode 100644 index 000000000..092c87581 --- /dev/null +++ b/bundle-size/test/common.test.js @@ -0,0 +1,94 @@ +/** + * Copyright 2022 The AMP HTML Authors. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS-IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +const {sortBundleSizeItems} = require('../common'); + +describe('bundle-size common', () => { + describe('sortBundleSizeItems', () => { + it('sorts desc', async () => { + expect( + sortBundleSizeItems( + [ + {file: 'bar.js', bundleSizeDelta: -0.1}, + {file: 'foo.js', bundleSizeDelta: 0.1}, + ], + 'desc' + ) + ).toEqual([ + {file: 'foo.js', bundleSizeDelta: 0.1}, + {file: 'bar.js', bundleSizeDelta: -0.1}, + ]); + }); + it('sorts asc', async () => { + expect( + sortBundleSizeItems( + [ + {file: 'foo.js', bundleSizeDelta: 0.1}, + {file: 'bar.js', bundleSizeDelta: -0.1}, + ], + 'asc' + ) + ).toEqual([ + {file: 'bar.js', bundleSizeDelta: -0.1}, + {file: 'foo.js', bundleSizeDelta: 0.1}, + ]); + }); + it('groups by filename without extension desc', async () => { + expect( + sortBundleSizeItems( + [ + {file: 'bar.js', bundleSizeDelta: -0.1}, + {file: 'foo.js', bundleSizeDelta: 0.15}, + {file: 'baz.js', bundleSizeDelta: 0.05}, + {file: 'baz.mjs', bundleSizeDelta: 0.08}, + {file: 'bar.mjs', bundleSizeDelta: 0.2}, + {file: 'foo.mjs', bundleSizeDelta: 0.1}, + ], + 'desc' + ) + ).toEqual([ + {file: 'bar.mjs', bundleSizeDelta: 0.2}, + {file: 'bar.js', bundleSizeDelta: -0.1}, + {file: 'foo.js', bundleSizeDelta: 0.15}, + {file: 'foo.mjs', bundleSizeDelta: 0.1}, + {file: 'baz.mjs', bundleSizeDelta: 0.08}, + {file: 'baz.js', bundleSizeDelta: 0.05}, + ]); + }); + it('groups by filename without extension asc', async () => { + expect( + sortBundleSizeItems( + [ + {file: 'bar.js', bundleSizeDelta: -0.1}, + {file: 'foo.js', bundleSizeDelta: 0.15}, + {file: 'baz.js', bundleSizeDelta: 0.05}, + {file: 'baz.mjs', bundleSizeDelta: 0.08}, + {file: 'bar.mjs', bundleSizeDelta: 0.2}, + {file: 'foo.mjs', bundleSizeDelta: 0.1}, + ], + 'asc' + ) + ).toEqual([ + {file: 'bar.js', bundleSizeDelta: -0.1}, + {file: 'bar.mjs', bundleSizeDelta: 0.2}, + {file: 'baz.js', bundleSizeDelta: 0.05}, + {file: 'baz.mjs', bundleSizeDelta: 0.08}, + {file: 'foo.mjs', bundleSizeDelta: 0.1}, + {file: 'foo.js', bundleSizeDelta: 0.15}, + ]); + }); + }); +});