Skip to content

Add pluralization support to JS i18n implementation#5950

Merged
aduth merged 2 commits intomainfrom
aduth-i18n-js-count
Feb 15, 2022
Merged

Add pluralization support to JS i18n implementation#5950
aduth merged 2 commits intomainfrom
aduth-i18n-js-count

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Feb 14, 2022

Follow-up to: #5913 (comment)

Why: So that a developer can expect that the JavaScript i18n implementation behaves reasonably similar to the Ruby/Rails behavior.

Related documentation: https://guides.rubyonrails.org/i18n.html#pluralization

**Why**: So that a developer can expect that the JavaScript i18n implementation behaves reasonably similar to the Ruby/Rails behavior.
@aduth aduth requested a review from SammySteiner February 14, 2022 20:44
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

* @return Entry string.
*/
const getString = (entry: Entry, count?: number): string =>
typeof entry === 'object' ? entry[getPluralizationKey(count)] : entry;
Copy link
Contributor

Choose a reason for hiding this comment

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

should we also make sure count in not undefined? Otherwise the combo of a too-short key and a missing count
could result in the wrong value?

Suggested change
typeof entry === 'object' ? entry[getPluralizationKey(count)] : entry;
typeof entry === 'object' && count !== undefined ? entry[getPluralizationKey(count)] : entry;

Copy link
Contributor

Choose a reason for hiding this comment

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

If count is an optional param that is only supplied if we need to call getPluralizationKey, why not simplify to
count != undefined ? entry[getPluralizationKey(count)] : entry;

Copy link
Contributor Author

@aduth aduth Feb 14, 2022

Choose a reason for hiding this comment

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

It's a bit of an edge case, one that I'd sort of allowed for, and that TypeScript is keeping us honest about (count is an optional parameter of getPluralizationKey).

What would be the correct value for an undefined count? To me, that's a bit of an unsupported usage. I might even go so far to say we should throw a TypeError if the entry is pluralized but not given a count.

The fallback entry behavior is a bit of an odd relic, imo. I think I implemented it that way purely for the tests to be able to test the presence of the key in rendered content. Edit: I was getting confused here for the fallback behavior of getEntry, not getString.

Copy link
Contributor Author

@aduth aduth Feb 14, 2022

Choose a reason for hiding this comment

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

To this and @SammySteiner 's point below, I suppose there's a strong connection between count and PluralizedEntry. It'd be nice if we could define it in a way that TypeScript would enforce it on the consumer end, though I dunno if that'd be possible.

Maybe throwing is the way to go?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was imagining like a case where we have strings like this:

some: {
  value: {
    a: "the string"
  }
}

and if we call t('some.value') in Ruby i18n we get the whole hash: { a: "the string" } so my worry here was that if we did that, the JS code here would be like "ah yes, an object let me look up the count"

A TypeError seems appropriate to me, it also depends how much we want to match Ruby i18n. I think that returning a whole object is rarely desirable behavior so we could just TypeError on that until we find ourselves needing that.

Copy link
Contributor Author

@aduth aduth Feb 14, 2022

Choose a reason for hiding this comment

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

and if we call t('some.value') in Ruby i18n we get the whole hash: { a: "the string" } so my worry here was that if we did that, the JS code here would be like "ah yes, an object let me look up the count"

A TypeError seems appropriate to me, it also depends how much we want to match Ruby i18n. I think that returning a whole object is rarely desirable behavior so we could just TypeError on that until we find ourselves needing that.

That's a good point. And while I agree it's not really a common use-case, if it's easy enough to match the behavior, I'll try to do that, since part of the goal of this work is developer ergonomics switching back-and-forth.

Copy link
Contributor Author

@aduth aduth Feb 15, 2022

Choose a reason for hiding this comment

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

I went down the route of trying to match the Ruby behavior, but ultimately I think it would be more hassle than it's worth for how it would impact usage, since it would change the return value of t from string to Entry (i.e. a string or an object), which would make usage more cumbersome when 99%+ of use-cases would be to use the t result as a string.

Instead, I went with the idea to throw an error when missing a count for an object entry in f3fbfc1 .

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, I think that the Ruby behavior of "just return the hash" can get super confusing, I think the error is a better fit here 💯

Copy link
Contributor

@SammySteiner SammySteiner left a comment

Choose a reason for hiding this comment

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

Nice consolidation! Will definitely make using this easier in the future.

* @return Entry string.
*/
const getString = (entry: Entry, count?: number): string =>
typeof entry === 'object' ? entry[getPluralizationKey(count)] : entry;
Copy link
Contributor

Choose a reason for hiding this comment

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

If count is an optional param that is only supplied if we need to call getPluralizationKey, why not simplify to
count != undefined ? entry[getPluralizationKey(count)] : entry;

@aduth aduth merged commit 838b11d into main Feb 15, 2022
@aduth aduth deleted the aduth-i18n-js-count branch February 15, 2022 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants