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

brendankenny · 2019-08-21T01:08:01Z

fixes part of #7238 (second bullet point)

tl;dr better Intl polyfill guard and switch our Util number formatters to use Intl methods. No change to ICU message formatters.

Our current polyfill relies only on browserify build ignoring intl, which means we're polyfilling even on node that has full-icu. But if we base it just on if Intl and (e.g.) Intl.NumberFormat are defined we won't get locales on system-icu or small-icu where those are defined but only (usually) a single locale is supported.

Instead, we can use the approach suggested by intl-locales-supported and check if all our needed locales are supported and polyfilling if not. If we take that literally, though, we'll still polyfill because for some reason (TODO:) 'no' and 'tl' aren't included in the latest Intl release in Chrome/Node.

Instead we just check that a representative sample of locales are supported and, if so, count that Intl as up-to-date enough. I adopted the same set of locales that intl-pluralrules uses but we could tweak to our own needs.

Finally, our Util.format*() methods are switched to use new Intl.NumberFormat().format() directly instead of indirectly through Number.toLocaleString(). This means that the Intl polyfill will be picked up in Node without full-icu and anywhere we were using those formatters (mostly in the renderer) will now be formatted. And we can re-enable some long-skipped tests.

brendankenny · 2019-08-21T01:08:57Z

lighthouse-core/lib/i18n/i18n.js


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


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

brendankenny · 2019-08-21T01:10:13Z

lighthouse-core/report/html/renderer/util.js

+    if (locale === 'en-XA') locale = 'de';
+
+    Util.numberDateLocale = locale;
+    Util.numberFormatter = new Intl.NumberFormat(locale);


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)

brendankenny · 2019-08-21T01:12:12Z

lighthouse-core/test/lib/i18n/swap-locale-test.js

+    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');


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

brendankenny · 2019-08-21T01:12:45Z

lighthouse-core/test/report/html/renderer/util-test.js

@@ -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', () => {


- it.skip

🎉

patrickhulce

pretty much LGTM

patrickhulce · 2019-08-21T15:34:09Z

lighthouse-core/lib/i18n/i18n.js

-  // 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.


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?

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.

lighthouse-core/lib/i18n/i18n.js

patrickhulce · 2019-08-21T15:38:25Z

lighthouse-core/test/report/html/renderer/util-test.js

@@ -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.


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?

exterkamp

LGTM. i18n might be getting a bit ridiculous because I'm even losing track now.

exterkamp · 2019-08-21T16:54:06Z

lighthouse-core/report/html/renderer/util.js

@@ -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`;


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:

brendankenny · 2019-08-21T18:50:51Z

updated.

Intl now will never be touched if the modules have been emptied by browserify, followed by a second check that a minimum set of locales is supported before polyfilling in node.

patrickhulce

LGTM!

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

51704af

brendankenny requested a review from paulirish as a code owner August 21, 2019 01:08

googlebot added the cla: yes label Aug 21, 2019

brendankenny commented Aug 21, 2019

View reviewed changes

jk spanish

e2e6c22

vercel bot deployed to staging August 21, 2019 01:15 View deployment

paulirish mentioned this pull request Aug 21, 2019

Internationalization of Lighthouse (Phase 2) #7238

Closed

36 tasks

patrickhulce reviewed Aug 21, 2019

View reviewed changes

exterkamp approved these changes Aug 21, 2019

View reviewed changes

be more conditional with your polyfills

406fa91

vercel bot deployed to staging August 21, 2019 18:38 View deployment

test comment

1d72986

vercel bot deployed to staging August 21, 2019 18:45 View deployment

patrickhulce approved these changes Aug 21, 2019

View reviewed changes

brendankenny merged commit 2ff07d2 into master Aug 21, 2019

brendankenny deleted the intlpolyfill branch August 21, 2019 19:32

paulirish pushed a commit that referenced this pull request Nov 6, 2019

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

556f5c0

snyk-bot mentioned this pull request Mar 21, 2020

[Snyk] Upgrade lighthouse from 5.1.0 to 5.6.0 godaddy/lighthouse4u#13

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

brendankenny commented Aug 21, 2019 •

edited

Loading

brendankenny Aug 21, 2019

brendankenny Aug 21, 2019

brendankenny Aug 21, 2019

brendankenny Aug 21, 2019

patrickhulce left a comment

patrickhulce Aug 21, 2019

brendankenny Aug 21, 2019

patrickhulce Aug 21, 2019

exterkamp left a comment

exterkamp Aug 21, 2019

brendankenny commented Aug 21, 2019

patrickhulce left a comment


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

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

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

Conversation

brendankenny commented Aug 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exterkamp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Aug 21, 2019

patrickhulce left a comment

Choose a reason for hiding this comment

brendankenny commented Aug 21, 2019 •

edited

Loading