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

core(i18n): improve Intl polyfill and use it in Util #9584

Merged
merged 4 commits into from
Aug 21, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
26 changes: 14 additions & 12 deletions lighthouse-core/lib/i18n/i18n.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,23 +21,25 @@ const MESSAGE_INSTANCE_ID_REGEX = /(.* \| .*) # (\d+)$/;
const MESSAGE_INSTANCE_ID_QUICK_REGEX = / # \d+$/;

(() => {
// Node usually doesn't come with the locales we want built-in, so load the polyfill if we can.
// Node without full-icu doesn't come with the locales we want built-in. Load the polyfill if needed.
// See https://nodejs.org/api/intl.html#intl_options_for_building_node_js
// The bundler should remove these dependencies, so this will be a no-op in the browser.
Copy link
Collaborator

Choose a reason for hiding this comment

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

might be worth clarifying "no-op".

right now it appears to me that we would override a browser that failed to support those 4 locales with Intl.NumberFormat = undefined, which would be worse than a no-op. Is it more like All our environments where IntlPolyfill is empty are guaranteed to support these 4 locales, so this will be a no-op?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it shouldn't be possible in Chrome (other browsers never see this code because it's only in the bundle, not in the report) but you never know what Chrome builds are out there and at the least it would be good for the code to make it completely clear it's not possible. I'll fix it.


try {
// @ts-ignore
const IntlPolyfill = require('intl');
// In browser environments where we don't need the polyfill, this won't exist
if (!IntlPolyfill.NumberFormat) return;
// @ts-ignore
const IntlPolyfill = require('intl');

// Check if global implementation supports a minimum set of locales.
const minimumLocales = ['en', 'es', 'ru', 'zh'];
Copy link
Member Author

Choose a reason for hiding this comment

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

this should be enough to establish whether Node is full-icu or not, but happy to add or take away from this minimum list.

const supportedLocales = Intl.NumberFormat.supportedLocalesOf(minimumLocales);

if (supportedLocales.length !== minimumLocales.length) {
brendankenny marked this conversation as resolved.
Show resolved Hide resolved
Intl.NumberFormat = IntlPolyfill.NumberFormat;
Intl.DateTimeFormat = IntlPolyfill.DateTimeFormat;

// Intl.PluralRules polyfilled on require().
// @ts-ignore
require('intl-pluralrules');
} catch (_) {
log.warn('i18n', 'Failed to install `intl` polyfill');
}

// intl-pluralrules polyfills itself (with check) on require().
// @ts-ignore
require('intl-pluralrules');
})();


Expand Down
22 changes: 14 additions & 8 deletions lighthouse-core/report/html/renderer/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ class Util {
*/
static formatNumber(number, granularity = 0.1) {
const coarseValue = Math.round(number / granularity) * granularity;
return coarseValue.toLocaleString(Util.numberDateLocale);
return Util.numberFormatter.format(coarseValue);
}

/**
Expand All @@ -201,8 +201,7 @@ class Util {
* @return {string}
*/
static formatBytesToKB(size, granularity = 0.1) {
const kbs = (Math.round(size / 1024 / granularity) * granularity)
.toLocaleString(Util.numberDateLocale);
const kbs = Util.numberFormatter.format(Math.round(size / 1024 / granularity) * granularity);
return `${kbs}${NBSP}KB`;
}

Expand All @@ -213,7 +212,7 @@ class Util {
*/
static formatMilliseconds(ms, granularity = 10) {
const coarseTime = Math.round(ms / granularity) * granularity;
return `${coarseTime.toLocaleString(Util.numberDateLocale)}${NBSP}ms`;
return `${Util.numberFormatter.format(coarseTime)}${NBSP}ms`;
Copy link
Member

Choose a reason for hiding this comment

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

Out of scope, but disappointing that we just append 'ms' or 's' here. We need that unit's proposal to go through!

Part of the process of making this consistent:
image

}

/**
Expand All @@ -223,7 +222,7 @@ class Util {
*/
static formatSeconds(ms, granularity = 0.1) {
const coarseTime = Math.round(ms / 1000 / granularity) * granularity;
return `${coarseTime.toLocaleString(Util.numberDateLocale)}${NBSP}s`;
return `${Util.numberFormatter.format(coarseTime)}${NBSP}s`;
}

/**
Expand Down Expand Up @@ -556,10 +555,11 @@ class Util {
* @param {LH.Locale} locale
*/
static setNumberDateLocale(locale) {
Util.numberDateLocale = locale;

// When testing, use a locale with more exciting numeric formatting
if (Util.numberDateLocale === 'en-XA') Util.numberDateLocale = 'de';
if (locale === 'en-XA') locale = 'de';

Util.numberDateLocale = locale;
Util.numberFormatter = new Intl.NumberFormat(locale);
Copy link
Member Author

Choose a reason for hiding this comment

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

more static-ish state is unfortunate, but it seemed wasteful to do new Intl.NumberFormat(Util.numberDateLocale).format(coarseTime) for each format call...I'm not sure if it matters much

(tsc ensures it's initialized on the class, though, so that's a plus)

}

/**
Expand Down Expand Up @@ -618,6 +618,12 @@ class Util {
*/
Util.numberDateLocale = 'en';

/**
* This value stays in sync with Util.numberDateLocale.
* @type {Intl.NumberFormat}
*/
Util.numberFormatter = new Intl.NumberFormat(Util.numberDateLocale);

/**
* Report-renderer-specific strings.
* @type {LH.I18NRendererStrings}
Expand Down
6 changes: 6 additions & 0 deletions lighthouse-core/test/lib/i18n/swap-locale-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,12 @@ describe('swap-locale', () => {
// Renderer formatted strings
expect(lhrEn.i18n.rendererFormattedStrings.labDataTitle).toEqual('Lab Data');
expect(lhrEs.i18n.rendererFormattedStrings.labDataTitle).toEqual('Datos de prueba');

// Formatted numbers in placeholders.
expect(lhrEn.audits['render-blocking-resources'].displayValue)
.toEqual('Potential savings of 1,130 ms');
expect(lhrEs.audits['render-blocking-resources'].displayValue)
.toEqual('Potential savings of 1.130 ms');
Copy link
Member Author

Choose a reason for hiding this comment

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

this is actually formatted by intl-messageformat, but it's one more check that the number locales are being initialized correctly and it seemed like a missing piece of checking locale swapping

});

it('can roundtrip back to english correctly', () => {
Expand Down
29 changes: 24 additions & 5 deletions lighthouse-core/test/report/html/renderer/util-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ const assert = require('assert');
const Util = require('../../../../report/html/renderer/util.js');
const sampleResult = require('../../../results/sample_v2.json');

// Require i18n to make sure Intl is polyfilled in Node without full-icu.
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe worth noting that this code won't actually be run in node though for a real report but it's OK because browser should support it?

// eslint-disable-next-line no-unused-vars
const i18n = require('../../../../lib/i18n/i18n.js');

const NBSP = '\xa0';

/* eslint-env jest */
Expand Down Expand Up @@ -47,25 +51,40 @@ describe('util helpers', () => {
assert.equal(Util.formatDuration(28 * 60 * 60 * 1000 + 5000), `1${NBSP}d 4${NBSP}h 5${NBSP}s`);
});

// TODO: need ICU support in node on Travis/Appveyor
it.skip('formats based on locale', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

- it.skip

🎉

it('formats numbers based on locale', () => {
// Requires full-icu or Intl polyfill.
const number = 12346.858558;

const originalLocale = Util.numberDateLocale;
Util.setNumberDateLocale('de');
assert.strictEqual(Util.formatNumber(number), '12.346,9');
Util.setNumberDateLocale(originalLocale); // reset
assert.strictEqual(Util.formatBytesToKB(number), `12,1${NBSP}KB`);
assert.strictEqual(Util.formatMilliseconds(number), `12.350${NBSP}ms`);
assert.strictEqual(Util.formatSeconds(number), `12,3${NBSP}s`);

Util.setNumberDateLocale(originalLocale); // reset locale
assert.strictEqual(Util.formatNumber(number), '12,346.9');
assert.strictEqual(Util.formatBytesToKB(number), `12.1${NBSP}KB`);
assert.strictEqual(Util.formatMilliseconds(number), `12,350${NBSP}ms`);
assert.strictEqual(Util.formatSeconds(number), `12.3${NBSP}s`);
});

it.skip('uses decimal comma with en-XA test locale', () => {
it('uses decimal comma with en-XA test locale', () => {
// Requires full-icu or Intl polyfill.
const number = 12346.858558;

const originalLocale = Util.numberDateLocale;
Util.setNumberDateLocale('en-XA');
assert.strictEqual(Util.formatNumber(number), '12.346,9');
Util.setNumberDateLocale(originalLocale); // reset
assert.strictEqual(Util.formatBytesToKB(number), `12,1${NBSP}KB`);
assert.strictEqual(Util.formatMilliseconds(number), `12.350${NBSP}ms`);
assert.strictEqual(Util.formatSeconds(number), `12,3${NBSP}s`);

Util.setNumberDateLocale(originalLocale); // reset locale
assert.strictEqual(Util.formatNumber(number), '12,346.9');
assert.strictEqual(Util.formatBytesToKB(number), `12.1${NBSP}KB`);
assert.strictEqual(Util.formatMilliseconds(number), `12,350${NBSP}ms`);
assert.strictEqual(Util.formatSeconds(number), `12.3${NBSP}s`);
});

it('calculates a score ratings', () => {
Expand Down