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

Safari 14 Intl.NumberFormat usage throwing "Error: locale data nu must be an object" #886

Closed
ronenst opened this issue Oct 3, 2020 · 12 comments
Labels
library Relates to an Origami library

Comments

@ronenst
Copy link

ronenst commented Oct 3, 2020

Bug Report

What

Trying to initialize an Intl.NumberFormat object in Safari 14 with the latest version of the polyfill loaded throws an exception.

Details

Browser
Safari (Desktop) Version 14.0 (15610.1.28.1.9, 15610)

Minimal code example
https://jsbin.com/xikuwesiru/edit?html,console

<script src="https://polyfill.io/v3/polyfill.min.js?features=Intl.~locale.ja" crossorigin></script>

<script>
  const nf = new Intl.NumberFormat();
  console.log(nf.format(1e3));
</script>

Expected result
Console logs: 1,000
Screen Shot 2020-10-03 at 13 22 56

Actual result
Error: locale data nu must be an object is thrown.
Screen Shot 2020-10-03 at 13 23 31

Possible workaround for users of pollyfill.io service
Set the library to an earlier version without these polyfills: (i.e. https://polyfill.io/v3/polyfill.min.js?features=Intl.~locale.ja&version=3.89.4)

More info

@github-actions github-actions bot added the library Relates to an Origami library label Oct 3, 2020
@origamiserviceuser origamiserviceuser added this to incoming in Origami ✨ Oct 3, 2020
@Calvein
Copy link

Calvein commented Oct 7, 2020

I have the same issue in Safari 13 with RelativeTimeFormat and fixing the version to 3.89.4 (I tried every versions between 3.89.4 & 3.96.0) also fixed the issue.

@longlho
Copy link
Contributor

longlho commented Oct 8, 2020

The issue here is that you're polyfilling Intl with locale ja, but then initialize new Intl.NumberFormat with no locale, which defaults to en.

The Intl.NumberFormat polyfill is ES2020 which Safari 14 does not have. This explains why the polyfill was activated.

@ghost
Copy link

ghost commented Oct 8, 2020

Hi, I'm getting the same error with polyfills for 'no' locale. @longlho thanks for the workaround with v3.89.4.

I'm sure the reason is that in 3.89.4 polyfills used locale aliases. I loaded polyfills for nb locale and polyfilled API correctly used nb locale data for no locale.

For example in this polyfill https://cdn.polyfill.io/v3/polyfill.js?features=Intl.PluralRules.~locale.nb&version=3.89.4 you can find 'aliases' variable and it is used in resolveSupportedLocales function. However the latest polyfill (v3.96.0 now) doesn't use aliases https://cdn.polyfill.io/v3/polyfill.js?features=Intl.PluralRules.~locale.nb.

I wonder would it be possible to support custom locale data for locales that are not supported by polyfill service?

@longlho
Copy link
Contributor

longlho commented Oct 8, 2020

aliases are used to resolve legacy locales like zh-CN -> zh-Hans-CN so it's not related to this problem. The old behavior was that en data is shipped all the time for all locales as it's the fallback, which you can imagine isn't ideal, esp for big API like Intl.NumberFormat

@longlho
Copy link
Contributor

longlho commented Oct 8, 2020

To answer your question, technically it is possible to use custom locale data but it's an implementation detail of the polyfills and changes frequently as normative specs are changed. CLDR data is also preprocessed and packed to reduce size as well. At the end of the day there's __addLocaleData method in the polyfilled version but I highly discourage using it

@longlho
Copy link
Contributor

longlho commented Oct 10, 2020

@JakeChampion this is actually working as designed

@romainmenke
Copy link
Collaborator

romainmenke commented Oct 10, 2020

@longlho I still think this error message should be more helpful.
There is no way developers can deduce what action is needed when they get Error: locale data nu must be an object.

Is it possible to produce a message that states :

  • the error is throw by code from https://github.com/formatjs/formatjs
  • which polyfill (PluralRules/NumberFormat/...) has missing data
  • which locale has missing data

This would at least give developers a way to resolve this issue without coming here.

@ronenst
Copy link
Author

ronenst commented Oct 10, 2020

@longlho Thank you for responding and looking into this.

Am I right to assume your recommendation to avoid this issue would be to either always use the Intl APIs with a loaded locale or to always have ~locale.en loaded? Is en as the default guaranteed?

To clarify regarding Safari 14 not supporting Intl.NumberFormat, I sorry but I'm still a bit confused? Is the Safari 14 implementation perhaps non-standard or lacking in some way?

According to the previously linked kangax table and Can I use, it would seem Safari 14 fully supports this API.

It also seems to work without the polyfill when just running in the Safari 14 console:
Screen Shot 2020-10-10 at 14 50 23

While deciding this is behaving as intended is perfectly reasonable of course, I feel I should clarify why this originally seemed like a bug:

  • While it may seem that this change is just adding a new feature, it has actually subtly caused previously working straightforward code to break.
  • This behaviour is slightly surprising in the sense that code that works in unpolyfilled browsers may not work in polyfilled browsers. I was under the impression that normally the service will automatically load whatever dependencies it requires.
  • I would assume the idea of having a default is to have a safe fallback to ensure APIs always works if a locale is unspecified but the current behaviour breaks this principal. I skimmed over the specs and it seems to me the decision for a default locale can be anything, perhaps the library could decide to just use one of the locales that is already loaded?
  • While minimizing the size of the bundle is appreciated, personally when it comes to polyfills, I just consider it a tax the user has to pay for using an outdated browser.

I personally believe most users would prefer a simpler and safer configuration over a performance gain.

@longlho
Copy link
Contributor

longlho commented Oct 10, 2020

@romainmenke yes can you create a GH issue over at formatjs to fix the err message?

@ronenst there're several issues with just picking a default locale:

  1. It's not spec-compliant and also not deterministic. This is the same issue with DefaultTimezone as well in the spec. Browsers do not expose these info due to potential fingerprinting/privacy issue.
  2. Which one of the loaded locales to use? If it's the 1st one then the locale loading order has to be deterministic, which from usages we know that it's a common source of bug from users of formatjs.

So size is not the only issue but it is indeed a big issue. Locale data for en is pretty big (~20/30K but don't quote me on that).

At a high level by not specifying a locale you basically guarantee your product works correctly in ALL 200+ locales that browsers throw at you.

On top of that locale negotiation is not a straightforward lookup it's a hierarchy-based lookup. E.g if you ask for Polish and browsers don't have it it's at its own liberty to pick literally whatever it thinks work best (best-fit locale lookup). So it could be french or german or english.

My recommendation is to fix the callsite to always be explicit about picking a locale and make sure you test against that and have monitoring around it.

@ronenst
Copy link
Author

ronenst commented Oct 10, 2020

My recommendation is to fix the callsite to always be explicit about picking a locale and make sure you test against that and have monitoring around it.

Thanks, will look into that although it may not always be possible to control that, for example, when using a 3rd party library which calls this API.

@longlho
Copy link
Contributor

longlho commented Oct 10, 2020

If a 3rd party i18n library decides to pick a locale for you and don't allow you to override I recommend not using that :) personally I haven't seen it since i18n space is fairly complex so there're less options out there.

In terms of Safari 14 supporting Intl.NumberFormat. It does support it, but a es5 version and the polyfill is for es2020 version

longlho added a commit to longlho/polyfill-library that referenced this issue Oct 13, 2020
@JakeChampion
Copy link
Owner

@longlho and @romainmenke thank you for taking the time to handle this issue 👍

Origami ✨ automation moved this from incoming to complete Oct 26, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 25, 2021
@robertboulton robertboulton removed this from Done in Origami ✨ Jul 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
library Relates to an Origami library
Projects
None yet
Development

No branches or pull requests

5 participants