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

i18n: remove default granularity values from formatter #13839

Merged
merged 24 commits into from
Apr 27, 2022
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions docs/new-audits.md
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,28 @@ 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 either be between 0 - 1 (excluding zero), or any positive integer power of 10. Some examples of valid values for `granularity`:

- 0.001
- 0.01
- 0.1
- 0.5
- 1
- 10
- 100
connorjclark marked this conversation as resolved.
Show resolved Hide resolved

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)
Expand Down
2 changes: 1 addition & 1 deletion flow-report/src/summary/category.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ const SummaryTooltip: FunctionComponent<{
{
!displayAsFraction && category.score !== null && <>
<span> · </span>
<span>{i18n.formatNumber(category.score * 100)}</span>
<span>{i18n.formatInteger(category.score * 100)}</span>
</>
}
</div>
Expand Down
12 changes: 7 additions & 5 deletions lighthouse-core/util-commonjs.js
Original file line number Diff line number Diff line change
Expand Up @@ -433,10 +433,11 @@ class Util {
break;
case 'devtools': {
const {cpuSlowdownMultiplier, requestLatencyMs} = throttling;
// eslint-disable-next-line max-len
cpuThrottling = `${Util.i18n.formatNumber(cpuSlowdownMultiplier)}x slowdown (DevTools)`;
networkThrottling = `${Util.i18n.formatNumber(requestLatencyMs)}${NBSP}ms HTTP RTT, ` +
`${Util.i18n.formatNumber(throttling.downloadThroughputKbps)}${NBSP}Kbps down, ` +
`${Util.i18n.formatNumber(throttling.uploadThroughputKbps)}${NBSP}Kbps up (DevTools)`;
networkThrottling = `${Util.i18n.formatMilliseconds(requestLatencyMs)} HTTP RTT, ` +
`${Util.i18n.formatKbps(throttling.downloadThroughputKbps)} down, ` +
`${Util.i18n.formatKbps(throttling.uploadThroughputKbps)} up (DevTools)`;

const isSlow4G = () => {
return requestLatencyMs === 150 * 3.75 &&
Expand All @@ -448,9 +449,10 @@ class Util {
}
case 'simulate': {
const {cpuSlowdownMultiplier, rttMs, throughputKbps} = throttling;
// eslint-disable-next-line max-len
cpuThrottling = `${Util.i18n.formatNumber(cpuSlowdownMultiplier)}x slowdown (Simulated)`;
networkThrottling = `${Util.i18n.formatNumber(rttMs)}${NBSP}ms TCP RTT, ` +
`${Util.i18n.formatNumber(throughputKbps)}${NBSP}Kbps throughput (Simulated)`;
networkThrottling = `${Util.i18n.formatMilliseconds(rttMs)} TCP RTT, ` +
`${Util.i18n.formatKbps(throughputKbps)} throughput (Simulated)`;

const isSlow4G = () => {
return rttMs === 150 && throughputKbps === 1.6 * 1024;
Expand Down
11 changes: 6 additions & 5 deletions report/renderer/details-renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -79,9 +79,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);
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
const textEl = this._renderText(value);
textEl.title = Util.i18n.formatBytes(details.value);
return textEl;
Expand All @@ -92,9 +91,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);
Expand Down Expand Up @@ -173,7 +174,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;
Expand Down
177 changes: 116 additions & 61 deletions report/renderer/i18n.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,25 +22,64 @@ export class I18n {
// When testing, use a locale with more exciting numeric formatting.
if (locale === 'en-XA') locale = 'de';

this._numberDateLocale = locale;
this._numberFormatter = new Intl.NumberFormat(locale);
this._percentFormatter = new Intl.NumberFormat(locale, {style: 'percent'});
this._locale = locale;
this._strings = strings;
}

get strings() {
return this._strings;
}

/**
* @param {number} number
* @param {number|undefined} granularity
* @param {Intl.NumberFormatOptions} opts
Copy link
Member

Choose a reason for hiding this comment

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

nit: seems like we can make granularity and opts optional. opts even has a default value.

Copy link
Member

Choose a reason for hiding this comment

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

Bump on this if it's not too much work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

just now: I added = to the opts type to better reflect how this method is used.

granularity should remain undefined because all the use cases here are passing a value explicitly, even if it may be undefined.

all this stuff is pretty subjective i think. if you feel strongly I'll make the change.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah that's fine with me

* @return {string}
*/
_formatNumberWithGranularity(number, granularity, opts = {}) {
if (granularity !== undefined) {
Copy link
Member

Choose a reason for hiding this comment

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

it does feel like we're working too hard to maintain granularity which was kind of a haphazard choice long ago, but it's unfortunately in the LHR, so...:(

opts = {...opts};
const log10 = -Math.log10(granularity);
if (!Number.isFinite(log10) || (granularity > 1 && Math.floor(log10) !== log10)) {
throw new Error(`granularity of ${granularity} is invalid`);
}

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;
} else if (Math.abs(number) <= 0.0005) {
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
// Also avoids "-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 in full.
* @return {string}
*/
formatNumber(number, granularity = undefined) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: optional params are already undefined by default

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Even so, I think having a second way to represent "this is optional" is nice.

Copy link
Member

Choose a reason for hiding this comment

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

I'm definitely for not including = undefined for optional params

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I don't think we do this anywhere else. I still prefer without = undefined

return this._formatNumberWithGranularity(number, granularity);
}

/**
* Format integer.
* Just like {@link formatNumber} but uses a granularity of 1, rounding to the nearest
* whole number.
* @param {number} number
* @return {string}
*/
formatNumber(number, granularity = 0.1) {
const coarseValue = Math.round(number / granularity) * granularity;
return this._numberFormatter.format(coarseValue);
formatInteger(number) {
return this._formatNumberWithGranularity(number, 1);
}

/**
Expand All @@ -49,92 +88,99 @@ export class I18n {
* @return {string}
*/
formatPercent(number) {
return this._percentFormatter.format(number);
return new Intl.NumberFormat(this._locale, {style: 'percent'}).format(number);
}

/**
* @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) {
const formatter = this._byteFormatterForGranularity(granularity);
const kbs = formatter.format(Math.round(size / 1024 / granularity) * granularity);
return `${kbs}${NBSP2}KiB`;
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) {
const formatter = this._byteFormatterForGranularity(granularity);
const kbs = formatter.format(Math.round(size / (1024 ** 2) / granularity) * granularity);
return `${kbs}${NBSP2}MiB`;
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) {
const formatter = this._byteFormatterForGranularity(granularity);
const kbs = formatter.format(Math.round(size / granularity) * granularity);
return `${kbs}${NBSP2}bytes`;
return this._formatNumberWithGranularity(size, granularity, {
style: 'unit',
unit: 'byte',
unitDisplay: 'long',
});
}

/**
* @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.formatNumber(size, granularity) + '\xa0B';
return this._formatNumberWithGranularity(size, granularity, {
style: 'unit',
unit: 'byte',
unitDisplay: 'narrow',
});
}

/**
* Format bytes with a constant number of fractional digits, i.e. for a granularity of 0.1, 10 becomes '10.0'
* @param {number} granularity Controls how coarse the displayed value is
* @return {Intl.NumberFormat}
* @param {number} size
* @param {number=} granularity Controls how coarse the displayed value is.
* If undefined, the number will be displayed in full.
* @return {string}
*/
_byteFormatterForGranularity(granularity) {
// assume any granularity above 1 will not contain fractional parts, i.e. will never be 1.5
let numberOfFractionDigits = 0;
if (granularity < 1) {
numberOfFractionDigits = -Math.floor(Math.log10(granularity));
}

return new Intl.NumberFormat(this._numberDateLocale, {
...this._numberFormatter.resolvedOptions(),
maximumFractionDigits: numberOfFractionDigits,
minimumFractionDigits: numberOfFractionDigits,
formatKbps(size, granularity = undefined) {
return this._formatNumberWithGranularity(size, granularity, {
style: 'unit',
unit: 'kilobit-per-second',
unitDisplay: 'short',
});
}

/**
* @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) {
const coarseTime = Math.round(ms / granularity) * granularity;
return coarseTime === 0
? `${this._numberFormatter.format(0)}${NBSP2}ms`
: `${this._numberFormatter.format(coarseTime)}${NBSP2}ms`;
formatMilliseconds(ms, granularity = undefined) {
return this._formatNumberWithGranularity(ms, granularity, {
style: 'unit',
unit: 'millisecond',
unitDisplay: 'short',
});
}

/**
* @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) {
const coarseTime = Math.round(ms / 1000 / granularity) * granularity;
return `${this._numberFormatter.format(coarseTime)}${NBSP2}s`;
formatSeconds(ms, granularity = undefined) {
return this._formatNumberWithGranularity(ms / 1000, granularity, {
style: 'unit',
unit: 'second',
unitDisplay: 'short',
});
}

/**
Expand All @@ -154,10 +200,10 @@ export class I18n {
// and https://github.com/GoogleChrome/lighthouse/pull/9822
let formatter;
try {
formatter = new Intl.DateTimeFormat(this._numberDateLocale, options);
formatter = new Intl.DateTimeFormat(this._locale, options);
} catch (err) {
options.timeZone = 'UTC';
formatter = new Intl.DateTimeFormat(this._numberDateLocale, options);
formatter = new Intl.DateTimeFormat(this._locale, options);
}

return formatter.format(new Date(date));
Expand All @@ -169,6 +215,10 @@ export class I18n {
* @return {string}
*/
formatDuration(timeInMilliseconds) {
// There is a proposal for a Intl.DurationFormat.
// https://github.com/tc39/proposal-intl-duration-format
// Until then, we do things a bit more manually.

let timeInSeconds = timeInMilliseconds / 1000;
if (Math.round(timeInSeconds) === 0) {
return 'None';
Expand All @@ -177,19 +227,24 @@ export class I18n {
/** @type {Array<string>} */
const parts = [];
/** @type {Record<string, number>} */
const unitLabels = {
d: 60 * 60 * 24,
h: 60 * 60,
m: 60,
s: 1,
const unitToSecondsPer = {
day: 60 * 60 * 24,
hour: 60 * 60,
minute: 60,
second: 1,
};

Object.keys(unitLabels).forEach(label => {
const unit = unitLabels[label];
const numberOfUnits = Math.floor(timeInSeconds / unit);
Object.keys(unitToSecondsPer).forEach(unit => {
const secondsPerUnit = unitToSecondsPer[unit];
const numberOfUnits = Math.floor(timeInSeconds / secondsPerUnit);
if (numberOfUnits > 0) {
timeInSeconds -= numberOfUnits * unit;
parts.push(`${numberOfUnits}\xa0${label}`);
timeInSeconds -= numberOfUnits * secondsPerUnit;
const part = this._formatNumberWithGranularity(numberOfUnits, 1, {
style: 'unit',
unit,
unitDisplay: 'narrow',
});
parts.push(part);
}
});

Expand Down
Loading