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

I18nProvider remounts all the children, but not rerender #1136

Closed
AlgoTrader opened this issue Sep 16, 2021 · 14 comments · Fixed by #1501
Closed

I18nProvider remounts all the children, but not rerender #1136

AlgoTrader opened this issue Sep 16, 2021 · 14 comments · Fixed by #1501

Comments

@AlgoTrader
Copy link

Describe the bug
I18nProvider implementation has a very unpleasant effect of changing own key when locale changes. It declares, that forceRenderOnLocaleChange force rerender of all children. Sorry, but its not a rerender, it's full remount of it's children. As a result React.memo is killed and state of all the components is reset. I have google map inside and its remount is slow and flickering

To Reproduce

export default function App() {
   return <I18nProvider i18n={i18n}><PureComponentWithReactMemo></I18nProvider>
}

Expected behavior
It would be great to have all the components just rerendered, not remounted. It is usually done with a trick like having state with counter and incrementing it by one every time we need children to be rerendered.

I cannot have a single global I18nProvider as it remounts everything inside, the heavy components like maps are remounted with unpleasant flicker.

Additional context

  • jsLingui version 3.11.1
  • Babel version - not related
@AlgoTrader
Copy link
Author

AlgoTrader commented Sep 16, 2021

comments in code: We can't use useMemo hook either, because we want to recalculate value manually. useMemo can be used like this:

const context = useMemo(() => {...}, [i18n.locale])

i18n is not changed, but dependency list can hold it's properties, then memo is recalculated on deep changes in i18n

There is another stuff in code:

React.useState<I18nContext>(makeContext())

useState may be passed a function as a parameter, it is named initialState, but you use it like useMemo for tracking props. It's really bad practice to have initialState parameter recalculated on each render. I would write

const context = useMemo(makeContext, [i18n.locale])

BTW makeContext may be wrapped in useCallback

@semoal
Copy link
Contributor

semoal commented Sep 16, 2021

Hi mate, when we worked on the reimplementation of Lingui 3 we forgot a lot about the React side, also we lost a bit the north. I'll be happy to review a pull request from you making the React implementation much better than it is right now.

@JSteunou
Copy link
Contributor

instead of using key a wrapper with the prop lang could be used or a state, to re-render on lang change

@JSteunou
Copy link
Contributor

JSteunou commented Oct 6, 2021

Ok, ignore my previous comment, I talked before looking at the code.

After analysis the issue, I think there are mainly a hole in the documentation and a bad option naming. Children of I18nProvider do re-render after each locale change, because the context is an object containing i18n and not the i18n instance directly. It is correctly re-created after each locale change, see https://github.com/lingui/js-lingui/blob/main/packages/react/src/I18nProvider.tsx#L98

Buuuuut not everything re-render if there is a memo in the tree somewhere under the provider. Only the React parts receiving locale related prop or using context will re-render alone. So using t from macro or core helpers does not re-render unless your component uses useLingui

So I understand the why behind the forceRenderOnLocaleChange option, only it leads to miss-understanding because it does not trigger a re-render, but a whole unmount / remount. This could lead to some issues or might be unwanted. (losing values in forms)

I suggest 2 actions:

  1. deprecate forceRenderOnLocaleChange and introduce a new forceRemountOnLocaleChange which take precedence of the 1st if used. This allow to be not breaking but starting the change. On new major version, forceRenderOnLocaleChange could be dropped and forceRemountOnLocaleChange be used
  2. find a way to use the lingui context when t and plural macros are used in React, so it will correctly re-render on locale change. A temporary solution is to do so
const {i18n} = useLingui()
const message = t(i18n)`Hello World`

@stale
Copy link

stale bot commented Dec 5, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Dec 5, 2021
@JSteunou
Copy link
Contributor

JSteunou commented Dec 9, 2021

plzzz stale bot :)

@stale stale bot removed the wontfix label Dec 9, 2021
@stale
Copy link

stale bot commented Feb 8, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Feb 8, 2022
@sakulstra
Copy link

Was just running into this as well.
I feel like having this behavior as a default is a weird choice.

probably just stupidity on my side:
Didn't realize the remount for quite some time and now have to refactor every use of t which is sad. Would have rather notices it earlier 😅

@stale stale bot removed the wontfix label Feb 11, 2022
@stale
Copy link

stale bot commented Apr 14, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Apr 14, 2022
@stale stale bot closed this as completed Apr 24, 2022
@sakulstra
Copy link

not stale

@athammer
Copy link

athammer commented Sep 22, 2022

A remount seems a bit heavy handed esp for larger apps and could lead to some really hard to debug bugs. I probably won't have time to contribute to this in a form of a PR but if we can create a bounty I'll contribute. I'm not sure we currently can due to the stale bot closing this though.

@timofei-iatsenko
Copy link
Collaborator

@JSteunou we are going to release v4 soon, and it seems it's a good time to fix this as well. Would you want to participate and prepare a PR? Or at least code-review if i implement what you mentioned in the comment above?

@andrii-bodnar andrii-bodnar linked a pull request Mar 13, 2023 that will close this issue
7 tasks
@zackcarlson
Copy link

I'm experiencing an issue where my entire app re-mounts for non-US-english translations. I've described my issue in depth here on StackOverflow. Please let me know if you have any guidance. Thanks so much!

@andrii-bodnar andrii-bodnar added this to the v4 milestone Mar 14, 2023
@JSteunou
Copy link
Contributor

@JSteunou we are going to release v4 soon, and it seems it's a good time to fix this as well. Would you want to participate and prepare a PR? Or at least code-review if i implement what you mentioned in the comment above?

I'm a bit late but kudos it works fine. The only issue now is that migration path is not that easy because of a limitation pointed out by @vonovak about the use of t macro and the absence of re-render.

@thekip you though about a new macro useLingui and I follow you. If really the t macro cannot be detected when it is in a react component, then having this hook from the macros would be a really nice feature. I could be then linked together with an ESLint rule to prevent use of t from macro when inside a React component. (I'm thinking DX & team work here)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants