Upgrade react-intl to v3 #2721

mcmxcdev · 2019-08-19T22:29:06Z

Based on the comment by @julienben in #2686, I decided to upgrade react-intl to its newest version.

Unfortunately I was not able to test if the polyfills are working since I dont have access to neither Safari, Edge or IE on Ubuntu, maybe someone can help out ;)

* new formatjs/intl-relativetimeformat and intl-pluralrules dependencies as react-intl polyfills * changes in jest.config.js, webpack.base.babel.js, i18n.js and ToggleOption component according to react-intl upgrade guide * various snapshot updates due to internal react-intl changes Signed-off-by: mhatvan <[email protected]>

coveralls · 2019-08-19T22:33:02Z

Coverage remained the same at 100.0% when pulling 5a90578 on mhatvan:chore/upgrade-react-intl-to-v3 into 23a4be4 on react-boilerplate:dev.

mcmxcdev · 2019-09-17T06:31:55Z

@julienben saw your feedback in #2686 (comment) :
How exactly is this PR broken/incomplete? If its just about the conflicting files thats done fast.
I was waiting for feedback which I didn't receive yet. Would be happy to finish this ;)

julienben · 2019-09-19T16:47:07Z

Hey @mhatvan,

Sorry about that! I just assumed something was broken given that the CI didn't pass. From a quick look at the code, everything seems good and the CI issues seem easy to fix.

I'm assuming the two new dependencies are required by the new version of react-intl? I'd like to check the evolution of the build size after this change.

A couple more things:

Did you check if the documentation needs to be updated? Does it make any reference to now-outdated code? Does it explain how to add a new language and now those instructions should be updated?
Same for the generators which allow adding a new language. Did you try out npm run generate language?

Btw, I have access to a PC with IE and other browsers and will test it out ASAP.

…erplate into chore/upgrade-react-intl-to-v3

* react-intl upgrade from 3.1.9 to 3.3.0 * switch from intl-pluralrules to @formatjs/intl-pluralrules * update i18n.js and i18n.md Signed-off-by: mhatvan <[email protected]>

mcmxcdev · 2019-09-20T20:07:55Z

@julienben In my newest commit, I resolved merge conflicts and updated the new dependency imports since the documentation of the actual upgrade guide itself changed.

I'm assuming the two new dependencies are required by the new version of react-intl? I'd like to check the evolution of the build size after this change.

The new dependencies are necessary due to breaking API changes -> addLocaleDate has been
removed from the react-intl dependency. You can read about it here: https://github.com/formatjs/react-intl/blob/master/docs/Upgrade-Guide.md#breaking-api-changes

Did you check if the documentation needs to be updated? Does it make any reference to now-outdated code? Does it explain how to add a new language and now those instructions should be updated?

Good point, I updated the i18n.md accordingly.

Same for the generators which allow adding a new language. Did you try out npm run generate language?

Still works as expected, verified it with added fr locale.

Btw, I have access to a PC with IE and other browsers and will test it out ASAP.

That would be great, so that we can get this merged.

gretzky

LGTM on my end! Thanks @mhatvan

julienben

Hey @mhatvan! Thanks for your hard work on this! Looks great and almost ready to merge. Please see a couple of final comments.

Also, are you sure about the language generator still working? Shouldn't it also add language-specific locale data for the PluralRules and RelativeTimeFormat polyfills?

// In app/i18n.js, when adding for example 'fr':

require('@formatjs/intl-pluralrules/dist/locale-data/fr');

require('@formatjs/intl-relativetimeformat/dist/locale-data/fr');

Right now it's adding it using the old format:

const frLocaleData = require('react-intl/locale-data/fr');

addLocaleData(frLocaleData);

See internals/generators/language/index.js and related HBS templates.

julienben · 2019-09-22T08:08:01Z

app/i18n.js

+if (!Intl.RelativeTimeFormat) {
+ require('@formatjs/intl-relativetimeformat/polyfill');
+ require('@formatjs/intl-relativetimeformat/dist/locale-data/de'); // Add locale data for de
+}


The sample app has both de and en so I believe you're missing a couple lines above:

if (!Intl.PluralRules) { require('@formatjs/intl-pluralrules/polyfill'); require('@formatjs/intl-pluralrules/dist/locale-data/de'); // Add locale data for de require('@formatjs/intl-pluralrules/dist/locale-data/en'); // Add locale data for en } if (!Intl.RelativeTimeFormat) { require('@formatjs/intl-relativetimeformat/polyfill'); require('@formatjs/intl-relativetimeformat/dist/locale-data/de'); // Add locale data for de require('@formatjs/intl-relativetimeformat/dist/locale-data/en'); // Add locale data for en }

No?

You can read here that the en locale is the default locale, the example shows only the de locale being imported explicitly.

I don't see any mention of en being the default locale at this link. IMO, if @formatjs/intl-relativetimeformat/dist/locale-data/en didn't need to be included, it wouldn't exist in the locale-data folder but it does.

I guess I was not imagining it, they removed it from the README.md a while ago: formatjs/formatjs@cda3cf2#diff-70b3d6472c5178da3042f45cb893ef56

I am updating react-intl and relevant dependencies again, since there were a lot of new version releases in the mean time, I will keep you updated.

Thanks @mhatvan! Sorry again for the delay on my part!

julienben · 2019-09-22T08:19:01Z

internals/webpack/webpack.base.babel.js

@@ -21,7 +21,8 @@ module.exports = options => ({
 rules: [
 {
 test: /\.jsx?$/, // Transform all .js and .jsx files required somewhere with Babel
- exclude: /node_modules/,
+ // excluding full node_modules folder except for react-intl related deps, see: https://github.com/formatjs/react-intl/blob/master/docs/Upgrade-Guide.md#webpack
+ exclude: /node_modules\/(?!(react-intl|intl-messageformat|intl-messageformat-parser)\/).*/,


IMO, your comment should link to the setup part of the docs instead of the upgrade guide:

https://github.com/formatjs/react-intl/blob/master/docs/Getting-Started.md#esm-build

Also, did you check if adding those artifacts to the exclude instead of including them specifically mean they do get transpiled correctly? (If you're not sure, I'll find out when checking this upgrade on an IE setup this week.)

You are right, changed the link ;)

I checked the output of npm run build and the libraries appeared as chunks, transpiled to ES5, but please doublecheck this on IE.

tested using windows env, as windows have different file paths maybe exclude should be
/node_modules[\\,/](?!(react-intl|intl-messageformat|intl-messageformat-parser)[\\,/]).*/

Thank you, I changed it in the newest commit.

julienben · 2019-09-22T08:43:41Z

A couple more things I forgot about:

Coverage went down from 100% to 99.3%, probably because of polyfills being conditionally required. Can you just add istanbul ignore comments to fix that?
In app/app.js, we import locale-data as well (+ the generator adds this line for new languages as well). Do you know if this is now redundant because the new polyfill setup takes care of that or not?

(Sorry about this upgrade being so tedious. i18n is always annoying to deal with.)

mcmxcdev · 2019-09-23T21:28:56Z

In app/app.js, we import locale-data as well (+ the generator adds this line for new languages as well). Do you know if this is now redundant because the new polyfill setup takes care of that or not?

Hard to get a good overview of the intl stuff, but:
After checking the react-intl repo, I saw that they don't have intl in their dependencies, and the intl bundle languages use IntlPolyfill.__addLocaleData internally which is deprecated according to the documentation.
So I would assume that we could remove the code in app/app.js.

Apart from that, all browsers seem to be green for at least basic functionality of intl, see: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl#Browser_compatibility

* change add-locale-data.hbs and intl-locale-data.hbs generation * add istanbul ignore to conditional polyfill imports * remove intl polyfill in app/app.js and polyfill-intl-locale.hbs * change documentation url for intl webpack transpilation Signed-off-by: mhatvan <[email protected]>

mcmxcdev · 2019-09-23T22:22:27Z

@julienben I pushed another commit which updates the language generation, more specifically, it adds the required language polyfills for new languages e.g.: fr

merqurio · 2019-09-28T07:43:15Z

app/app.js

- throw err;
- });
-} else {
- render(translationMessages);


Hi @mhatvan , thanks for your work.

I tested this PR; removing ther render() in line 88 makes the application not load for the first time, although if you make any change in the source code the HMR kicks in and the app renders correctly. This affects the production build, that doesn't render the application at all.

Please, note that I'm working on a fork, but this should have the same results in the template

Hope this helps with the PR.

Hey, thank you for your review!

You are totally right and I am glad you spotted this, I accidentally removed the render() when removing the check for window.Intl, I re-added it again in the newest commit.

Happy to help,

Are you sure we can remove window.Intl safely ? Checking Can I use and Mozilla seems like not all the major browsers support window.Intl.PluralRules. Not sure if this applies.

Bests !

The new @formatjs/intl-pluralrules dependency should take care of that.

Check up in the react-intl upgrade guide: https://github.com/formatjs/react-intl/blob/master/docs/Upgrade-Guide.md#migrate-to-using-native-intl-apis

And you can read the discussion here: #2721 (comment)

Signed-off-by: markushatvan <[email protected]>

…ndows environments Signed-off-by: markushatvan <[email protected]>

…erplate into chore/upgrade-react-intl-to-v3 Signed-off-by: mhatvan <[email protected]>

…eact-boilerplate into chore/upgrade-react-intl-to-v3

absqueued · 2019-11-04T06:06:03Z

Can we get this through? Please.

julienben

Sorry it took me so long to review this! I only added one comment regarding en as the default locale.

More importantly though, I cloned your fork and tested this branch on a Windows 10 laptop with IE11 and the app was broken. This is what I got in the console:

I'm not sure what the issue is but if you have an idea and would like me to try a fix on PC, let me know. I'll make sure to take care of it faster this time!

julienben · 2019-11-17T13:13:01Z

app/i18n.js

+if (!Intl.RelativeTimeFormat) {
+ require('@formatjs/intl-relativetimeformat/polyfill');
+ require('@formatjs/intl-relativetimeformat/dist/locale-data/de'); // Add locale data for de
+}


I don't see any mention of en being the default locale at this link. IMO, if @formatjs/intl-relativetimeformat/dist/locale-data/en didn't need to be included, it wouldn't exist in the locale-data folder but it does.

…erplate into chore/upgrade-react-intl-to-v3

* add full-icu package to fix locale data missing error in jest tests * add english locale data polyfill for Intl.Pluralrules and Intl.RelativeTimeFormat * change language generation to add locale data polyfills after english locale data Signed-off-by: mhatvan <[email protected]>

mcmxcdev · 2019-11-21T17:36:01Z

More importantly though, I cloned your fork and tested this branch on a Windows 10 laptop with IE11 and the app was broken. This is what I got in the console:

I'm not sure what the issue is but if you have an idea and would like me to try a fix on PC, let me know. I'll make sure to take care of it faster this time!

@julienben I found out and was able to reproduce this by adding delete Intl.PluralRules in i18n.js to simulate a kind of faked IE11 mode.
After updating react-intl and related dependencies to the newest versions again, the error you mentioned went away.

I also added the full-icu dependency to fix missing locale data errors in jest, see: formatjs/formatjs#1401

Please run the latest commit again locally against your W10 IE11 environment and tell me if there are any remaining issues.

mcmxcdev · 2020-01-28T20:59:40Z

@julienben I am aware of the conflicting files, but didn't do any further changes since I want you to decide when you have time and are ready to review this MR.

ividic · 2020-02-10T04:41:55Z

package.json

 "chalk": "2.4.2",
 "compression": "1.7.4",
 "connected-react-router": "6.5.2",
 "cross-env": "6.0.3",
 "express": "4.17.1",
 "fontfaceobserver": "2.1.0",
+ "full-icu": "1.3.0",


Should this be a devDependency?

The install instructions on https://www.npmjs.com/package/full-icu are displaying the commands for putting it in dependencies (no -D flag), but I am open to move it to the devDependencies, if desired.

I hope we can expect some activity from @julienben soon?

Hope to check soon. I have a problem with upgrade intl to v3

cuongdevjs · 2020-02-13T09:09:32Z

package.json

 "chalk": "2.4.2",
 "compression": "1.7.4",
 "connected-react-router": "6.5.2",
 "cross-env": "6.0.3",
 "express": "4.17.1",
 "fontfaceobserver": "2.1.0",
+ "full-icu": "1.3.0",


Hope to check soon. I have a problem with upgrade intl to v3

Can-Sahin · 2020-07-03T12:57:28Z

Edit: Sorry I am opening this back as a result of not merging the #2935 if that means anything;)

mcmxcdev requested review from gretzky, julienben, justingreenberg, jwinn and JohnMostlyR as code owners August 19, 2019 22:29

justingreenberg added the dependencies Pull requests that update a dependency file label Aug 21, 2019

julienben mentioned this pull request Sep 16, 2019

React Boilerplate v4.1 #2686

Closed

mhatvan added 2 commits September 20, 2019 21:04

Merge branch 'dev' of https://github.com/react-boilerplate/react-boil…

18c7683

…erplate into chore/upgrade-react-intl-to-v3

Upgrade react-intl to v3 (#2721)

b23f561

* react-intl upgrade from 3.1.9 to 3.3.0 * switch from intl-pluralrules to @formatjs/intl-pluralrules * update i18n.js and i18n.md Signed-off-by: mhatvan <[email protected]>

gretzky approved these changes Sep 21, 2019

View reviewed changes

julienben requested changes Sep 22, 2019

View reviewed changes

Markus Hatvan and others added 2 commits September 24, 2019 00:34

Merge branch 'dev' into chore/upgrade-react-intl-to-v3

f53ec6c

Merge branch 'dev' into chore/upgrade-react-intl-to-v3

7fcfa8f

merqurio reviewed Sep 28, 2019

View reviewed changes

markushatvan added 2 commits September 29, 2019 17:36

re-add render() to app.js

867a52c

Signed-off-by: markushatvan <[email protected]>

change exclude regex in webpack.base.babel.js to work for unix and wi…

5955318

…ndows environments Signed-off-by: markushatvan <[email protected]>

julienben mentioned this pull request Oct 25, 2019

Update i18n.md #2765

Closed

7 tasks

mhatvan added 2 commits October 28, 2019 18:35

Merge branch 'dev' of https://github.com/react-boilerplate/react-boil…

35128fe

…erplate into chore/upgrade-react-intl-to-v3 Signed-off-by: mhatvan <[email protected]>

Merge branch 'chore/upgrade-react-intl-to-v3' of github.com:mhatvan/r…

c368d93

…eact-boilerplate into chore/upgrade-react-intl-to-v3

merqurio approved these changes Nov 15, 2019

View reviewed changes

Merge branch 'dev' into chore/upgrade-react-intl-to-v3

772e4ce

julienben requested changes Nov 17, 2019

View reviewed changes

Merge branch 'dev' of https://github.com/react-boilerplate/react-boil…

37ad6a7

…erplate into chore/upgrade-react-intl-to-v3

julienben mentioned this pull request Nov 21, 2019

React Boilerplate v5.0 #2815

Draft

6 tasks

ividic reviewed Feb 10, 2020

View reviewed changes

cuongdevjs approved these changes Feb 13, 2020

View reviewed changes

colbysadams mentioned this pull request Feb 23, 2020

Update react intl v3 - saga based translation files #2866

Closed

Can-Sahin closed this Jul 3, 2020

Can-Sahin reopened this Jul 12, 2020

mcmxcdev closed this Oct 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade react-intl to v3 #2721

Upgrade react-intl to v3 #2721

mcmxcdev commented Aug 19, 2019

coveralls commented Aug 19, 2019 •

edited

mcmxcdev commented Sep 17, 2019

julienben commented Sep 19, 2019

mcmxcdev commented Sep 20, 2019

gretzky left a comment

julienben left a comment •

edited

julienben Sep 22, 2019 •

edited

mcmxcdev Sep 23, 2019 •

edited

julienben Nov 17, 2019

mcmxcdev Nov 21, 2019

julienben Nov 21, 2019

julienben Sep 22, 2019

julienben Sep 22, 2019

mcmxcdev Sep 23, 2019

ewief Sep 30, 2019

mcmxcdev Oct 2, 2019

julienben commented Sep 22, 2019 •

edited

mcmxcdev commented Sep 23, 2019 •

edited

mcmxcdev commented Sep 23, 2019

merqurio Sep 28, 2019

mcmxcdev Sep 29, 2019

merqurio Sep 30, 2019

mcmxcdev Oct 2, 2019

absqueued commented Nov 4, 2019

julienben left a comment

julienben Nov 17, 2019

mcmxcdev commented Nov 21, 2019 •

edited

mcmxcdev commented Jan 28, 2020

ividic Feb 10, 2020

mcmxcdev Feb 12, 2020

cuongdevjs Feb 13, 2020

cuongdevjs Feb 13, 2020

Can-Sahin commented Jul 3, 2020 •

edited

Upgrade react-intl to v3 #2721

Upgrade react-intl to v3 #2721

Conversation

mcmxcdev commented Aug 19, 2019

coveralls commented Aug 19, 2019 • edited

mcmxcdev commented Sep 17, 2019

julienben commented Sep 19, 2019

mcmxcdev commented Sep 20, 2019

gretzky left a comment

Choose a reason for hiding this comment

julienben left a comment • edited

Choose a reason for hiding this comment

julienben Sep 22, 2019 • edited

Choose a reason for hiding this comment

mcmxcdev Sep 23, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julienben commented Sep 22, 2019 • edited

mcmxcdev commented Sep 23, 2019 • edited

mcmxcdev commented Sep 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

absqueued commented Nov 4, 2019

julienben left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcmxcdev commented Nov 21, 2019 • edited

mcmxcdev commented Jan 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Can-Sahin commented Jul 3, 2020 • edited

coveralls commented Aug 19, 2019 •

edited

julienben left a comment •

edited

julienben Sep 22, 2019 •

edited

mcmxcdev Sep 23, 2019 •

edited

julienben commented Sep 22, 2019 •

edited

mcmxcdev commented Sep 23, 2019 •

edited

mcmxcdev commented Nov 21, 2019 •

edited

Can-Sahin commented Jul 3, 2020 •

edited