i18n: add better support for the default locale in bundles #13211
Conversation
brendankenny
requested review from
connorjclark
and removed request for
a team
| @@ -283,6 +283,7 @@ function resolveSettings(settingsJson = {}, overrides = undefined) { | |||
| // If a locale is requested in flags or settings, use it. A typical CLI run will not have one, | |||
| // however `lookupLocale` will always determine which of our supported locales to use (falling | |||
| // back if necessary). | |||
| // TODO: could do more work to sniff out the user's locale | |||
There was a problem hiding this comment.
longtime TODO in i18n.js but seemed better here where the locale is actually chosen
| * is to replace the default export of this file with `{}` so that no locale messages | ||
| * are included. Strings will work normally through the IcuMessage.formattedDefault | ||
| * fallback, and locale messages can be added on demand (e.g. dynamically fetched) | ||
| * through `format.registerLocaleData()`. |
There was a problem hiding this comment.
not sure who will ever look in here, but worth documenting somewhere :)
#9520 (comment) is a previous discussion about it. the linked blog post sez:
That does line up with the plan here. Anyway, for the moment, english inheritance is kinda broken in CLDR. so.. there's no "right" answer for our default being either 'en' or 'en-US'. So it's fine going with whatever's more straightforward. :) |
|
Ok, the devtools error is real, so I'm going to back out the |
brendankenny
force-pushed
the
default-locale
branch
from
d76efaf
to
aa63612
Compare
Recently we've been circling around a few different ways to strip the locale data out of bundles, each slightly different, so it seemed like a good time to settle on one (this also makes the brfs replacement easier).
This PR sets us up so we can always replace
locales.jswith{}. Everything works without reaching too deep and/or custom shimming. There are no messages available for localization unless the client using the bundle brings them, either viaregisterLocaleDatawhen running lighthouse-core or viarendererFormattedStringsequivalents in the report. Viewer locale swapping already works like this and it's working well :)changes in format.js/i18n.js:
{}for our locales because the default fallbacks (provided by each file'sUIStrings) are always available, however there are a few places whereLOCALE_MESSAGES[requestedLocale]is checked for truthiness, which would fail with this approach. Added a new condition where if the default locale is being requested, the caller now gets a return value that allows the default fallback behavior to happen.format.jsnow defines theDEFAULT_LOCALE, which is nowen-US, noten. I don't remember the discussion with how we landed onenfor the theoretical default, but we actually default toen-USso I'm treating that as the thing to use for now. @paulirish if you remember how we were supposed to resolve thischanges in build/build-bundle.js
DevTools currently uses a slightly different transform, where each locale in locales.js is replaced with an empty object (nevermind for the moment: #13211 (comment){'ar': {}, 'ar-XB': {}, 'bg': {}, ...}). To verify the new approach, we go back to the approach before #12921 and shim justlocale.jswith a{}.