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

Fix plural issue with lang-region locale #257

Merged

Conversation

xu3u4
Copy link
Contributor

@xu3u4 xu3u4 commented Nov 8, 2023

To fix #256

For {language}-{region} format locale, the plural count key can't be generated correctly.
The locale from the result of pluralRule.resolvedOptions(); may only contain the language part, without the region.
Although the value of locale should be BCP 47 language tag, but node(only tested on v18.17) and some browsers don't support it yet. So I think we can also compare the language part of the locale.

On node v18.17

const pluralRules = new Intl.PluralRules('en-US');
const options = pluralRules1.resolvedOptions();

console.log(options);
// output: Object { locale: "en", type: "cardinal",....}

@xu3u4 xu3u4 force-pushed the fix-plural-issue-with-lang-region-locale branch 2 times, most recently from 33a8540 to a6ae378 Compare November 8, 2023 08:47
@xu3u4 xu3u4 changed the title Stripe out region part from locale Fix plural issue with lang-region locale Nov 8, 2023
@xu3u4
Copy link
Contributor Author

xu3u4 commented Nov 8, 2023

Hi @gilbsgilbs
May I have your review when you have time 🙏

@gilbsgilbs
Copy link
Owner

Thanks for the PR, I'll look into it ASAP. In the meantime, could you add a test for your fix?

@xu3u4
Copy link
Contributor Author

xu3u4 commented Nov 8, 2023

Thank you for your suggestion 🙇‍♀️ , just added!

@gilbsgilbs gilbsgilbs force-pushed the fix-plural-issue-with-lang-region-locale branch from 0c5dfbc to b19d316 Compare November 9, 2023 07:53
When resolving a PluralRule options, node only matches the language part
of the BCP 47 locale string. Therefore, we need to be a bit more
permissive when we validate the options returned by Node.
@gilbsgilbs gilbsgilbs force-pushed the fix-plural-issue-with-lang-region-locale branch from b19d316 to 1274800 Compare November 9, 2023 07:55
@gilbsgilbs gilbsgilbs merged commit adbd6ee into gilbsgilbs:master Nov 9, 2023
@xu3u4
Copy link
Contributor Author

xu3u4 commented Nov 9, 2023

@gilbsgilbs
I saw your change that using startWith looks good!
Thank you very much.

@gilbsgilbs
Copy link
Owner

I'll make a release later this week, probably tonight or tomorrow. I want to do some chore before releasing (mainly update dependencies, drop EOLed node versions and officially support newer node versions).

@gilbsgilbs
Copy link
Owner

Just published it under the 0.9.1 tag on NPM. Thank you again for your contribution, that's appreciated.

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.

Adding count to t options throws error at computeDerivedKeys and causes webpack build to fail
2 participants