[FF] Setup React intl #9449

lidimayra · 2022-02-18T17:08:08Z

This is part of the result of an internationalization project that was worked on during the hackathon (hack9).

Changes in this PR consist of a very basic structure to enable localization on Force.
Once we move forward with the initial setup, we could split subsequent work - internationalizing other strings contained in the project - maybe we could mob on it on another Future Friday (?)

Slack channel: #hack9-localizations
Notion page

We still have some more changes that were excluded from this PR but can be found here like:

Internationalization of some components (This PR is already big enough)
Non-English language translations (we won't support any other language yet)
Dropdown containing language selector on footer (for the same reason above)

src/v2/Apps/Components/AppShell.tsx

dzucconi · 2022-02-18T17:11:04Z

Could you leave some info on these dependencies and why they were chosen (vs possible alternatives)? Like what's with this compile step?

jest.config.legacy.js

dzucconi · 2022-02-18T17:12:39Z

src/v2/Components/NavBar/NavBar.tsx

@@ -149,7 +150,7 @@ export const NavBar: React.FC = track(
                  flex={1}
                  size="small"
                >
-                  Sign Up
+                  <FormattedMessage id="navbar.signup" />


Is there type support (like do these ids autocomplete)?

if this doesnt exist, having a hook or a global func could totally solve this with TS.

const t = useTranslations() return ( <Text> t("navbar.signup") </Text> )

https://formatjs.io/docs/react-intl/api/

https://formatjs.io/docs/react-intl/components#why-components

It seems like this lib does a lot and tries to be a do-all solution for text formatting. Which 🤷

I'm not sure why the hook wouldn't "Provide TypeScript type definitions" though?

m indeed. we can at least do what we do with palette tokens, and wrap their func in our func, and add types based on our json translation files 🤔

pvinis · 2022-02-18T17:13:50Z

src/v2/compiled-lang/en-us.json

@@ -0,0 +1,98 @@
+{
+  "navbar.artists": [


whats the diff of the lang and compiled-lang files? do we need both?

The ones in lang are the ones added by us.
In case we also adopt a third-party translation tool, these are the ones that are synced between our project and the tool.

The ones in compiled-lang are the ones that are actually consumed by the application.

More on this can be found here:

Message extraction

Message distribution

pvinis · 2022-02-18T17:14:28Z

src/v2/Apps/lang/en-us.json

@@ -0,0 +1,17 @@
+{


should we merge this file with the other one outside Apps/?

I think this should live in v2/System/i18n/lang (or we should discuss further per this comment)

I moved this to v2/System/i18n/lang for now, but I can change this in case we decide we really want to have multiple locale files

pvinis · 2022-02-18T17:15:57Z

src/v2/Components/NavBar/NavBar.tsx

@@ -149,7 +150,7 @@ export const NavBar: React.FC = track(
                  flex={1}
                  size="small"
                >
-                  Sign Up
+                  <FormattedMessage id="navbar.signup" />


i still feel very strongly that this should not look like <FormattedMessage id="navbar.signup" />, and instead should look like t("navbar.signup") or something very very minimal.

Yeah, this is kinda confusing. There's a hook that can be used which seems nicer.

Agree with @pvinis! Being locked into an react component for this can be limiting here since we're often using logic in plain js -- ` foo ? "hello" : "world". That kind of thing.

But that said, i could be convinced that a react component like this is indeed sufficient. Worth a bit of discussion.

I left this comment on the wrong parent thread:

https://formatjs.io/docs/react-intl/api/
https://formatjs.io/docs/react-intl/components#why-components

I'm wondering about (and scared of) what their date formatting does.

An implementation like this doesn't seem to be this straightforward with react-intl, but it's the natural way to go in react-i18next.

There is this other PR here with an attempt of using react-i18next: #9797

pvinis · 2022-02-18T17:16:31Z

added some comments, none of them blocking, mostly suggestions and questions.

package.json

damassi · 2022-02-18T17:21:15Z

src/v2/Apps/Components/AppShell.tsx

@@ -18,6 +18,8 @@ import { AppToasts } from "./AppToasts"
 import { useNavBarHeight } from "v2/Components/NavBar/useNavBarHeight"
 import { useProductionEnvironmentWarning } from "v2/Utils/Hooks/useProductionEnvironmentWarning"
 import { useAuthValidation } from "v2/Utils/Hooks/useAuthValidation"
+import { IntlProvider } from "react-intl"
+import locale_en_us from "../../compiled-lang/en-us.json"


Follow-up: I'm curious what including all of the different languages would amount to file-size-wise. In the future we should probably code-split via import() and react-loadable as we do with our other split components as seen here.

damassi · 2022-02-18T17:33:31Z

src/v2/System/SystemContext.tsx

+      <SystemContext.Provider value={providerValues}>
+        {children}
+      </SystemContext.Provider>
+    </IntlProvider>


This can be removed in favor of the Boot suggestion up above

Although we will be using system context for triggering updates to the language, but that's separate from this.

const { setLanguage } = useSystemContext() ... <Select values={languages} onChange={lang => setLanguage(lang) />

That kind of thing.

I made the changes to create an InternationalizationProvider and I'm importing it in Boot, but it's still not really clear to me how can I remove this from SystemContext without affecting the tests... 🤔

It might impact the tests, but in general all of our react context providers live in boot since that's the root of our application. For tests, if we need access to certain providers, we can wrap the test in <MockBoot>

Cool, thank you!!
I wrapped NavBar tests in <MockBoot> and those are passing now! 🙌

Lack of IntlProvider in SystemContext is leading to issues on /terms, though... is this one an exception that doesn't benefit from Boot?

This started to happen after removing IntlProvider from SystemContext because /terms does not rely on Boot. As we can see in the .jade files, we use some stich components, which replicate parts of the components using on static pages like these.

In order to make this work, we should also add the IntlProvider to the components in the stitch_components directory (components that can't benefit from Boot)

src/v2/compiled-lang/en-us.json

damassi · 2022-02-18T17:48:02Z

src/v2/lang/en-us.json

@@ -0,0 +1,50 @@
+{


Wondering what other's think, but should we keep all per-language translations in one single file? I can imagine it getting messy, but at the same time I can also imagine it being functionally easier to use add to and revise, all things considered.

Another option would be to follow the pattern that we've got setup for tests and which everyone is familiar with and add folder-level __i18n__. So like:

├── Bar.tsx ├── Foo.tsx ├── __i18n__ │ ├── Bar.i18n.json │ └── Foo.i18n.json └── __tests__ ├── Bar.test.tsx └── Foo.test.tsx

damassi

PR is looking awesome @lidimayra 💯

Can we add a testing example to this? I'm still uncertain about how that side of things will work, if we were to add intel and then run our existing tests.

damassi

Also echoing @dzucconi, I too am curious about alternative libs and so on. I know React-intl has been around the longest and has a lot users, but what else is out there? We might be able to get away with something simpler that doesn't require a compiler, json files and so on.

Totally naive implementation without understanding any of the underlying gotchas:

const content = {
  en_us: {
    nav: {
      about: "About"
      ... 
    }
  }
}

const useContent = () => {
  const { lang } = useSystemContext()
  
  return {
    content: content[lang]
  }
}

const MyComponent = () => {
  const { content } = useContent() 
  
  return (
    <>
      <Text>{content.nav.about}</Text>
    </>
  )
}

We'd get instant typescript support with no effort, and everything is pretty straightforward.

Again, I'm not an i18n expert, but I do wonder what's strictly needed for the scope of our site.

dzucconi · 2022-02-18T18:37:56Z

@damassi Right, this is exactly what I was wondering. Seems like total overkill for our purposes but maybe I'm missing something?

damassi · 2022-02-18T18:42:33Z

Further, we could bundle split out that content js per language (if indeed there's so much content that it impacts js size -- which I doubt), so we don't even need a compiler other than what webpack natively provides and which our app already supports, while also getting the benefit of not having to work with json files (which is 60% of the time a huge headache).

dzucconi · 2022-02-18T18:44:30Z

@damassi we'd be able to chose the bundle at runtime though? I suppose you could tie this to routing (e.g. https://artsy.net/en-us/artist/andy-warhol) — though that's a whole other convo.

damassi · 2022-02-18T19:01:27Z

Yup, we would. The thing with loadable components (which matches how our route file imports work) is that it's only loaded on demand. For example:

const initLang = (lang) => {
  const content = {
    en_us: () =>  loadable(
      () => import(/* webpackChunkName: "i18n-en_us" */ "./lang/en_us"),
    ),
    es_ty: () =>  loadable(
      () => import(/* webpackChunkName: "i18n-es_ty" */ "./lang/es_ty"),
    )
  }
  
  return {
    load: content[lang]
  }
}

const content = await initLang('en_us').load()

Roughly, something like this would do it, and we could toggle langs similarly.

damassi · 2022-02-19T17:40:06Z

Another thing I woke up thinking about that I can't quite shake is this supposed need for multiple language files, which will be hard to keep in sync (maybe thats what formatjs does? validate and scan all lang content files and evaluate subtle differences?).

But that got me thinking of other possible structures if we were to go with a lightweight internal i18n lib:

const content = {
  nav: {
    home: {
      en_us: "Home",
      es_es: "Casa", 
      ab_cd: "AAfuenf"
    }
  }
}

So in this case, there's only one single master graph for everything which eliminates any potential for drift, and if we need to add a new translation we just update the leaves of each word branch with a new language key / translation. We could then use a js proxy so that we wouldn't have to manually access the current language each time:

content.nav.home

then proxies to

content.nav.home[lang]

Trying to think about how to achieve what we need to achieve with the least amount of moving parts. Having to manually maintain all of our old data fixtures through lib upgrades and so on triggers a bit of code ptsd in me... That's why if we can do simple, we should. Our site isn't so big, all said.

damassi · 2022-02-19T17:45:13Z

Wanted to /cc @jonallured on this. Thoughts on i18n from past experience?

jonallured · 2022-02-22T15:45:12Z

👋 lots of good stuff to ponder in this PR thanks for getting us going on this @lidimayra!! ❤️

My only experience with i18n is in the context of a Rails app and a third party translation service. We worked with them to settle on a file format for the translations and then make a rake task to import them into the project. I don't remember all the details but I'll just note that whatever we do here should probably connect with how we'll work with a third party. I'd hate to see us make tech choices without taking into consideration the larger loop of working with a service ya know?

lidimayra · 2022-03-04T11:53:49Z

Thank you so much for for the comments and the careful review! 🙌❤️
I'm trying to go through everything today.

Something that might answer some questions that were raised by @damassi , @dzucconi and @jonallured :
react-intl works with other third-party services (as it can be seen on this page)

At some point, we want to adopt one of these services (it would be useful for the product design team, and Erin has been talking about this on #hack9-localizations channel). A bit more on this is also available on this notion page.

Besides this, react-intl also provides everything we need to handle pluralization, date formatting, numbers, etc. And it's a widely used lib, with great support from the community.

Adopt react-intl for internationalization See: https://formatjs.io/docs/react-intl/

Add formatsj cli package so we can use it to extract message from files Ref: https://formatjs.io/docs/getting-started/message-extraction/ Example of command to generate en-us.json: ``` yarn i18n:extract 'src/**/*.ts*' --ignore='**/*.d.ts' --out-file \ src/v2/System/i18n/lang/en-us.json --id-interpolation-pattern \ '[sha512:contenthash:base64:6]' ```

Ref: https://formatjs.io/docs/getting-started/message-distribution Example: ``` yarn i18n:compile src/v2/System/i18n/lang/en-us.json --ast \ --out-file src/v2/__generated_i18n__/en-us.json ```

This setting was leading our json files to be misinterpreted by the transpiler (build would fail trying to read json files as js files)

So we can have access to InternationalizationProvider inside tests

lidimayra · 2022-04-01T13:07:39Z

There is another PR with a react-i18next implementation.
See #9797

damassi · 2022-07-15T18:23:08Z

👋 Heads up! A major change is incoming in Force, where we flatten out the src/v2 directory into src. This will take place next Thursday and be merged in, which could impact PRs with rebase conflicts (since all of our import paths will soon change). See this thread for more info.

lidimayra · 2022-07-18T18:48:00Z

Closing this PR in favor of #9797

lidimayra added the #FF label Feb 18, 2022

lidimayra requested review from pvinis, dzucconi, damassi, sonecabr, spkng-slnc, araujobarret and chr-tatu February 18, 2022 17:08

lidimayra self-assigned this Feb 18, 2022

dzucconi reviewed Feb 18, 2022

View reviewed changes

src/v2/Apps/Components/AppShell.tsx Outdated Show resolved Hide resolved

pvinis reviewed Feb 18, 2022

View reviewed changes

jest.config.legacy.js Outdated Show resolved Hide resolved

dzucconi reviewed Feb 18, 2022

View reviewed changes

pvinis reviewed Feb 18, 2022

View reviewed changes

damassi reviewed Feb 18, 2022

View reviewed changes

package.json Outdated Show resolved Hide resolved

damassi reviewed Feb 18, 2022

View reviewed changes

package.json Outdated Show resolved Hide resolved

damassi reviewed Feb 18, 2022

View reviewed changes

src/v2/compiled-lang/en-us.json Outdated Show resolved Hide resolved

damassi reviewed Feb 18, 2022

View reviewed changes

damassi requested a review from jonallured February 19, 2022 17:40

lidimayra force-pushed the ff-setup-react-intl branch 7 times, most recently from 3b4cc33 to f9746ca Compare March 10, 2022 18:07

lidimayra force-pushed the ff-setup-react-intl branch 2 times, most recently from 0dd40f6 to 4d099a4 Compare March 11, 2022 17:02

chr-tatu and others added 8 commits March 18, 2022 16:36

Initial setup of react-intl

63605db

Adopt react-intl for internationalization See: https://formatjs.io/docs/react-intl/

Compile json files from lang/ directory

11c0886

Ref: https://formatjs.io/docs/getting-started/message-distribution Example: ``` yarn i18n:compile src/v2/System/i18n/lang/en-us.json --ast \ --out-file src/v2/__generated_i18n__/en-us.json ```

Start implementation i18n using navbar as example

992edc0

Move lang and compiled-lang to src/v2

8915a73

Enable TS to resolve Json module

a16f685

Ensure json files are excluded in jest config

d54bda1

This setting was leading our json files to be misinterpreted by the transpiler (build would fail trying to read json files as js files)

Apply MockBoot in NavBar tests

a790af9

So we can have access to InternationalizationProvider inside tests

lidimayra force-pushed the ff-setup-react-intl branch from 4d099a4 to a790af9 Compare March 18, 2022 15:39

lidimayra mentioned this pull request Apr 1, 2022

feat(i18n): Setup react-i18next #9797

Merged

lidimayra closed this Jul 18, 2022

[FF] Setup React intl #9449

[FF] Setup React intl #9449

Conversation

lidimayra commented Feb 18, 2022

dzucconi commented Feb 18, 2022

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

damassi Feb 18, 2022 • edited Loading

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

damassi Feb 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvinis commented Feb 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damassi Feb 18, 2022 • edited Loading

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

damassi left a comment

Choose a reason for hiding this comment

damassi left a comment • edited Loading

Choose a reason for hiding this comment

dzucconi commented Feb 18, 2022

damassi commented Feb 18, 2022 • edited Loading

dzucconi commented Feb 18, 2022

damassi commented Feb 18, 2022 • edited Loading

damassi commented Feb 19, 2022 • edited Loading

damassi commented Feb 19, 2022 • edited Loading

jonallured commented Feb 22, 2022

lidimayra commented Mar 4, 2022 • edited Loading

lidimayra commented Apr 1, 2022

damassi commented Jul 15, 2022

lidimayra commented Jul 18, 2022

damassi Feb 18, 2022 •

edited

Loading

damassi Feb 18, 2022 •

edited

Loading

damassi Feb 18, 2022 •

edited

Loading

damassi left a comment •

edited

Loading

damassi commented Feb 18, 2022 •

edited

Loading

damassi commented Feb 18, 2022 •

edited

Loading

damassi commented Feb 19, 2022 •

edited

Loading

damassi commented Feb 19, 2022 •

edited

Loading

lidimayra commented Mar 4, 2022 •

edited

Loading