Sort bundle-size summary lists

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,27 +369,26 @@ 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.
           allPotentialApproverTeams.add(
             JSON.stringify(fileApprovers[fileGlob].approvers)
           );
         } else {
-          bundleSizeDeltasAutoApproved.push(
-            `* \`${file}\`: ${formatBundleSizeDelta(bundleSizeDelta)}`
-          );
+          bundleSizeDeltasAutoApproved.push({file, bundleSizeDelta});
         }
       }
     }
 
     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`
+          )
         );
       }
     }

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;

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'

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},
+      ]);
+    });
+  });
+});