kalinchernev / using-i18n

Using https://github.com/gatsbyjs/gatsby/tree/master/examples/using-i18n
MIT License
24 stars 4 forks source link

useTranslation does not update on page change when used inside a page template #1

Open deanc opened 4 years ago

deanc commented 4 years ago

Steps to reproduce:

  1. Clone this repo
  2. Remove the Welcome component from the layouts/index.js file
  3. Add the Welcome component to pages/index.js file.

Switch language using the language switcher. The welcome text translation does not change on first click. It requires a full page reload.

kalinchernev commented 4 years ago

Hi @deanc of course, this is the key https://github.com/kalinchernev/using-i18n/blob/master/src/layouts/index.js#L22

The HOC is loading translations. I haven't touched src/pages/index.js in comparison with original code and could be it's not using this layout. In fact, this file is showing another approach to translations: extracting strings via graphql query, not through i18n resource loading.

deanc commented 4 years ago

@kalinchernev thanks for the quick reply.

In fact, pages/index.js should be using the layout by default as you are using the gatsby-plugin-layout plugin.

I am specifically not using the loading of translations from files (with useTranslations) for now. I am using default react-i18next which loads the translations from resources.json. The only place in this repo that is used is in the Welcome component :)

I think it's just a case that the child component doesn't update based on props changing. Which is strange because pageContext.locale clearly is changing.

If I change changeLanguage in the HOC to this, it unsurprisingly forces a re-render:

 changeLanguage = () => {
      const { pageContext } = this.props

      this.addResources(pageContext)
      this.i18n.changeLanguage(pageContext.locale)
      this.forceUpdate()
    }

I'm not sure this is the ideal scenario though.

kalinchernev commented 4 years ago

An idea without having idea about actual code

Try to extract location data from page parameters and then instead of forcing update in the higher order do it in the component itself

import { useTranslation } from 'react-i18next';

...

const { i18n } = useTranslation();

....
  useEffect(() => {
    i18n.changeLanguage(locale);
  }, [location]);
deanc commented 4 years ago

Hi @kalinchernev - I tested this using your code. So it's a fresh checkout of your repo with the steps provided in the original issue applied. Basically any component that uses useTranslation that is not in the layout file, will not be translated without a page reload or second click on the route.

My concern with having to put the code you suggested to each individual component, is it kinda defeats the purpose of having the HOC take care of everything. As a developer, the experience of having to remember to use useEffect alongside useTranslation for every component that uses the t function is not ideal.

kalinchernev commented 4 years ago

As I said, no code => hard to tell ...

Here's a quick test I did: https://github.com/kalinchernev/using-i18n/commit/7e34cc5891191eaf36fe0f10d0659e1f914e5833#diff-32906244faafabcd47e32d8505ec3b33

not_found

Not sure what's on your side ... could be that the language switcher is losing path or something.

At any case: the repo is not to give you the definite approach, but to show a possible one, it's up to you to decide what works best for you ....

deanc commented 4 years ago

@kalinchernev I made a branch here in a fork to show you what the problem is: https://github.com/deanc/using-i18n/tree/bug

Note I only changed these few lines: https://github.com/deanc/using-i18n/commit/59cc5f2fe770b82fbd8f14def2c5e9b3eae2692a

You should be able to check that branch out, run yarn install and yarn start and then just click "DE" and you'll see it doesn't translate the welcome message.

kalinchernev commented 4 years ago

Suggestion "which worked on my computer"

import React, { useEffect } from "react";
import { useTranslation } from "react-i18next";

import "../global.css";

import Navigation from "../components/navigation";

import withI18next from "../i18n/withI18next";

const Layout = ({ children, location, pageContext: { locale } }) => {
  const { i18n } = useTranslation();

  useEffect(() => {
    i18n.changeLanguage(locale);
  }, [location]);

  return (
    <div className="global-wrapper">
      <header className="global-header">
        <Navigation />
      </header>
      <main>{children}</main>
    </div>
  );
};

export default withI18next()(Layout);

Same idea as before, but "binding" the language change on the location which comes from the layout.

deanc commented 4 years ago

That seems to have done the trick @kalinchernev . Thanks!

Should I submit a PR to the repo with this or would you like to take care of it?

kalinchernev commented 4 years ago

@deanc yeah, sure, if it works, please make a PR, although I plan to leave the issue open to be honest. I liked the way you addressed a specific question which we discussed and the outcome can be used someone else.

In the meantime, the repo is not meant to be a starter or something go-to, rather an experiment, just as you did try a specific use case.

Because it was started off from an example from the main mono repo of gatsby, and it still has other approaches of i18n, i didn't really want to rework it so much.

At any case, a tweak like this to have the i18n logic only in the layout will be good addition in general I believe.