fix: do not remount children of I18nProvider #1501

vonovak · 2023-03-11T11:08:58Z

Description

While working on #1426 I noticed the issue reported in #1136. Using a custom key in the I18nProvider results in remounting of the whole app which is not a standard practice in React providers.

If someone needs to do remounting, I would suggest they implement this themselves, but the lingui/react library should not do this out of the box. The PR removes the usage of key prop altogether.

Breaking changes:

Previously the whole app would re-mount when calling activate() or load() and this will no longer be the case. If someone uses translations in their components using just

t`some string`

then those occurrences will not re-render. This, I believe, is correct behavior. We need to document how to make those places re-render - ~~I'm not really sure myself~~ using the useLingui hook .

In version 2 you could do

i18n._(t`this will be translated`)

what is the equivalent of it for version 4?

Thanks for reviewing! 🙂

Types of changes

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation update

Fixes # (issue)

Checklist

I have read the CONTRIBUTING and CODE_OF_CONDUCT docs
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation (if appropriate)

vercel · 2023-03-11T11:09:02Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated
js-lingui	✅ Ready (Inspect)	Visit Preview	💬 Add your feedback	Mar 19, 2023 at 9:04AM (UTC)

github-actions · 2023-03-11T11:12:42Z

size-limit report 📦

Path	Size
./packages/core/build/esm/index.js	1.57 KB (0%)
./packages/detect-locale/build/esm/index.js	812 B (0%)
./packages/react/build/esm/index.js	1.67 KB (0%)
./packages/remote-loader/build/esm/index.js	7.25 KB (0%)

codecov · 2023-03-11T16:15:46Z

Codecov Report

Patch coverage: 92.85% and project coverage change: -0.70 ⚠️

Comparison is base (4f1d30e) 75.75% compared to head (53cd2e1) 75.06%.

Additional details and impacted files

@@            Coverage Diff             @@
##             next    #1501      +/-   ##
==========================================
- Coverage   75.75%   75.06%   -0.70%     
==========================================
  Files          79       79              
  Lines        1980     1961      -19     
  Branches      516      513       -3     
==========================================
- Hits         1500     1472      -28     
- Misses        360      372      +12     
+ Partials      120      117       -3

Impacted Files	Coverage Δ
packages/react/src/I18nProvider.tsx	`91.30% <92.85%> (-0.70%)`	⬇️

... and 17 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

timofei-iatsenko · 2023-03-12T11:04:07Z

We need to document how to make those places re-render - I'm not really sure myself

the idiomatic way to make it rerender in component is

function MyComponent() {
  const i18n = useLingui();
  t(i18n)`My Message`;
}

But it's quite verbose and actually is enough to just use useLingui(); without passing a custom i18n instance to t macro

function MyComponent() {
  useLingui(); // <-- will subscribe this component for the locale changes
  t`My Message`; // <-- uses global i18n instance
}

i actually thinking of creating an additional macro, similar to useLingui

function MyComponent() {
 const {t} = useLingui(); // <-- imported from @lingui/macro
 t`My Message`;
}

This will use i18n instance from components tree, automatically subscribe for changes and in general less error-prone. But tranformation of this macro is a bit tricky from AST perspective, because you need to traverse scope in both directions (babel-macro-plugin limitations)

timofei-iatsenko · 2023-03-12T12:49:25Z

Could you document that change in a migration guide?

timofei-iatsenko · 2023-03-13T08:41:29Z

big, big thanks for updating other outdated pages as well!

website/docs/ref/react.md

timofei-iatsenko · 2023-03-13T15:14:47Z

website/docs/releases/migration-4.md

+### I18nProvider no longer remounts its children on locale change
+
+Previously, the `I18nProvider` remounted its children on locale change. This ensured that the whole app was re-rendered with the new locale and all strings were rendered correctly translated.
+Apart from not being very performant, this approach had the drawback that the state of the app was lost - all components were re-mounted and their state was reset. This is not a standard behavior of React Context providers and could cause some confusion.
+
+In v4, the `I18nProvider` no longer remounts its children on locale change. Instead, when locale changes, the context value provided by `I18nProvider` is updated and all components that consume the provided React Context are re-rendered with the new locale.
+This includes components provided by Lingui, such as `Trans` or `Plural` and also custom components that use the `useLingui` hook.
+
+This is the default behavior, but you can disable it by setting `forceRenderOnLocaleChange={false}`.


is there any way to turn v3 behaviour for transition period? As far as i understand, forceRenderOnLocaleChange={false} will not make it as in v3, right?

I checked the code one more time and question arised. What is the value of forceRenderOnLocaleChange=false ? I could not imagine a usecase where user will not want to have application to react to the locale changes.

I think the previous behaviour of this flag was indeed confusing and some people wanted to disable it, that's why this flag exists. But with current implementation, is it better to delete it completely? Or make this flag kinda enabling v3 compatibility?

hey @thekip I'm not sure about the history of the code and why forceRenderOnLocaleChange was added TBH.
I guess someone had some use case for the I18nProvider to not do anything "clever" but only pass the i18n object down.

I think a good way forward would be to remove the forceRenderOnLocaleChange prop, have the I18nProvider behave as if forceRenderOnLocaleChange={true} AND export the LinguiContext so that people can build their own context providers where they can do whatever they want.
This will make the usual thing easy to do, and harder / unusual things possible (the v3 behavior, for example - whatever it was 😄).

Wdyt?

#1194 - might be related

@vonovak

export the LinguiContext so that people can build their own context providers

I always thought that exporting context directly from the lib is not a good thing. At least i haven't seen such examples in big projects.

Usually, libs give more high level primitives to work with. Such as usLingui() hook. What could be done with exported Context what couldn't be done with just useLingui hook? (sorry, I'm not too much into React)

I think a good way forward would be to remove the forceRenderOnLocaleChange prop

What stopping us from deleting it in this PR? I don't see a value of this anymore.

I also a bit worried about transition path. If someone rely on the forceRenderOnLocaleChange in v3, for transition to v4, should go and find all places where unsafe t (and other from core) macro calls are used and add a useLingui hook. That might be quite a blocking factor for adopting v4. What do you think?

I always thought that exporting context directly from the lib is not a good thing. At least i haven't seen such examples in big projects.

Would be nice to not have to do that but I don't think it does a lot of harm (it's just an exported object) and it gives users the flexibility to do anything they want.

Here are some examples that export the Context: react-redux, ~~mobx-react~~ (no longer having own context) - these are pretty big projects.

Usually, libs give more high level primitives to work with

yes, and we give the I18nProvider which should cover the usual cases. And useLingui() which works fine as-is.

If our I18nProvider does not suit someone well, and the LinguiContext is exported, we're not blocking users to do whatever they please. When someone encounters a problem with I18nProvider and opens an issue, a solution can be found if the LinguiContext can be used by developers (Then they can share their custom I18nProvider and perhaps we can integrate it in the next version).

If we don't export the LinguiContext then we're blocking people from making custom solutions possible. At the same time, exporting it doesn't mean maintenance problems for the lib afaict.

What could be done with exported Context what couldn't be done with just useLingui hook?

Well, this whole question concerns the I18nProvider, not really the useLingui hook which works fine. But to answer "what can be done with with exported Context". The answer is that anything can be done. People use open source software in ways that a maintainer cannot always anticipate and I think it's best not to block people from doing what they want unless it has maintenance burden (exporting one Context object is not a maintenance burden).

If you want a more specific example I think I can come up with something :).

What stopping us from deleting forceRenderOnLocaleChange in this PR?

Nothing, I'll happily do it.

If someone rely on the forceRenderOnLocaleChange in v3, for transition to v4, should go and find all places where unsafe t (and other from core) macro calls are used and add a useLingui hook. That might be quite a blocking factor for adopting v4.

they should do it anyways, regardless of whether they had forceRenderOnLocaleChange true or false.

If they (incorrectly) used the t macro without useLingui then their app might break in some cases. If we export LinguiContext then they can copy the I18nProvider from v3 into their code base. Then they will have the old behavior of translation rendering but enjoy other benefits of v4.
I agree I need to document this better.

Does this make sense? Thanks! 🙂

@vonovak can we move forward with this PR and merge it or you're going to address the discussion regarding the forceRenderOnLocaleChange here?

packages/react/src/I18nProvider.test.tsx

packages/react/src/I18nProvider.tsx

ghost · 2023-03-16T19:33:38Z

Will this fix be merged to version v4 for testing soon?

andrii-bodnar · 2023-03-16T19:45:52Z

@arindamganguly it is still in draft but we will merge it into v4 as soon it will be ready

website/docs/ref/react.md

website/docs/releases/migration-4.md

andrii-bodnar

@vonovak thank you!

siva-I · 2023-03-20T17:48:34Z

@andrii-bodnar Could you please advise when the new version with this fix available?

andrii-bodnar · 2023-03-20T17:50:22Z

@siva-I I'm going to release the v4.0.0-next.3 later this week (probably on Thursday or Friday)

siva-I · 2023-03-20T17:56:54Z

@andrii-bodnar is there a chance you could do it earlier? we are currently have big blocker and I really wanted to test it if this fixes our issue. I really appreciate the efforts.

andrii-bodnar · 2023-03-21T14:57:57Z

@siva-I the v4.0.0-next.3 is available

siva-I · 2023-03-21T17:13:21Z

@andrii-bodnar Thanks for the quick update. How can i upgrade my lingui package. The version doesn't seems to be on npmjs

I am trying this and it is not updating the next version.
yarn upgrade --scope @lingui --registry https://registry.npmjs.org --latest

vonovak · 2023-03-21T18:01:07Z

@andrii-bodnar Thanks for the quick update. How can i upgrade my lingui package. The version doesn't seems to be on npmjs

I am trying this and it is not updating the next version. yarn upgrade --scope @lingui --registry https://registry.npmjs.org --latest

please see here https://www.npmjs.com/package/@lingui/react?activeTab=versions

vercel bot deployed to Preview March 11, 2023 11:11 View deployment

timofei-iatsenko requested review from timofei-iatsenko and andrii-bodnar March 11, 2023 18:54

vonovak changed the title ~~fix: do not unmount children of I18nProvider~~ fix: do not remount children of I18nProvider Mar 11, 2023

vercel bot deployed to Preview March 12, 2023 13:35 View deployment

vonovak marked this pull request as draft March 12, 2023 13:42

vercel bot deployed to Preview March 12, 2023 19:10 View deployment

vonovak force-pushed the fix/I18nProvider-unmount branch from 1368562 to 15c3a98 Compare March 12, 2023 19:12

vercel bot deployed to Preview March 12, 2023 19:14 View deployment

andrii-bodnar linked an issue Mar 13, 2023 that may be closed by this pull request

I18nProvider remounts all the children, but not rerender #1136

Closed

vonovak force-pushed the fix/I18nProvider-unmount branch from 15c3a98 to e53823a Compare March 13, 2023 13:08

vercel bot deployed to Preview March 13, 2023 13:12 View deployment

vonovak force-pushed the fix/I18nProvider-unmount branch from d19665d to 898ac86 Compare March 13, 2023 13:23

vercel bot deployed to Preview March 13, 2023 13:25 View deployment

vercel bot deployed to Preview March 13, 2023 14:17 View deployment

vonovak marked this pull request as ready for review March 13, 2023 14:26

timofei-iatsenko reviewed Mar 13, 2023

View reviewed changes

vercel bot deployed to Preview March 13, 2023 18:51 View deployment

vercel bot deployed to Preview March 13, 2023 19:30 View deployment

vercel bot deployed to Preview March 13, 2023 19:42 View deployment

andrii-bodnar force-pushed the next branch from 8d34239 to 2b6f563 Compare March 14, 2023 14:33

vercel bot deployed to Preview March 16, 2023 08:25 View deployment

vonovak added 2 commits March 16, 2023 13:29

fix: do not unmount children of I18nProvider

bb2d99a

docs: document I18nProvider changes

b82334c

vercel bot deployed to Preview March 16, 2023 12:32 View deployment

vonovak marked this pull request as draft March 16, 2023 12:39

docs: update I18nProvider docs and v4 changes

d50f139

vercel bot deployed to Preview March 17, 2023 18:50 View deployment

vonovak commented Mar 17, 2023

View reviewed changes

website/docs/ref/react.md Show resolved Hide resolved

refactor: remove react.Fragment

32ad005

vercel bot deployed to Preview March 17, 2023 18:56 View deployment

docs: fix lint

b9055c1

vonovak marked this pull request as ready for review March 17, 2023 18:58

vonovak requested review from timofei-iatsenko and andrii-bodnar and removed request for andrii-bodnar and timofei-iatsenko March 17, 2023 18:58

vercel bot deployed to Preview March 17, 2023 19:00 View deployment

andrii-bodnar reviewed Mar 18, 2023

View reviewed changes

website/docs/ref/react.md Outdated Show resolved Hide resolved

website/docs/releases/migration-4.md Outdated Show resolved Hide resolved

vonovak added 2 commits March 19, 2023 09:59

docs: fix headings

b1df433

docs: change useLingui link

53cd2e1

vercel bot deployed to Preview March 19, 2023 09:04 View deployment

andrii-bodnar approved these changes Mar 20, 2023

View reviewed changes

andrii-bodnar merged commit 1e9a581 into lingui:next Mar 20, 2023

vonovak deleted the fix/I18nProvider-unmount branch March 20, 2023 09:42

timofei-iatsenko mentioned this pull request Sep 6, 2023

Some translated texts using the t(like txxx) method did not take effect when switching languages. #1757

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: do not remount children of I18nProvider #1501

fix: do not remount children of I18nProvider #1501

vonovak commented Mar 11, 2023 •

edited

Loading

vercel bot commented Mar 11, 2023 •

edited

Loading

github-actions bot commented Mar 11, 2023 •

edited

Loading

codecov bot commented Mar 11, 2023 •

edited

Loading

timofei-iatsenko commented Mar 12, 2023 •

edited

Loading

timofei-iatsenko commented Mar 12, 2023

timofei-iatsenko commented Mar 13, 2023

timofei-iatsenko Mar 13, 2023

timofei-iatsenko Mar 13, 2023

vonovak Mar 13, 2023 •

edited

Loading

andrii-bodnar Mar 14, 2023

timofei-iatsenko Mar 14, 2023

vonovak Mar 14, 2023 •

edited

Loading

andrii-bodnar Mar 16, 2023

ghost commented Mar 16, 2023

andrii-bodnar commented Mar 16, 2023

andrii-bodnar left a comment

siva-I commented Mar 20, 2023 •

edited

Loading

andrii-bodnar commented Mar 20, 2023

siva-I commented Mar 20, 2023

andrii-bodnar commented Mar 21, 2023

siva-I commented Mar 21, 2023

vonovak commented Mar 21, 2023

fix: do not remount children of I18nProvider #1501

fix: do not remount children of I18nProvider #1501

Conversation

vonovak commented Mar 11, 2023 • edited Loading

Description

Types of changes

Checklist

vercel bot commented Mar 11, 2023 • edited Loading

github-actions bot commented Mar 11, 2023 • edited Loading

size-limit report 📦

codecov bot commented Mar 11, 2023 • edited Loading

Codecov Report

timofei-iatsenko commented Mar 12, 2023 • edited Loading

timofei-iatsenko commented Mar 12, 2023

timofei-iatsenko commented Mar 13, 2023

timofei-iatsenko Mar 13, 2023

Choose a reason for hiding this comment

timofei-iatsenko Mar 13, 2023

Choose a reason for hiding this comment

vonovak Mar 13, 2023 • edited Loading

Choose a reason for hiding this comment

andrii-bodnar Mar 14, 2023

Choose a reason for hiding this comment

timofei-iatsenko Mar 14, 2023

Choose a reason for hiding this comment

vonovak Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

andrii-bodnar Mar 16, 2023

Choose a reason for hiding this comment

ghost commented Mar 16, 2023

andrii-bodnar commented Mar 16, 2023

andrii-bodnar left a comment

Choose a reason for hiding this comment

siva-I commented Mar 20, 2023 • edited Loading

andrii-bodnar commented Mar 20, 2023

siva-I commented Mar 20, 2023

andrii-bodnar commented Mar 21, 2023

siva-I commented Mar 21, 2023

vonovak commented Mar 21, 2023

vonovak commented Mar 11, 2023 •

edited

Loading

vercel bot commented Mar 11, 2023 •

edited

Loading

github-actions bot commented Mar 11, 2023 •

edited

Loading

codecov bot commented Mar 11, 2023 •

edited

Loading

timofei-iatsenko commented Mar 12, 2023 •

edited

Loading

vonovak Mar 13, 2023 •

edited

Loading

vonovak Mar 14, 2023 •

edited

Loading

siva-I commented Mar 20, 2023 •

edited

Loading