formatjs / formatjs-old

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

IntlMessageFormat class breaks the conditional polyfilling #587

Closed mortargrind closed 4 years ago

mortargrind commented 4 years ago

Which package? intl-messageformat

Describe the bug IntlMessageFormat.defaultLocale is being calculated when the library first loaded. In the application code, I first do my custom polyfill check, load necessary Intl polyfills and then we initiate the app itself. Since static IntlMessageFormat.defaultLocale property initialization happens immediately during class declaration it will throw an error if the browser does not support Intl API.

To Reproduce Steps to reproduce the behavior:

  1. Clone this repo: https://github.com/mortargrind/intl-formatmessage-polyfilling/
  2. Start the app: npm start / yarn start
  3. Go to localhost and observe the error message

Expected behavior All Intl referencing should be lazy to enable conditional polyfilling in the consumer code.

Additional context You can use Chrome as a browser that does not support Intl since I have added a snippet like delete window.Intl; in the index.html.

I think easiest fix for now: Instead of locales: string | string[] = IntlMessageFormat.defaultLocale it should be locales: string | string[] = new Intl.NumberFormat().resolvedOptions().locale so it should be calculated when the class constructor is called.

longlho commented 4 years ago

I think I'm confused, if u're loading the necessary Intl polyfills before initiating the app, shouldn't it work. IntlMessageFormat should be part of the app chunk bc it's not in the spec (so technically not a polyfill)

mortargrind commented 4 years ago

I am loading the pollyfills before initializing the app as you can see the in the example repo (See: https://github.com/mortargrind/intl-formatmessage-polyfilling/blob/6849cbd6cce43e4f3b51409a82e4bf2a2adbf098/src/index.js#L20).

But the intl-messageformat lib is referencing the Intl API during initial load, not when the IntlMessageFormat class is actually being called. So before me deciding which polyfill is needed and loading them before initializing the app, it throws an error since there's no window.Intl yet.

I am following the https://github.com/formatjs/react-intl/blob/master/docs/Getting-Started.md#runtime-requirements guide and this approach works for intl-messageformat version 7.3.2. But not working for later versions since default value for the locales argument is updated to be calculated before actually calling the IntlMessageFormat constructor (because it is a static variable that is referencing the window.Intl).

This forces your consumers to check for the polyfills before loading anything else (not before calling anything else). While in theory you can do optional polyfilling before your application code loads at all, in practice it's a bit hard and unnecessary to configure your bundler (Webpack or something else) to do it. For example; suggested way to polyfill Intl (or anything else that might require a polyfill) for a create-react-app project is this: https://github.com/facebook/create-react-app/issues/1943 (exactly same as what I am doing) but it does not work if you are using intl-messageformat right now.

longlho commented 4 years ago

Polyfilling before importing application-related code is the correct solution here which means moving all your app imports to be after polyfills. There are services like polyfill.io which are exactly for this purpose.

Polyfills are meant to be in the boostrap phase of your app because they affect the global environment (by modifying window or document or such) so having a stable runtime should be the 1st thing that happens.

For reference this is what Yahoo & Dropbox do.

mortargrind commented 4 years ago

I agree what you are saying in general but hear me out:

Right now this library is executing some Intl related code even before calling anything from it. And it's doing this not because it has to have some sort of side effect on load time: It just happens because of having a static member.

In my opinion, loading the polyfills before your application runs should be enough if there's no physical need to do otherwise. Not because what you are saying is incorrect; but because it makes things so much easier as you can see in this create-react-app example.

Using a 3rd party service for polyfilling is not viable in my case and this will be true for many of your consumers. I cannot simply delegate the responsibility of showing an error page or serving the app itself properly to a 3rd party service.

By the way I am not using create-react-app or any other 3rd party pre-made bundler scripts; I have my own custom Webpack config so I have full control over how things are built and bundled. So I can live with this personally, but that might not be true for everyone else. Hence my example.

In addition to this: these issues' actual reasons are the same thing that I am reporting here: https://github.com/formatjs/react-intl/issues/1579 https://github.com/formatjs/react-intl/issues/1584

None of the libs I have used so far has this kind of requirement so this will be unexpected for your consumers, especially if they can't easily adjust their build & bundling scripts however they like. Changing this one line from a static class member to a static class method would make everything much easier for everyone in the long term. Including people who has much less experience with bundlers, like the majority of create-react-app users.

So my proposal is: Instead of having defaultLocale static property, having a static getDefaultLocale method that returns new Intl.NumberFormat().resolvedOptions().locale would solve the issue. I can open a PR for this if you are okay with this.

longlho commented 4 years ago

I'm open to a PR to turn that into a memoized fn call.