deps: remove intl polyfill #12426

brendankenny · 2021-04-29T22:14:07Z

Remove intl in Lighthouse's next major release

No effect on bundled clients, only CLI and node usage
Node 13+ is now built with full-icu by default
The intl polyfill is archived and was last updated almost five years ago. Already out of date on some languages and several Intl features
the primary reason someone would now use e.g. small-icu is to save disk space (e.g. for use in a docker image), which we immediately negate by including a 25 MiB polyfill :)

I suggest we lose the dependency:

for almost everyone on Node 13+ there will be no change in behavior
for anyone on Node 13+ built with small-icu, we should honor the choice of keeping a slim build and just support en as advertised. We can log that we're doing so.
for anyone somehow stuck on Node 12 and a default small-icu build (if we still want to support Node 12 in LH8), we can advise the use of the official full-icu package, which downloads the latest ICU files and gives instructions on how to get Node to use them. full-icu is also about half the size of the intl polyfill :)

More Node ICU background: https://nodejs.org/api/intl.html

brendankenny · 2021-04-29T22:16:41Z

.github/workflows/ci.yml

@@ -87,10 +87,10 @@ jobs:
        # Depth of at least 2 for codecov coverage diffs. See https://github.com/GoogleChrome/lighthouse/pull/12079
        fetch-depth: 2

-    - name: Use Node.js 12.x


just updating to demonstrate that everything works as expected in the current node LTS (14). Otherwise, there's a few lookupLocale tests that fail in node 12 because they all return 'en'.

If we maintain Node 12 support, we could e.g. unit test on Node 12 and node 14, and assert either the correct locale is returned or we're in a small-icu build of node and a warning was logged.

brendankenny · 2021-05-18T22:02:46Z

This is ready for review. In addition to the log messages linking to the full-icu explanation when it looks like you're running a small-icu build of node, also added a readme faq entry.

brendankenny · 2021-05-19T00:46:55Z

this should wait on #12513 because we should have a test for Node 12 behavior (warning that it's running with small-icu and what to do about it) and a few of our existing localization tests will only work as currently expected in Node 13+ with full-icu available

connorjclark

lgtm

connorjclark · 2021-05-25T19:22:54Z

readme.md

@@ -26,6 +26,7 @@
  * [Can I configure the lighthouse run?](#can-i-configure-the-lighthouse-run)
  * [How does Lighthouse use network throttling, and how can I make it better?](#how-does-lighthouse-use-network-throttling-and-how-can-i-make-it-better)
  * [Are results sent to a remote server?](#are-results-sent-to-a-remote-server)
+  * [How do I get localized Lighthouse results?](#how-do-i-get-localized-lighthouse-results)


bad link

haha, how on earth did you notice that :)

thanks!

paulirish

a few suggestions…

the plan is to have a release with these warnings (and the 'en' fallback) and next major will remove node 12 support and we'll also then remove all the ifNode12() checks in tests, yes?

paulirish · 2021-05-25T21:50:36Z

lighthouse-core/lib/i18n/i18n.js

+  if (!closestLocale) {
+    // Log extra info if we're pretty sure this version of Node was built with `--with-intl=small-icu`.
+    if (Intl.NumberFormat.supportedLocalesOf('es').length === 0) {
+      log.warn('i18n', 'Requested locale not available in this version of node. The `full-icu` npm module can provide additional locales. See https://nodejs.org/api/intl.html#intl_providing_icu_data_at_runtime for help.');


let's link to https://github.com/GoogleChrome/lighthouse/blob/master/readme.md#how-do-i-get-localized-lighthouse-results-via-the-cli instead?

This is probably the first error message that node 12 users will see so we'll route them to our FAQ first.

paulirish · 2021-05-25T21:54:20Z

readme.md

@@ -442,6 +443,10 @@ Read more in our [guide to network throttling](./docs/throttling.md).
 Nope. Lighthouse runs locally, auditing a page using a local version of the Chrome browser installed on the
 machine. Report results are never processed or beaconed to a remote server.

+### How do I get localized Lighthouse results via the CLI?
+
+Starting in Lighthouse 8.0, the `Intl` API is no longer polyfilled. If you're using Node 13 or later there should be no issue because Node is now [built with `full-icu` by default](https://nodejs.medium.com/node-js-12-to-lts-and-node-js-13-is-here-e28d6a4a2bd#9514), but if you're somehow stuck with `small-icu` and see Lighthouse log messages about your locale not being available, you can manually install ICU data by using the [`full-icu`](https://www.npmjs.com/package/full-icu) module and the [`--icu-data-dir` node flag](https://nodejs.org/api/intl.html#intl_providing_icu_data_at_runtime) at launch.


Suggested change

Starting in Lighthouse 8.0, the `Intl` API is no longer polyfilled. If you're using Node 13 or later there should be no issue because Node is now [built with `full-icu` by default](https://nodejs.medium.com/node-js-12-to-lts-and-node-js-13-is-here-e28d6a4a2bd#9514), but if you're somehow stuck with `small-icu` and see Lighthouse log messages about your locale not being available, you can manually install ICU data by using the [`full-icu`](https://www.npmjs.com/package/full-icu) module and the [`--icu-data-dir` node flag](https://nodejs.org/api/intl.html#intl_providing_icu_data_at_runtime) at launch.

Starting in Lighthouse 8.0, Lighthouse relies entirely on native `Intl` support and no longer uses an `Intl` polyfill. If you're using Node 13 or later, there should be no issue because Node is now [built with `full-icu` by default](https://nodejs.medium.com/node-js-12-to-lts-and-node-js-13-is-here-e28d6a4a2bd#9514).

However, if you're using Node 12 (when `small-icu` was the default) or another `small-icu` situation, you may see Lighthouse log messages about your locale not being available. To remedy this, you can upgrade to Node 14+ or manually install ICU data by using the [`full-icu`](https://www.npmjs.com/package/full-icu) module and the [`--icu-data-dir` node flag](https://nodejs.org/api/intl.html#intl_providing_icu_data_at_runtime) at launch.

brendankenny · 2021-05-25T23:22:05Z

the plan is to have a release with these warnings (and the 'en' fallback) and next major will remove node 12 support and we'll also then remove all the ifNode12() checks in tests, yes?

yes indeed

brendankenny requested a review from paulirish April 29, 2021 22:14

brendankenny requested a review from a team as a code owner April 29, 2021 22:14

google-cla bot added the cla: yes label Apr 29, 2021

brendankenny added breaking and removed cla: yes labels Apr 29, 2021

google-cla bot added the cla: yes label Apr 29, 2021

brendankenny added 8.0 i18n internationalization thangs and removed cla: yes labels Apr 29, 2021

google-cla bot added the cla: yes label Apr 29, 2021

devtools-bot assigned paulirish Apr 29, 2021

devtools-bot added the waiting4reviewer label Apr 29, 2021

brendankenny commented Apr 29, 2021

View reviewed changes

brendankenny mentioned this pull request Apr 29, 2021

☂️ 👣 LH v8 burndown #11866

Closed

17 tasks

mspaansen approved these changes May 2, 2021

View reviewed changes

GoogleChrome deleted a comment from mspaansen May 4, 2021

brendankenny force-pushed the rmintl branch from 64d85b8 to 40295b0 Compare May 18, 2021 22:01

vercel bot deployed to Preview May 18, 2021 22:01 View deployment

brendankenny changed the title ~~proposal: deps: remove intl polyfill~~ deps: remove intl polyfill May 18, 2021

brendankenny mentioned this pull request May 19, 2021

tests(ci): run unit tests on all active node versions #12513

Merged

brendankenny force-pushed the rmintl branch from 40295b0 to 227ab05 Compare May 20, 2021 23:09

vercel bot deployed to Preview May 20, 2021 23:09 View deployment

connorjclark reviewed May 25, 2021

View reviewed changes

brendankenny added 3 commits May 25, 2021 15:08

deps: remove intl polyfill

d3e1959

add full-icu faq entry to readme

34479f4

add node 12 vs node 13+ split in locale unit tests

8c0a254

fix readme link

c6fd7bc

brendankenny force-pushed the rmintl branch from 227ab05 to c6fd7bc Compare May 25, 2021 20:10

vercel bot deployed to Preview May 25, 2021 20:10 View deployment

paulirish approved these changes May 25, 2021

View reviewed changes

feedback

feef8e0

vercel bot deployed to Preview May 25, 2021 23:22 View deployment

brendankenny added the land-when-ci-is-green label May 25, 2021

devtools-bot merged commit 2511725 into master May 25, 2021

devtools-bot deleted the rmintl branch May 25, 2021 23:36

devtools-bot removed the land-when-ci-is-green label May 25, 2021

brendankenny mentioned this pull request Jun 24, 2021

tests: add explicit small-icu detection for i18n #12696

Merged

brendankenny mentioned this pull request Oct 21, 2021

deps: move to minimum Node 14 #13243

Merged

ryan-ally mentioned this pull request Nov 30, 2023

[Snyk] Security upgrade lighthouse from 7.5.0 to 8.0.0 ryan-ally/lighthouse#48

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deps: remove intl polyfill #12426

deps: remove intl polyfill #12426

brendankenny commented Apr 29, 2021

brendankenny Apr 29, 2021

brendankenny commented May 18, 2021

brendankenny commented May 19, 2021

connorjclark left a comment

connorjclark May 25, 2021

brendankenny May 25, 2021

paulirish left a comment

paulirish May 25, 2021

paulirish May 25, 2021

brendankenny commented May 25, 2021

-Starting in Lighthouse 8.0, the `Intl` API is no longer polyfilled. If you're using Node 13 or later there should be no issue because Node is now [built with `full-icu` by default](https://nodejs.medium.com/node-js-12-to-lts-and-node-js-13-is-here-e28d6a4a2bd#9514), but if you're somehow stuck with `small-icu` and see Lighthouse log messages about your locale not being available, you can manually install ICU data by using the [`full-icu`](https://www.npmjs.com/package/full-icu) module and the [`--icu-data-dir` node flag](https://nodejs.org/api/intl.html#intl_providing_icu_data_at_runtime) at launch.
+Starting in Lighthouse 8.0, Lighthouse relies entirely on native `Intl` support and no longer uses an `Intl` polyfill. If you're using Node 13 or later, there should be no issue because Node is now [built with `full-icu` by default](https://nodejs.medium.com/node-js-12-to-lts-and-node-js-13-is-here-e28d6a4a2bd#9514).
+However, if you're using Node 12 (when `small-icu` was the default) or another `small-icu` situation, you may see Lighthouse log messages about your locale not being available. To remedy this, you can upgrade to Node 14+ or manually install ICU data by using the [`full-icu`](https://www.npmjs.com/package/full-icu) module and the [`--icu-data-dir` node flag](https://nodejs.org/api/intl.html#intl_providing_icu_data_at_runtime) at launch.

deps: remove intl polyfill #12426

deps: remove intl polyfill #12426

Conversation

brendankenny commented Apr 29, 2021

brendankenny Apr 29, 2021

Choose a reason for hiding this comment

brendankenny commented May 18, 2021

brendankenny commented May 19, 2021

connorjclark left a comment

Choose a reason for hiding this comment

connorjclark May 25, 2021

Choose a reason for hiding this comment

brendankenny May 25, 2021

Choose a reason for hiding this comment

paulirish left a comment

Choose a reason for hiding this comment

paulirish May 25, 2021

Choose a reason for hiding this comment

paulirish May 25, 2021

Choose a reason for hiding this comment

brendankenny commented May 25, 2021