Plural rule with injected argument not working as expected for Polish

[mledl]

Hi Ivan! 👋

Firstly, thanks for your work on this project! 🙂

Today I used patch-package to patch typesafe-i18n@5.24.3 for the project I'm working on.

We were experiencing issues with certain languages like Polish (pl) and Arabic (ar) when using the plural feature. When checking the source code of your library I noticed that you are using Intl.PluralRules for mapping the correct form.
We are using the plural (inject passed argument) rule: {{singular|?? plural}} and noticed that for the majority of languages pluralRules.select(value) returns other for some value that will trigger plural for the given locale. Somehow, Polish never returns other, but many in all those cases whicht leads to the given plural rule not working since the switch statement in your getPlural() method won't return the correct value.

We managed to patch it as shown below for the Svelte adapter. Maybe there is a better and more general solution. Do you know about this problem already? I couldn't find any related issue.
Another option would be to change the plural rules on our end to the most generic one, which we would not really fancy to do.

Cheers, Martin

Here is the diff that solved my problem:

diff --git a/node_modules/typesafe-i18n/svelte/index.cjs b/node_modules/typesafe-i18n/svelte/index.cjs
index 49d6c03..eaa5195 100644
--- a/node_modules/typesafe-i18n/svelte/index.cjs
+++ b/node_modules/typesafe-i18n/svelte/index.cjs
@@ -269,10 +269,10 @@ var parsePluralPart = (content, lastAccessor) => {
     return { k: key, r: zero };
   }
   if (nrOfEntries === 2) {
-    return { k: key, o: zero, r: one };
+    return { k: key, o: zero, m: one, r: one };
   }
   if (nrOfEntries === 3) {
-    return { k: key, z: zero, o: one, r: two };
+    return { k: key, z: zero, o: one, m: two, r: two };
   }
   return { k: key, z: zero, o: one, t: two, f: few, m: many, r: rest };
 };
diff --git a/node_modules/typesafe-i18n/svelte/index.mjs b/node_modules/typesafe-i18n/svelte/index.mjs
index 1c65b2d..f863327 100644
--- a/node_modules/typesafe-i18n/svelte/index.mjs
+++ b/node_modules/typesafe-i18n/svelte/index.mjs
@@ -39,10 +39,10 @@ var parsePluralPart = (content, lastAccessor) => {
     return { k: key, r: zero };
   }
   if (nrOfEntries === 2) {
-    return { k: key, o: zero, r: one };
+    return { k: key, o: zero, m: one, r: one };
   }
   if (nrOfEntries === 3) {
-    return { k: key, z: zero, o: one, r: two };
+    return { k: key, z: zero, o: one, m: two, r: two };
   }
   return { k: key, z: zero, o: one, t: two, f: few, m: many, r: rest };
 };

This issue body was partially generated by patch-package.

[ivanhofer]

HI Martin, thanks for the notice. Would you like to create a PR and add a few test cases for this?
Here is the place where you can change it: https://github.com/ivanhofer/typesafe-i18n/blob/main/packages/parser/src/basic.mts#L82-L87
This should probably also be mentioned in the docs somehow.

Thanks!

[mledl]

I was checking the CLDR specification while working on a good solution and noticed that the category other is stated to be required, see CLDR spec. Intl.Pluralrules is an implementation of this specification as of TC39.

Edit:
The most recent Language Plural Rules (v43) exactly describe the category mapping per locale. It is very interesting and impressive how complex pluralisation in various languages is.
From what I can obtain from this table. We will need to introduce the handling of many and maybe also backup the category few with some tests.

[ivanhofer]

Hi @mledl, thanks for checking the spec.
Yes, other is required, and typesafe-i18n also marks it as required: https://github.com/ivanhofer/typesafe-i18n/blob/main/packages/parser/src/basic.mts#L21

Maybe the easiest fix is to chane || '' to || part.r in here: https://github.com/ivanhofer/typesafe-i18n/blob/main/packages/runtime/src/core.mts#L187

I wonder if this breaks some existing tests. Please consider this too when you create the PR.

[ivanhofer]

@mledl have you had the chance to try the suggested change in your project already?

[mledl]

@mledl have you had the chance to try the suggested change in your project already?

We are using the patch I started this issue with and it works since beginning of May without bug report and we went live with it last week.
Since we can clean up stuff now, I will work on a PR.

[ivanhofer]

Thanks!

[mledl]

Hi @mledl, thanks for checking the spec. Yes, other is required, and typesafe-i18n also marks it as required: https://github.com/ivanhofer/typesafe-i18n/blob/main/packages/parser/src/basic.mts#L21

Maybe the easiest fix is to chane || '' to || part.r in here: https://github.com/ivanhofer/typesafe-i18n/blob/main/packages/runtime/src/core.mts#L187

I wonder if this breaks some existing tests. Please consider this too when you create the PR.

I have been checking and verifying your proposed solution and noticed that replacing || '' with || part.r in https://github.com/ivanhofer/typesafe-i18n/blob/main/packages/runtime/src/core.mts#L187 will break handling of singular values.
I see two other solutions here:

1. The one proposed when opening this issue (which is the patch we have been running currently). Meaning we need to adapt the returns around https://github.com/ivanhofer/typesafe-i18n/blob/main/packages/runtime/src/basic.mts#L80.
2. We need to have the cases few and many in getPlural fallback to r in case the corresponding values are undefined. Meaning we would replace https://github.com/ivanhofer/typesafe-i18n/blob/main/packages/runtime/src/core.mts#L160 and https://github.com/ivanhofer/typesafe-i18n/blob/main/packages/runtime/src/core.mts#L162 with return f || r and return m || r correspondingly.

What do you think?

[ivanhofer]

Right, then I think solution 2 would be the best.

[ivanhofer]

Was fixed in 5.26.0