formatjs / formatjs-old

The monorepo home to all of the FormatJS related libraries.
https://formatjs.io/
156 stars 53 forks source link

Unnecessary Intl.PluralRules check causing problems in some browsers #558

Closed HitkoDev closed 4 years ago

HitkoDev commented 4 years ago

Which package? intl-messageformat

Describe the bug https://github.com/formatjs/formatjs/blob/b262d6d6d7d4914a379ed70f8a369438b7ba5ee6/packages/intl-messageformat/src/formatters.ts#L244-L254

This part throws when there's no Intl.PluralRules implementation, even when formatters.getPluralRules uses a custom implementation which doesn't require Intl.PluralRules.

To Reproduce See description.

Expected behavior See description.

longlho commented 4 years ago

Can you clarify your use case?

On Fri, Mar 13, 2020 at 7:16 AM Hitko Development notifications@github.com wrote:

Which package? intl-messageformat

Describe the bug

https://github.com/formatjs/formatjs/blob/b262d6d6d7d4914a379ed70f8a369438b7ba5ee6/packages/intl-messageformat/src/formatters.ts#L244-L254

This part throws when there's no Intl.PluralRules implementation, even when formatters.getPluralRules uses a custom implementation which doesn't require Intl.PluralRules.

To Reproduce See description.

Expected behavior See description.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/formatjs/formatjs/issues/558, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABQM35C555EOTKEY2JCT5LRHIIYDANCNFSM4LHAKUJA .

HitkoDev commented 4 years ago

What's there to clarify? Maybe I just don't want a rather large polyfill when I already have a tiny function that does exactly what getPluralRules.select does?

Anyway, does it really matter? There's a documented public API that allows for custom formatters, and it shouldn't throw errors for something that's not needed, no matter what the use case may be.

longlho commented 4 years ago

That API is not public just because you found and export :)

On Fri, Mar 13, 2020 at 9:51 AM Hitko Development notifications@github.com wrote:

What's there to clarify? Maybe I just don't want a rather large polyfill when I already have a tiny function that does exactly what getPluralRules.select does?

Anyway, does it really matter? There's a documented public API that allows for custom formatters, and it shouldn't throw errors for something that's not needed, no matter what the use case may be.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/formatjs/formatjs/issues/558#issuecomment-598730003, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABQM36GIA6G42FXHF44YTTRHI26HANCNFSM4LHAKUJA .

HitkoDev commented 4 years ago

No, that API is public because it's listed as such in README: https://github.com/formatjs/formatjs/blob/master/packages/intl-messageformat/README.md#public-api

HitkoDev commented 4 years ago

Also there's no such check for Intl.NumberFormat and Intl.DateTimeFormat, and custom getNumberFormat / getDateTimeFormat implementations work without a problem, so there's no logical need for this check, other than to promote @formatjs/intl-pluralrules polyfill (as noted in README, number & date formatters use third-party polyfills 😉 ).

longlho commented 4 years ago

We don't check for those because we only support IE11+ as the oldest browser which has DateTimeFormat & NumberFormat. As the doc mentioned we require Intl.PluralRules to be there as runtime requirements and custom formatters override are only used for caching purposes.

HitkoDev commented 4 years ago

If that's the case, why even mention polyfills for date & numbers anywhere, and why not turn caching into a simple bool flag instead of making a full interface that allows overrides?

longlho commented 4 years ago

We don't dictate caching strategy per formatters since they have different implications in browser vs SSR. Doing that via a boolean also means exposing cache itself to flush/purge/add on demand for finer grain control which is even worse.