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

Internationalization & localization #1822

Merged
merged 14 commits into from
Nov 9, 2023
Merged

Internationalization & localization #1822

merged 14 commits into from
Nov 9, 2023

Conversation

ansh
Copy link
Contributor

@ansh ansh commented Nov 6, 2023

Allow for the app to be consumed in multiple languages and locales

This PR installs Lingui and converts most text to support localization. Check internationalization.md for detailed info

@ansh ansh marked this pull request as draft November 6, 2023 00:13
docs/internationalization.md Outdated Show resolved Hide resolved
docs/internationalization.md Show resolved Hide resolved

// ✅ Good! `useMemo` has i18n context in the dependency
export function Welcome() {
const linguiCtx = useLingui();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I understand correctly that these pitfalls only matter if we allow changing the locale without reloading the app? I guess that's unavoidable on native?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is correct.

If we use the hook and properly memoize any functions that use data from the hook, then the UI will react to changes in the locale.

When it's implemented in the whole app, I doubt it'll be be instantaneous as the new locale will need to be loaded, then all components will need to be re-rendered. It'll be like switching from light mode -> dark mode, I think.

@@ -26,6 +29,7 @@ const App = observer(function AppImpl() {
setRootStore(store)
analytics.init(store)
})
dynamicActivate(defaultLocale) // async import of locale data
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks concerning to me. What will it be doing while the locale data is loading? Is this blocking? If it's not blocking, how does the UI work in the meantime?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can check the i18n.ts code -- it's just a dynamic import

Copy link
Collaborator

@gaearon gaearon Nov 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand it's a dynamic import — what I don't understand is what strings will we be showing until the default locale loads?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can either show nothing or the default locale (English)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's see if any screen flashing becomes an issue...

Copy link
Collaborator

@gaearon gaearon Nov 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Things to verify on mobile:

  • we shouldn't flash the wrong locale
  • we shouldn't introduce any new async delays; all files are already on the disk

Things to verify on web:

  • we shouldn't flash the wrong locale
  • we shouldn't introduce a waterfall (fetch code -> try to render -> discover we don't have a locale -> download locale). if locale is known at the request time, it should download in parallel with the code.

At FB I think we'd just create a complete JS bundle per locale and force a full reload on locale change. This is nice because you don't have to worry about locale changing dynamically at all.

I wonder if an approach like this would work for us? My concern with locales as separate files is that they grow unbounded: we'd have to download all strings even if we use code splitting for the actual code (and so the initial bundle doesn't need all strings). I think that's the motivation for replacing content in the bundles instead.

Its docs are not great, but I strongly recommend investigating how https://facebook.github.io/fbt/ handles this. I wonder if the open source version does bundle replacement... Can you check how this works? https://github.com/facebook/fbt/tree/main/demo-app

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lingui works very similar to FBT from what I can tell. I'll take a deeper look and then we can adjust the method that we use. However, I do think that not reloading the app on locale change would be a better user experience.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm inclined to say we should bundle the english locale and then dynamically load the preferred locale once we have the preference. It's not ideal but it may be the least worst option - ensures there are strings and keeps the bundle as small as possible.

The calculus is likely different between native and web. Web is the harder one. Caching may help us -- though we also need to ensure dynamic loading works in the prod build

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On mobile, everything will be bundled automatically and then switch when we want it to. I expect our locale messages.js files to be around 20kb. Right now its 14kb for English.

On web, it will be fetched when required. We can cache it and that will definitely help. Here is how it works on web: https://lingui.dev/guides/dynamic-loading-catalogs#conclusion

On RN, we would have to implement our own solution. There has been some prior art on this: lingui/js-lingui#503

@ansh ansh marked this pull request as ready for review November 8, 2023 02:37
Copy link
Collaborator

@pfrazee pfrazee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whew! Nice work @ansh. Added some fixes. If you can hit those, then we can merge but finish the conversation about how locales are loaded and do a followup PR.

Copy link
Collaborator

@pfrazee pfrazee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

@ansh ansh merged commit 4c7850f into main Nov 9, 2023
4 checks passed
estrattonbailey added a commit that referenced this pull request Nov 9, 2023
* origin:
  Fix tab alignment on the web (#1857)
  Show tabs when swiping feeds (#1856)
  Sync top/bottom bar disappearance to the scroll (#1855)
  Hotfix internationalization on mobile (#1854)
  Internationalization & localization (#1822)
  Hide/show header and footer without re-renders, take two (#1849)
@ansh ansh mentioned this pull request Nov 16, 2023
@skipness skipness mentioned this pull request Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants