Fall back to 'other' if a plural form is missing

[36degrees]

If a service currently has a character count inside an element with a non-English lang tag and tried to use the current implementation, the Character Count may throw an error and not update the count message when the user has a certain number of characters remaining.

This is because we are only providing default translations for the one and other plural forms (the only forms required in English).

But if the character count was in a cy-GB locale for example it would expect plural forms for two, few and many, and when the user got down to 6-2 characters remaining an error would be thrown and the count message would not be updated.

In order to preserve the existing behaviour, falling back to other is a reasonable thing to do, even if it may lead to incorrect plural forms being used in some cases.

Closes #2918.

[36degrees]

I am not 100% sure about the warning – I think there are cases where users could omit plural forms and the translations would be fine.

Just because a language might use a different plural form, doesn't mean it will.

To give a slightly silly example, this would log a warning, but the localisation is perfectly correct:

const i18n = new I18n({
    'test.other': 'Ollie has %{count} sheep'
}

i18n.t('test', { count: 1 }) // Ollie has 1 sheep
i18n.t('test', { count: 2 }) // Ollie has 2 sheep

To silence the warning, they'd need to pass test.one even though it works exactly the same as other. Should we be trying to enforce passing all plural forms in code when they might not actually be needed?

[querkmachine]

The message formatting seems a bit inconsistent as it refers to the missing key as ".many" but then says it's falling back to the `other` key. I'd expect the missing key to probably also be written as `many`.

Otherwise this all looks good to me. Seems to do the job.

[36degrees]

Good catch! 👀

[querkmachine]

Should we be trying to enforce passing all plural forms in code when they might not actually be needed?

I'm gonna say yes, if just for consistency and error checking's sakes. In that example, it's more coincidental that the one and other forms of 'sheep' are the same than by the design of the language.

A reverse argument might be that we shouldn't require other in all situations because Chinese, Malaysian, Indonesian, etc. don't have plural forms at all and all nouns are singular. Russian is similarly weird, with the other form only used for decimal numbers, with all whole numbers using one of the other plural forms.

At least by pushing a warning out for a missing form, we can assist developers (who may not be speakers of the language at hand) in knowing if a required string is likely to be missing or malformed.

[colinrotherham]

Cheers @36degrees looks good

Sheep example aside (deer, fish, moose… Emoji) it's all working. Perhaps good to consider the page weight increase having to duplicate lots of text for these cases?

Added some nice-to-have (but not blocking) comments

[colinrotherham]

Worth trying to share these messages between the new Error() and console.warn()?

var pluralSuffixError = 'i18n: Plural form ".' + pluralSuffix + '" is required for "' + this.locale + '" locale'
var pluralSuffixWarn = pluralSuffixError + '. Falling back to `other`.'

console.warn(pluralSuffixWarn)

throw new Error(pluralSuffixError)

[36degrees]

I've opted to leave this as is for now as it didn't feel quite right to describe the missing plural forms as required when there's something we can fall back to.

(I think this ties in slightly to Beep's comment about future behaviour and what we want to do.)

[colinrotherham]

I'm seeing .many, .few and .two logged (not just .one)

Shall we do one of those fancy it.each([]) and make sure each plural form has a test?

Looks good

[colinrotherham]

This came up on Slack, so bringing over the points here

1. Do we suppress .two, .few, .many warnings, for all locales?
2. Do we suppress .two, .few, .many warnings, for English only?
3. Treating all locales equally, is it correct to need duplicate strings by design?

var TRANSLATIONS_DEFAULT = {
  // Characters
  charactersUnderLimit: {
    one: 'You have %{count} character remaining',
    two: 'You have %{count} characters remaining',
    few: 'You have %{count} characters remaining',
    many: 'You have %{count} characters remaining',
    other: 'You have %{count} characters remaining'
  },
  charactersAtLimit: 'You have 0 characters remaining',
  charactersOverLimit: {
    one: 'You have %{count} character too many',
    two: 'You have %{count} characters too many',
    few: 'You have %{count} characters too many',
    many: 'You have %{count} characters too many',
    other: 'You have %{count} characters too many'
  },
  // Words
  wordsUnderLimit: {
    one: 'You have %{count} word remaining',
    two: 'You have %{count} words remaining',
    few: 'You have %{count} words remaining',
    many: 'You have %{count} words remaining',
    other: 'You have %{count} words remaining'
  },
  wordsAtLimit: 'You have 0 words remaining',
  wordsOverLimit: {
    one: 'You have %{count} word too many',
    two: 'You have %{count} words too many',
    few: 'You have %{count} words too many',
    many: 'You have %{count} words too many',
    other: 'You have %{count} words too many'
  }
}

To silence the warning, they'd need to pass test.one even though it works exactly the same as other. Should we be trying to enforce passing all plural forms in code when they might not actually be needed?

👆 Above would be the Character Count defaults if we needed all strings

[36degrees]

Different plural forms are required depending on which locale the Character Count is in.

When in the default 'en' locale, only 'one' and 'other' are required.

So, I think the answers are:

1. We should warn when a plural form is missing that we try to use based on the locale, so 'it depends on the locale'
2. There shouldn't be any cases where we warn for a character count in the en locale, because we have all of the plural forms we need to localise in English ('one' and 'other')
3. I think that's what I'm asking with my question about the sheep. But I don't think we'd add plural forms for anything other than 'one' and 'other' to the defaults as the defaults are only designed to be used in an en locale – we'd expect if the locale is something other than en that we'd have a different set of translations passed with the correct plural forms for the locale. This PR is trying to address the edge case where that doesn't happen.

[colinrotherham]

@36degrees Perfect, thanks for the answers

Shall we do one of those fancy it.each([]) and make sure each plural form has a test?

Do you want to add tests to confirm?

1. English: Warnings do not get logged for missing .two, .few, .many
2. Others: Warnings do get logged for missing .two, .few, .many

We've got this one already, but won't harm to document this decision about the others in test form

expect(consoleWarn).toHaveBeenCalledWith(
  'i18n: Missing plural form ".one" for "en" locale. Falling back to ".other".'
)

[36degrees]

I'm wary of testing for specific locales as it feels like we're starting to test the implementation of Intl.PluralRules.select rather than our code.

The reason that we shouldn't output any warnings for the en locale is because of how the class interacts with the config for the character count – there is nothing happening in the I18n class itself that means we treat the en locale any differently to any other locale.

I can add comments to the tests to try and make it clearer how the config we're passing in tests specific scenarios, if that'd help?

[colinrotherham]

Yeah, makes sense

I like to think if a less experienced developer touches these lines in future, and they break something, the tests should be there to explain exactly why the change broke something—gives them a safety net.

Also, if we fix a Welsh bug, add a Welsh test case 😆

If it feels better in the Puppeteer tests, further away from I18n, can do that?

[36degrees]

I've added a failing test to the Character Count component in c757025.

[querkmachine]

As a stray thought (sorry!), in my mind, the functionality to fall back to other is only present to avoid a breaking change in existing services where they upgrade Frontend, and have a situation where the page is set in one lang but the component has not been translated yet.

However, falling back to other is a 'best guess' assumption and is not reliably true for all languages, nor is this something we actually want service developers to use or rely on as an intended feature. (Ideally, all plural forms needed should be explicitly set.)

Would it be a good idea to have a follow up card to remove the fallback in a future breaking release?