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: accept array of locales in lookupLocale #11349

Merged
merged 5 commits into from
Aug 31, 2020
Merged

Conversation

connorjclark
Copy link
Collaborator

@connorjclark connorjclark commented Aug 31, 2020

@connorjclark connorjclark requested a review from a team as a code owner August 31, 2020 19:16
@connorjclark connorjclark requested review from paulirish and removed request for a team August 31, 2020 19:16
* @param {string=} locale
* @return {LH.Locale}
* @return {LH.Locale|undefined}
*/
function lookupLocale(locale) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

paul suggested perhaps we should accept an array of locales here and do the "try the next value" in LH core.

@connorjclark connorjclark changed the title i18n: set default locale outside lookupLocale i18n: accept array of locales in lookupLocale Aug 31, 2020
Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

Fun fact: both Intl.getCanonicalLocales and lookupClosestLocale already take string|Array<string>|undefined :)

Can we use that to reduce work and custom resolution logic here?

it('falls back to root tag prefix if specific locale not available', () => {
expect(i18n.lookupLocale('en-JKJK')).toEqual('en');
expect(i18n.lookupLocale('es-JKJK')).toEqual('es');
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

test case was conflated with the default fallback behavior.

@connorjclark
Copy link
Collaborator Author

connorjclark commented Aug 31, 2020

Fun fact: both Intl.getCanonicalLocales and lookupClosestLocale already take string|Array<string>|undefined :)

Can we use that to reduce work and custom resolution logic here?

oooooh good catch. I tried to use this but it failed a test:

image

image


perhaps if lookupClosestLocale accepted string[]
image


edit: oh we just gave a too-narrow .d.ts. It works now!

@brendankenny
Copy link
Member

Wait, is there a bug in Intl.getCanonicalLocales? I don't think we polyfill that...

@brendankenny
Copy link
Member

perhaps if lookupClosestLocale accepted string[]

I think that's just our local d.ts declaration

@connorjclark connorjclark merged commit aac0b68 into master Aug 31, 2020
@connorjclark connorjclark deleted the i18n-default branch August 31, 2020 20:01
@brendankenny
Copy link
Member

@connorjclark do you mind updating the docs to mention the array behavior? To clear up how prefix shortening and array order interact in precedence, for instance.

gMakunde pushed a commit to gMakunde/lighthouse that referenced this pull request Sep 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants