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

Inform useTranslation hook of replaced i18n instance #1272

Closed
medihack opened this issue Mar 4, 2021 · 15 comments
Closed

Inform useTranslation hook of replaced i18n instance #1272

medihack opened this issue Mar 4, 2021 · 15 comments
Labels

Comments

@medihack
Copy link
Contributor

medihack commented Mar 4, 2021

🐛 Bug Report

I use the I18nextProvider to distribute the i18n instance in a Blitz.js app with SSR. When switching the language I simply replace the i18n instance in the provider (this is also how next-i18next works). This works when recreating every component, but breaks when just rerendering every component. It is because t of useTranslation is cached.
This also breaks next-i18next when using shallow routing. And it is not only a problem with shallow routing, but also with persistent layouts as those components in the layout (like app bars) are also not translated correctly after a page change (and replacing the i18n instance).

To Reproduce

Replace the i18n instance of a I18nextProvider and just force a rerender of all components that use the useTranslation hook. I also demonstrated this with a little code change in the example app of next-i18next (https://github.com/medihack/next-i18next).

Expected behavior

The useTranslation hook should somehow check if its provided i18n instance was replaced and not just provide a cached t function.

Your Environment

  • runtime version: node v15.5
  • i18next version: 11.8.8
  • os: Windows WSL
@jamuhl
Copy link
Member

jamuhl commented Mar 5, 2021

Not sure who came up with that idea of replacing the instance in the i18nProvider -> correct would be a call to i18n.changeLanguage which would replace the t function of useTranslation correctly.

Changing the instance looks like the badest idea regarding changing the language -> the instance is the base of everything -> all the events are bound to it: https://github.com/i18next/react-i18next/blob/master/src/useTranslation.js#L65

So, replacing the instance and just shallow rerender breaks basically everything...

@adrai
Copy link
Member

adrai commented Mar 5, 2021

Not sure who came up with that idea of replacing the instance in the i18nProvider -> correct would be a call to i18n.changeLanguage which would replace the t function of useTranslation correctly.

Changing the instance looks like the badest idea regarding changing the language -> the instance is the base of everything -> all the events are bound to it: https://github.com/i18next/react-i18next/blob/master/src/useTranslation.js#L65

So, replacing the instance and just shallow rerender breaks basically everything...

@isaachinman this may interest you

@isaachinman
Copy link
Contributor

In next-i18next@8, the assumption is that pages will be SSG/SSR/ISR. This means that we only ever have a single locale, and render out a I18nextProvider with an i18next instance based on that locale. If the user changes language, by the NextJs v10 spec, they are navigating to a new page, which has in turn been SSG/SSR/ISR with a separate i18next instance.

This pattern works fine without shallow routing, but breaks with shallow routing. At the moment I don't have any clever ideas about how to reconcile this.

@medihack
Copy link
Contributor Author

medihack commented Mar 5, 2021

Ok, I see. How do you recommend updating the i18n store when routing to another page? In the case of Next.js I get the language data preloaded from getServerSideProps. Should I just use addResourceBundle to update the store then with this data? A little problem I see that on a very large site the store this way always increases when switching pages and languages, but never gets reset (but I guess this won't ever be a problem).

On the other side, I wonder why it is not possible to check in the useTranslation hook if the i18n instance has changed. And if this is the case, then just reinitialize the t function (but maybe I am overlooking something here).

@isaachinman
Copy link
Contributor

If, in shallow routing, the I18nextProvider is passed a new i18next instance, can it not perform some sort of deep merge?

@medihack
Copy link
Contributor Author

medihack commented Mar 5, 2021

@isaachinman When switching the language and just stay on the same page I would expect the state of the page not to get lost. This is why I choose shallow routing in this scenario.

@jamuhl
Copy link
Member

jamuhl commented Mar 5, 2021

@medihack what you might try is adding the i18n instance https://github.com/i18next/react-i18next/blob/master/src/useTranslation.js#L9 to the array of side-effects https://github.com/i18next/react-i18next/blob/master/src/useTranslation.js#L75 so a new i18n instance should rerun the bindings...

@medihack
Copy link
Contributor Author

medihack commented Mar 5, 2021

@jamuhl Do you mean by wrapping the existing useTranslation hook somehow or by writing my own custom "useTranslation" hook?

@jamuhl
Copy link
Member

jamuhl commented Mar 5, 2021

No, changing that in your node_modules folder and test -> if it solves your issue provide a PR with the needed changes

@medihack
Copy link
Contributor Author

medihack commented Mar 5, 2021

Ok, I'll give it a try and report back.

@kachkaev
Copy link
Contributor

kachkaev commented Mar 5, 2021

I guess this issue is exactly what I’ve been trying to investigate this Friday night 😅

I have a project with two locales (en-GB and en-US). Despite that I was not doing anything crazy during the setup, date formats did not update in React components that were mounted during the locale switchover. Interestingly, if I unmounted those problematic components and mounted them back, the dates were updated.

Here’s the missing piece, which has fixed everything for me locally!

React.useEffect(() => {
  setT(getT())
}, [i18n]);

I added it after const [t, setT] = useState(getT()) in my node_modules copy of src/useTranslation. All worked as expected after that 🎉

Thanks for the clue @jamuhl! I hope that my experiment will help us with fixing this issue 🙌

@medihack
Copy link
Contributor Author

medihack commented Mar 5, 2021

Yes, I came to the same result of how to fix it (see below). Just about to write a test and then make a PR.

  const isInitial = useRef(true)
  useEffect(() => {
    if (isMounted.current && !isInitial.current) {
      setT(getT())
    }
    isInitial.current = false
  }, [i18n])

@medihack
Copy link
Contributor Author

medihack commented Mar 5, 2021

One little thing to add. It won't fix the shallow routing problem in next-i18next completely (but it makes it at least possible). The code there has another little problem, but I will discuss it over there.

medihack added a commit to medihack/react-i18next that referenced this issue Mar 6, 2021
medihack added a commit to medihack/react-i18next that referenced this issue Mar 6, 2021
@isaachinman
Copy link
Contributor

Thanks for looking into this @medihack and @kachkaev – happy to discuss any improvements or bug fixes in the next-i18next repo.

@stale
Copy link

stale bot commented Mar 14, 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.

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

No branches or pull requests

5 participants