i18n: remove default granularity values from formatter (#13839)

2022-04-27 13:49:04 -07:00 · 2022-04-27 13:49:04 -07:00 · 83121be132
--- a/docs/new-audits.md
+++ b/docs/new-audits.md
@ -31,8 +31,27 @@ An audit can return a number of different [detail types](https://github.com/Goog
 | `'url'`                   | Network Resource      | we will make it a pretty link          |
 | `'thumbnail'`             | Image Resource        | same as above, but we show a thumbnail |
 | `'link'`                  | -                     | arbitrary link / url combination       |
+| `'bytes'`                 | -                     | value is in bytes but formatted as KiB |
 | `'text'\|'ms'\|'numeric'` | -                     |                                        |

+### Granularity
+
+The following detail types accept a `granularity` field:
+
+- `bytes`
+- `ms`
+- `numeric`
+
+`granularity` must be an integer power of 10. Some examples of valid values for `granularity`:
+
+- 0.001
+- 0.01
+- 0.1
+- 1
+- 10
+- 100
+
+The formatted value will be rounded to that nearest number. If not provided, the default is `0.1` (except for `ms`, which is `10`).

 <!--- https://docs.google.com/document/d/1KS6PGPYDfE_TWrRdw55Rd67P-g_MU4KdMetT3cTPHjI/edit#heading=h.32w9jjm4c70w -->
 ![Detail type examples](../assets/detail-type-examples.png)
--- a/lighthouse-core/util-commonjs.js
+++ b/lighthouse-core/util-commonjs.js
@ -433,11 +433,9 @@ class Util {
        break;
      case 'devtools': {
        const {cpuSlowdownMultiplier, requestLatencyMs} = throttling;
-        // TODO: better api in i18n formatter such that this isn't needed.
-        const cpuGranularity = Number.isInteger(cpuSlowdownMultiplier) ? 1 : 0.1;
        // eslint-disable-next-line max-len
-        cpuThrottling = `${Util.i18n.formatNumber(cpuSlowdownMultiplier, cpuGranularity)}x slowdown (DevTools)`;
-        networkThrottling = `${Util.i18n.formatMilliseconds(requestLatencyMs, 1)} HTTP RTT, ` +
+        cpuThrottling = `${Util.i18n.formatNumber(cpuSlowdownMultiplier)}x slowdown (DevTools)`;
+        networkThrottling = `${Util.i18n.formatMilliseconds(requestLatencyMs)} HTTP RTT, ` +
          `${Util.i18n.formatKbps(throttling.downloadThroughputKbps)} down, ` +
          `${Util.i18n.formatKbps(throttling.uploadThroughputKbps)} up (DevTools)`;

@ -451,10 +449,8 @@ class Util {
      }
      case 'simulate': {
        const {cpuSlowdownMultiplier, rttMs, throughputKbps} = throttling;
-        // TODO: better api in i18n formatter such that this isn't needed.
-        const cpuGranularity = Number.isInteger(cpuSlowdownMultiplier) ? 1 : 0.1;
        // eslint-disable-next-line max-len
-        cpuThrottling = `${Util.i18n.formatNumber(cpuSlowdownMultiplier, cpuGranularity)}x slowdown (Simulated)`;
+        cpuThrottling = `${Util.i18n.formatNumber(cpuSlowdownMultiplier)}x slowdown (Simulated)`;
        networkThrottling = `${Util.i18n.formatMilliseconds(rttMs)} TCP RTT, ` +
          `${Util.i18n.formatKbps(throughputKbps)} throughput (Simulated)`;

--- a/report/renderer/details-renderer.js
+++ b/report/renderer/details-renderer.js
@ -78,9 +78,8 @@ export class DetailsRenderer {
   * @return {Element}
   */
  _renderBytes(details) {
-    // TODO: handle displayUnit once we have something other than 'kb'
-    // Note that 'kb' is historical and actually represents KiB.
-    const value = Util.i18n.formatBytesToKiB(details.value, details.granularity);
+    // TODO: handle displayUnit once we have something other than 'KiB'
+    const value = Util.i18n.formatBytesToKiB(details.value, details.granularity || 0.1);
    const textEl = this._renderText(value);
    textEl.title = Util.i18n.formatBytes(details.value);
    return textEl;
@ -91,9 +90,11 @@ export class DetailsRenderer {
   * @return {Element}
   */
  _renderMilliseconds(details) {
-    let value = Util.i18n.formatMilliseconds(details.value, details.granularity);
+    let value;
    if (details.displayUnit === 'duration') {
      value = Util.i18n.formatDuration(details.value);
+    } else {
+      value = Util.i18n.formatMilliseconds(details.value, details.granularity || 10);
    }

    return this._renderText(value);
@ -172,7 +173,7 @@ export class DetailsRenderer {
   * @return {Element}
   */
  _renderNumeric(details) {
-    const value = Util.i18n.formatNumber(details.value, details.granularity);
+    const value = Util.i18n.formatNumber(details.value, details.granularity || 0.1);
    const element = this._dom.createElement('div', 'lh-numeric');
    element.textContent = value;
    return element;
--- a/report/renderer/i18n.js
+++ b/report/renderer/i18n.js
@ -31,37 +31,44 @@ export class I18n {

  /**
   * @param {number} number
-   * @param {number} granularity
-   * @param {Intl.NumberFormatOptions} opts
+   * @param {number|undefined} granularity
+   * @param {Intl.NumberFormatOptions=} opts
   * @return {string}
   */
  _formatNumberWithGranularity(number, granularity, opts = {}) {
-    opts = {...opts};
-    const log10 = -Math.log10(granularity);
-    if (!Number.isFinite(log10) || (granularity > 1 && !Number.isInteger(log10))) {
-      console.warn(`granularity of ${granularity} is invalid, defaulting to value of 1`);
-      granularity = 1;
+    if (granularity !== undefined) {
+      const log10 = -Math.log10(granularity);
+      if (!Number.isInteger(log10)) {
+        console.warn(`granularity of ${granularity} is invalid. Using 1 instead`);
+        granularity = 1;
+      }
+
+      if (granularity < 1) {
+        opts = {...opts};
+        opts.minimumFractionDigits = opts.maximumFractionDigits = Math.ceil(log10);
+      }
+
+      number = Math.round(number / granularity) * granularity;
+
+      // Avoid displaying a negative value that rounds to zero as "0".
+      if (Object.is(number, -0)) number = 0;
+    } else if (Math.abs(number) < 0.0005) {
+      // Also avoids "-0".
+      number = 0;
    }

-    if (granularity < 1) {
-      opts.minimumFractionDigits = opts.maximumFractionDigits = Math.ceil(log10);
-    }
-
-    number = Math.round(number / granularity) * granularity;
-
-    // Avoid displaying a negative value that rounds to zero as "0".
-    if (Object.is(number, -0)) number = 0;
-
    return new Intl.NumberFormat(this._locale, opts).format(number).replace(' ', NBSP2);
  }

  /**
   * Format number.
   * @param {number} number
-   * @param {number=} granularity Number of decimal places to include. Defaults to 0.1.
+   * @param {number=} granularity Controls how coarse the displayed value is.
+   *                              If undefined, the number will be displayed as described
+   *                              by the Intl defaults: tinyurl.com/7s67w5x7
   * @return {string}
   */
-  formatNumber(number, granularity = 0.1) {
+  formatNumber(number, granularity) {
    return this._formatNumberWithGranularity(number, granularity);
  }

@ -87,25 +94,28 @@ export class I18n {

  /**
   * @param {number} size
-   * @param {number=} granularity Controls how coarse the displayed value is, defaults to 0.1
+   * @param {number=} granularity Controls how coarse the displayed value is.
+   *                              If undefined, the number will be displayed in full.
   * @return {string}
   */
-  formatBytesToKiB(size, granularity = 0.1) {
+  formatBytesToKiB(size, granularity = undefined) {
    return this._formatNumberWithGranularity(size / KiB, granularity) + `${NBSP2}KiB`;
  }

  /**
   * @param {number} size
-   * @param {number=} granularity Controls how coarse the displayed value is, defaults to 0.1
+   * @param {number=} granularity Controls how coarse the displayed value is.
+   *                              If undefined, the number will be displayed in full.
   * @return {string}
   */
-  formatBytesToMiB(size, granularity = 0.1) {
+  formatBytesToMiB(size, granularity = undefined) {
    return this._formatNumberWithGranularity(size / MiB, granularity) + `${NBSP2}MiB`;
  }

  /**
   * @param {number} size
-   * @param {number=} granularity Controls how coarse the displayed value is, defaults to 1
+   * @param {number=} granularity Controls how coarse the displayed value is.
+   *                              If undefined, the number will be displayed in full.
   * @return {string}
   */
  formatBytes(size, granularity = 1) {
@ -118,10 +128,11 @@ export class I18n {

  /**
   * @param {number} size
-   * @param {number=} granularity Controls how coarse the displayed value is, defaults to 0.1
+   * @param {number=} granularity Controls how coarse the displayed value is.
+   *                              If undefined, the number will be displayed in full.
   * @return {string}
   */
-  formatBytesWithBestUnit(size, granularity = 0.1) {
+  formatBytesWithBestUnit(size, granularity = undefined) {
    if (size >= MiB) return this.formatBytesToMiB(size, granularity);
    if (size >= KiB) return this.formatBytesToKiB(size, granularity);
    return this._formatNumberWithGranularity(size, granularity, {
@ -133,10 +144,11 @@ export class I18n {

  /**
   * @param {number} size
-   * @param {number=} granularity Controls how coarse the displayed value is, defaults to 1
+   * @param {number=} granularity Controls how coarse the displayed value is.
+   *                              If undefined, the number will be displayed in full.
   * @return {string}
   */
-  formatKbps(size, granularity = 1) {
+  formatKbps(size, granularity = undefined) {
    return this._formatNumberWithGranularity(size, granularity, {
      style: 'unit',
      unit: 'kilobit-per-second',
@ -146,10 +158,11 @@ export class I18n {

  /**
   * @param {number} ms
-   * @param {number=} granularity Controls how coarse the displayed value is, defaults to 10
+   * @param {number=} granularity Controls how coarse the displayed value is.
+   *                              If undefined, the number will be displayed in full.
   * @return {string}
   */
-  formatMilliseconds(ms, granularity = 10) {
+  formatMilliseconds(ms, granularity = undefined) {
    return this._formatNumberWithGranularity(ms, granularity, {
      style: 'unit',
      unit: 'millisecond',
@ -159,10 +172,11 @@ export class I18n {

  /**
   * @param {number} ms
-   * @param {number=} granularity Controls how coarse the displayed value is, defaults to 0.1
+   * @param {number=} granularity Controls how coarse the displayed value is.
+   *                              If undefined, the number will be displayed in full.
   * @return {string}
   */
-  formatSeconds(ms, granularity = 0.1) {
+  formatSeconds(ms, granularity = undefined) {
    return this._formatNumberWithGranularity(ms / 1000, granularity, {
      style: 'unit',
      unit: 'second',
--- a/report/renderer/util.js
+++ b/report/renderer/util.js
@ -429,11 +429,9 @@ class Util {
        break;
      case 'devtools': {
        const {cpuSlowdownMultiplier, requestLatencyMs} = throttling;
-        // TODO: better api in i18n formatter such that this isn't needed.
-        const cpuGranularity = Number.isInteger(cpuSlowdownMultiplier) ? 1 : 0.1;
        // eslint-disable-next-line max-len
-        cpuThrottling = `${Util.i18n.formatNumber(cpuSlowdownMultiplier, cpuGranularity)}x slowdown (DevTools)`;
-        networkThrottling = `${Util.i18n.formatMilliseconds(requestLatencyMs, 1)} HTTP RTT, ` +
+        cpuThrottling = `${Util.i18n.formatNumber(cpuSlowdownMultiplier)}x slowdown (DevTools)`;
+        networkThrottling = `${Util.i18n.formatMilliseconds(requestLatencyMs)} HTTP RTT, ` +
          `${Util.i18n.formatKbps(throttling.downloadThroughputKbps)} down, ` +
          `${Util.i18n.formatKbps(throttling.uploadThroughputKbps)} up (DevTools)`;

@ -447,10 +445,8 @@ class Util {
      }
      case 'simulate': {
        const {cpuSlowdownMultiplier, rttMs, throughputKbps} = throttling;
-        // TODO: better api in i18n formatter such that this isn't needed.
-        const cpuGranularity = Number.isInteger(cpuSlowdownMultiplier) ? 1 : 0.1;
        // eslint-disable-next-line max-len
-        cpuThrottling = `${Util.i18n.formatNumber(cpuSlowdownMultiplier, cpuGranularity)}x slowdown (Simulated)`;
+        cpuThrottling = `${Util.i18n.formatNumber(cpuSlowdownMultiplier)}x slowdown (Simulated)`;
        networkThrottling = `${Util.i18n.formatMilliseconds(rttMs)} TCP RTT, ` +
          `${Util.i18n.formatKbps(throughputKbps)} throughput (Simulated)`;

--- a/report/test/renderer/details-renderer-test.js
+++ b/report/test/renderer/details-renderer-test.js
@ -86,6 +86,55 @@ describe('DetailsRenderer', () => {
          '--thumbnail not set');
    });

+    it('renders with default granularity', () => {
+      const el = renderer.render({
+        type: 'table',
+        headings: [
+          {text: '', key: 'bytes', itemType: 'bytes'},
+          {text: '', key: 'numeric', itemType: 'numeric'},
+          {text: '', key: 'ms', itemType: 'ms'},
+          // Verify that 0 is ignored.
+          {text: '', key: 'ms', itemType: 'ms', granularity: 0},
+        ],
+        items: [
+          {
+            bytes: 1234.567,
+            numeric: 1234.567,
+            ms: 1234.567,
+          },
+        ],
+      });
+
+      assert.equal(el.querySelectorAll('td').length, 4, 'did not render table cells');
+      assert.equal(el.querySelectorAll('td')[0].textContent, '1.2\xa0KiB');
+      assert.equal(el.querySelectorAll('td')[1].textContent, '1,234.6');
+      assert.equal(el.querySelectorAll('td')[2].textContent, '1,230\xa0ms');
+      assert.equal(el.querySelectorAll('td')[3].textContent, '1,230\xa0ms');
+    });
+
+    it('renders with custom granularity', () => {
+      const el = renderer.render({
+        type: 'table',
+        headings: [
+          {text: '', key: 'bytes', itemType: 'bytes', granularity: 0.01},
+          {text: '', key: 'numeric', itemType: 'numeric', granularity: 100},
+          {text: '', key: 'ms', itemType: 'ms', granularity: 1},
+        ],
+        items: [
+          {
+            bytes: 1234.567,
+            numeric: 1234.567,
+            ms: 1234.567,
+          },
+        ],
+      });
+
+      assert.equal(el.querySelectorAll('td').length, 3, 'did not render table cells');
+      assert.equal(el.querySelectorAll('td')[0].textContent, '1.21\xa0KiB');
+      assert.equal(el.querySelectorAll('td')[1].textContent, '1,200');
+      assert.equal(el.querySelectorAll('td')[2].textContent, '1,235\xa0ms');
+    });
+
    it('renders critical request chains', () => {
      const details = {
        type: 'criticalrequestchain',
--- a/report/test/renderer/i18n-test.js
+++ b/report/test/renderer/i18n-test.js
@ -21,9 +21,27 @@ const NBSP = '\xa0';
 describe('util helpers', () => {
  it('formats a number', () => {
    const i18n = new I18n('en', {...Util.UIStrings});
-    assert.strictEqual(i18n.formatNumber(10), '10.0');
-    assert.strictEqual(i18n.formatNumber(100.01), '100.0');
-    assert.strictEqual(i18n.formatNumber(13000.456), '13,000.5');
+    assert.strictEqual(i18n.formatNumber(10), '10');
+    assert.strictEqual(i18n.formatNumber(100.01), '100.01');
+    assert.strictEqual(i18n.formatNumber(13000.456), '13,000.456');
+    assert.strictEqual(i18n.formatNumber(13000.456444), '13,000.456');
+
+    assert.strictEqual(i18n.formatNumber(10, 0.1), '10.0');
+    assert.strictEqual(i18n.formatNumber(100.01, 0.1), '100.0');
+    assert.strictEqual(i18n.formatNumber(13000.456, 0.1), '13,000.5');
+
+    assert.strictEqual(i18n.formatNumber(0), '0');
+    assert.strictEqual(i18n.formatNumber(-0), '0');
+    assert.strictEqual(i18n.formatNumber(-0, 0.1), '0.0');
+    assert.strictEqual(i18n.formatNumber(0.000001), '0');
+    assert.strictEqual(i18n.formatNumber(-0.000001), '0');
+    assert.strictEqual(i18n.formatNumber(0.000001, 0.1), '0.0');
+    assert.strictEqual(i18n.formatNumber(-0.000001, 0.1), '0.0');
+
+    assert.strictEqual(i18n.formatNumber(10), '10');
+    assert.strictEqual(i18n.formatNumber(100.01), '100.01');
+    assert.strictEqual(i18n.formatNumber(13000.456, 0.1), '13,000.5');
+
    assert.strictEqual(i18n.formatInteger(10), '10');
    assert.strictEqual(i18n.formatInteger(100.01), '100');
    assert.strictEqual(i18n.formatInteger(13000.6), '13,001');
@ -41,9 +59,10 @@ describe('util helpers', () => {

  it('formats bytes', () => {
    const i18n = new I18n('en', {...Util.UIStrings});
-    assert.equal(i18n.formatBytesToKiB(100), `0.1${NBSP}KiB`);
-    assert.equal(i18n.formatBytesToKiB(2000), `2.0${NBSP}KiB`);
-    assert.equal(i18n.formatBytesToKiB(1014 * 1024), `1,014.0${NBSP}KiB`);
+    assert.equal(i18n.formatBytesToKiB(100), `0.098${NBSP}KiB`);
+    assert.equal(i18n.formatBytesToKiB(100, 0.1), `0.1${NBSP}KiB`);
+    assert.equal(i18n.formatBytesToKiB(2000, 0.1), `2.0${NBSP}KiB`);
+    assert.equal(i18n.formatBytesToKiB(1014 * 1024, 0.1), `1,014.0${NBSP}KiB`);
  });

  it('formats bytes with different granularities', () => {
@ -59,11 +78,6 @@ describe('util helpers', () => {
    assert.strictEqual(i18n.formatBytes(15.12345, granularity), `15${NBSP}bytes`);
    assert.strictEqual(i18n.formatBytes(15.54321, granularity), `16${NBSP}bytes`);

-    granularity = 0.5;
-    assert.strictEqual(i18n.formatBytes(15.0, granularity), `15.0${NBSP}bytes`);
-    assert.strictEqual(i18n.formatBytes(15.12345, granularity), `15.0${NBSP}bytes`);
-    assert.strictEqual(i18n.formatBytes(15.54321, granularity), `15.5${NBSP}bytes`);
-
    granularity = 0.1;
    assert.strictEqual(i18n.formatBytes(15.0, granularity), `15.0${NBSP}bytes`);
    assert.strictEqual(i18n.formatBytes(15.12345, granularity), `15.1${NBSP}bytes`);
@ -75,6 +89,21 @@ describe('util helpers', () => {
    assert.strictEqual(i18n.formatBytes(15.19999, granularity), `15.20${NBSP}bytes`);
  });

+  it('formats bytes with invalid granularity', () => {
+    const i18n = new I18n('en', {...Util.UIStrings});
+    const granularity = 0.5;
+    const originalWarn = console.warn;
+
+    try {
+      console.warn = () => {};
+      assert.strictEqual(i18n.formatBytes(15.0, granularity), `15${NBSP}bytes`);
+      assert.strictEqual(i18n.formatBytes(15.12345, granularity), `15${NBSP}bytes`);
+      assert.strictEqual(i18n.formatBytes(15.54321, granularity), `16${NBSP}bytes`);
+    } finally {
+      console.warn = originalWarn;
+    }
+  });
+
  it('formats kibibytes with different granularities', () => {
    const i18n = new I18n('en', {...Util.UIStrings});

@ -95,7 +124,7 @@ describe('util helpers', () => {

  it('formats ms', () => {
    const i18n = new I18n('en', {...Util.UIStrings});
-    assert.equal(i18n.formatMilliseconds(123), `120${NBSP}ms`);
+    assert.equal(i18n.formatMilliseconds(123, 10), `120${NBSP}ms`);
    assert.equal(i18n.formatMilliseconds(2456.5, 0.1), `2,456.5${NBSP}ms`);
    assert.equal(i18n.formatMilliseconds(0.000001), `0${NBSP}ms`);
    assert.equal(i18n.formatMilliseconds(-0.000001), `0${NBSP}ms`);
@ -129,10 +158,10 @@ describe('util helpers', () => {
    const number = 12346.858558;

    const i18n = new I18n('de', {...Util.UIStrings});
-    assert.strictEqual(i18n.formatNumber(number), '12.346,9');
-    assert.strictEqual(i18n.formatBytesToKiB(number), `12,1${NBSP}KiB`);
-    assert.strictEqual(i18n.formatMilliseconds(number), `12.350${NBSP}ms`);
-    assert.strictEqual(i18n.formatSeconds(number), `12,3${NBSP}Sek.`);
+    assert.strictEqual(i18n.formatNumber(number), '12.346,859');
+    assert.strictEqual(i18n.formatBytesToKiB(number, 0.1), `12,1${NBSP}KiB`);
+    assert.strictEqual(i18n.formatMilliseconds(number, 10), `12.350${NBSP}ms`);
+    assert.strictEqual(i18n.formatSeconds(number), `12,347${NBSP}Sek.`);
  });

  it('uses decimal comma with en-XA test locale', () => {
@ -140,10 +169,10 @@ describe('util helpers', () => {
    const number = 12346.858558;

    const i18n = new I18n('en-XA', {...Util.UIStrings});
-    assert.strictEqual(i18n.formatNumber(number), '12.346,9');
-    assert.strictEqual(i18n.formatBytesToKiB(number), `12,1${NBSP}KiB`);
-    assert.strictEqual(i18n.formatMilliseconds(number), `12.350${NBSP}ms`);
-    assert.strictEqual(i18n.formatSeconds(number), `12,3${NBSP}Sek.`);
+    assert.strictEqual(i18n.formatNumber(number), '12.346,859');
+    assert.strictEqual(i18n.formatBytesToKiB(number, 0.1), `12,1${NBSP}KiB`);
+    assert.strictEqual(i18n.formatMilliseconds(number, 100), `12.300${NBSP}ms`);
+    assert.strictEqual(i18n.formatSeconds(number, 1), `12${NBSP}Sek.`);
  });

  it('should not crash on unknown locales', () => {