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

Suspence is fired during lang change when useTranslation called in between #975

Closed
klis87 opened this issue Oct 23, 2019 · 30 comments
Closed

Comments

@klis87
Copy link

klis87 commented Oct 23, 2019

Describe the bug
I don't want Suspence to render fallback during language change, hence I use bindI18n: 'languageChanged'. It does not work properly though, as in practise Suspense still renders fallback, which causes app flickering on lang change. It happens when for example useTranslation is called during namespaces for new lang are loading due to other reason than lang change, like state update for instance.

Just imagine oficial example https://github.com/i18next/react-i18next/blob/master/example/react/src/App.js#L25

with additional hook const [x, incX] = useState(0); and

  const changeLanguage = lng => {
    incX(x + 1);
    i18n.changeLanguage(lng);
  };

Suspence will be triggered despite the bindI18n: 'languageChanged' option. It works fine without incrementing x though.

Occurs in react-i18next version
"react-i18next": "10.1.2+"

To Reproduce
Steps to reproduce the behavior:

  1. Update example as described.
  2. Change language, preferably set slow network in devtools before.
  3. See Suspence hit and loading text during i18n jsons are loading
  4. Loading indicator is hidden after i18n files are loaded

Expected behaviour
Suspence should not be hit in this case.

OS (please complete the following information):

  • Browser: Chrome latest
@klis87
Copy link
Author

klis87 commented Oct 23, 2019

I guess the assumptions was that useTranslation wont be recalled until https://github.com/i18next/react-i18next/blob/master/src/useTranslation.js#L68 is fired, and with bindI18n: 'languageChanged' it wouldnt be called until new language is loaded.

However it can be called simply due to other reasons, and then during namespace loading we end up with triggering suspence at https://github.com/i18next/react-i18next/blob/master/src/useTranslation.js#L91

I don't know what is the best way to fix that, but we need another condition just before throwing promise, like if (!ready && useSuspense && previousLangIsReady) return ret;

@jamuhl
Copy link
Member

jamuhl commented Oct 23, 2019

You mix things:

bindI18n -> tells when to force a rerender (default rerender always when language was changed)
useTranslation (with Suspense): do render fallback when not ready (that can be in any case not ready; not initialized, not loaded initial, not loaded while loading new language, ...)

In which cases does this happen...? https://github.com/i18next/react-i18next/blob/master/src/utils.js#L68

The check returns ready if at least fallback lng was loaded...in your case do you have no fallback or do a languageChange with additional load of new namespace?!?

@klis87
Copy link
Author

klis87 commented Oct 24, 2019

This is my i18n setting, I have lng and fallback lang both set, and this issue is not for initial language load, thats fine that we have spinning fallback rendered in this scenario. I just don't want to have spinning on language change. I just edited the official React example from this repository:

import i18n from 'i18next';
import Backend from 'i18next-xhr-backend';
import { initReactI18next } from 'react-i18next';

i18n
  .use(Backend)
  .use(initReactI18next)
  .init({
    fallbackLng: 'en',
    lng: 'en',
    debug: true,
    interpolation: {
      escapeValue: false,
    },
    react: {
      bindI18n: 'languageChanged', // i am changing this as I dont wanna any flickering on language change
    },
  });

I also updated all libs to latest versions.

Now, the default example implementation works fine, as in this video https://nimb.ws/OHdSfi

However, once I update this component in this way:

function Page() {
  const { t, i18n } = useTranslation();
  const [x, incX] = useState(0);

  const changeLanguage = lng => {
    incX(x + 1);
    i18n.changeLanguage(lng);
  };

  return (
    <div className="App">
      <div className="App-header">
        <img src={logo} className="App-logo" alt="logo" />
        <Welcome />
        <button onClick={() => changeLanguage('de')}>de</button>
        <button onClick={() => changeLanguage('en')}>en</button>
      </div>
      <div className="App-intro">
        <MyComponent />
      </div>
      <div>{t('description.part2')}</div>
    </div>
  );
}

the outome is flickering https://s.nimbusweb.me/share/3459712/7y4m64vr5lwr0s68eyt6
I really want to have behaviour from the 1st movie. I want to show currently loaded language i18n, until new one is ready.

@jamuhl
Copy link
Member

jamuhl commented Oct 24, 2019

why you set state inside changeLanguage...remove that should work

@jamuhl
Copy link
Member

jamuhl commented Oct 24, 2019

let me try to reproduce this - will have to see what's going on

@klis87
Copy link
Author

klis87 commented Oct 24, 2019

@jamuhl this is just an example showing the problem, it could also happen that Page component is rerendered because its parent component is rerendered due to some reason, it can be really anything. in my case i am using material ui and on lang picking i am closing dropdown with languages.

let me try to reproduce this - will have to see what's going on

thx, let me know if you have any problems in reproduction

@klis87
Copy link
Author

klis87 commented Oct 24, 2019

This bug also would happen even if I didnt close dropdown inside changeLang function. Imagine case:

  1. i18n.changeLanguage(lng)
  2. namespaces are being loaded
  3. user manually closes dropdown before namespaces for new lang are loaded
  4. suddenly user gets loading spinner
  5. when loading is finished, loading spinner disappears and user goes back to previous view, but with dropdown closed

Generally this bug is happening when:

  1. lang is changed and a new one is activated for the 1st time
  2. sth rerenders component with useTranslation hook before namespaces are loaded

Looking at the code this is not suprising, but I guess that surprise is that ready is false in this case according to what you wrote

@jamuhl
Copy link
Member

jamuhl commented Oct 24, 2019

ok...this is correct -> ready is correctly false as that namespace is not loaded in the new language -> binding languageChanging per default shows some people like to trigger suspense while changing the language....

but I also see your wish to not trigger it on language change...

the problem lies in the check for ready -> i18next sets i18next.language and load resources after setting so (this was requested as some users rely on reading out i18next.language immediately). Some code there also depends on that language being set.

Handling the previousLanguage check on react-i18next seems overly complicated - so what I might try now is adding a mode to i18next that allows that i18next.language gets set after loading those translations -> which would result in a rerender triggered outside of our code to be still checking on the old language set

@klis87
Copy link
Author

klis87 commented Oct 24, 2019

I agree that handling this in React would be complicated, it potentially would require keeping some additional state inside I18nProvider itself.

Personally I would consider just to add another field to i18n without changing current behaviour of setting language timing. I would just add previousLanguage field.

Then, we could have another check inside useTranslation like isPreviousLanguageReady.

Potentially we could have another React option, so some people would like to retain current behaviour, and others would like to show previous language content until current is being loaded.

@jamuhl
Copy link
Member

jamuhl commented Oct 24, 2019

Currently, I got the solution up...setting i18next.language after loaded...so this runs your use case well - which is what I would prefer too in my app - but loosing the ability we had in the past to trigger a Suspense during languageChanging event...

Guess I need to add an attribute to i18next isLanguageChanging and if languageChanging is bound on react-i18next bind-i18n we will trigger a not ready if that flag is true.

Hm...looks like this will come out with major versions on i18next and react-i18next

@klis87
Copy link
Author

klis87 commented Oct 24, 2019

Good you figured it out so quickly, thanks!

I have 2 use cases I would like to ask though, generally about loading new namespaces (those which werent loaded for any language yet. Please confirm it will work like I described.

With those adjustment, what would happen in the following case:

  1. we load the page with en lang and translation namespace
  2. we load a component with other namespace, I guess useTranslation('other') would trigger suspence here? Then we could wrap only this component within a dedicated Suspence to have loader only for this specific part of the app

And also in this case:

  1. we load the page with en lang and translation namespace
  2. we change language to de, suspence is not triggered
  3. we load a component with other namespace, I guess useTranslation('other') would trigger suspence here? Then we could wrap only this component within dedicated Suspence to have loader only for this specific part of the app

@jamuhl
Copy link
Member

jamuhl commented Oct 24, 2019

first case: yes, Suspense will be triggered
second case: Suspense will not be triggered for translation but other

@jamuhl
Copy link
Member

jamuhl commented Oct 24, 2019

Can you try with:

react-i18next@11.0.0
i18next@18.0.0

@klis87
Copy link
Author

klis87 commented Oct 25, 2019

It works now on example repository, thanks! I will try to update the whole app to be sure now, it also has multiple namespaces plus lazy loaded components.

Nice idea with adding isLanguageChangingTo. You could use it to compute things like isLanguageChanging = language !== isLanguageChangingTo, or to kind of optimistically update language dropdown to new language before it is loaded

@jamuhl
Copy link
Member

jamuhl commented Oct 25, 2019

Great to hear...please report any issues you encounter...👍

@klis87
Copy link
Author

klis87 commented Oct 25, 2019

This works so great now, I used 1 suspence like in example to catch common translations which is only triggered on load, plus extra suspenses for components which can have additional namespaces. No flickering at all, it should boost performance very much as I had many useless rerenders (I mitigated this issue before by fallback unfound keys i18n to empty string :)

The only thing which stopped working is my AdblockDetector for some reason ; o

const AdblockWarning = () => {
  const addWrapper = useRef(null);
  const dispatch = useDispatch();

  useEffect(() => {
    if (addWrapper.current && addWrapper.current.clientHeight === 0) {
      dispatch(addMessage('addBlockDetected', null, null, 20000));
    }
  }, []);

  return (
    <div ref={addWrapper}>
      <div className="adBanner" style={{ height: 1 }} />
    </div>
  );
};

But I dont think I can blame this library for it :)

Probably this is Suspence affecting this component. Anyway, I am closing as really I cannot find anything wrong now, I dont have any warning for missing i18n either. I will reopen if I find anything new.

@klis87 klis87 closed this as completed Oct 25, 2019
@jamuhl
Copy link
Member

jamuhl commented Oct 25, 2019

Awesome...and:

If you like this module don’t forget to star this repo. Make a tweet, share the word or have a look at our https://locize.com to support the devs of this project -> there are many ways to help this project 🙏

@klis87
Copy link
Author

klis87 commented Oct 25, 2019

I starred this and many i18n addons already, you deserve it! Keep doing great job :) Btw, I am having some ideas regarding boosting performance, like creating some plugin for generating unique translation names based on content so we could use browser caching with infinitive TTL. Or maybe a plugin to allow collocating translation files with components. I cannot wait to contribute, will do after I finish my opensource library.

@klis87
Copy link
Author

klis87 commented Jan 30, 2020

@jamuhl Recently I added SSR to one of my app and it turned out that Suspense again is triggered on lang change. It works without SSR, but once this is added, then there are 2 strange behaviours I noticed:

  1. Suspense is triggered on lang change even with only common namespace
  2. If I have a route with 2 namespaces loaded, then on lang change 1st 1 namespace is loaded, and after that the common one, while with client side only rendering it was simultaneous, so for example 2 namespaces for new language were loaded at the same time

If that is not enough information I could provide some demo, but generally adding SSR should reproduce this.

If you prefer to raise this as a separate issue, let me know too.

And, which might be helpful:

// client
  i18next
    .use(initReactI18next)
    .use(i18nextXHRBackend)
    .init({
      fallbackLng: false,
      lng: lang,
      defaultNS: 'common',
      ns: [],
      debug: process.env.NODE_ENV !== 'production',
      load: 'languageOnly',
      backend: {
        loadPath: '/static/{{lng}}/{{ns}}.json',
      },
      interpolation: {
        escapeValue: false,
      },
      react: {
        bindStore: '',
      },
    });

// server
  i18next
    .use(backend)
    .use(LanguageDetector)
    .init({
      whitelist: ['en', 'de'],
      fallbackLng: false,
      preload: ['en', 'de'],
      debug: process.env.NODE_ENV !== 'production',
      load: 'languageOnly',
      ns: ['common', 'anotherNamespace'],
      defaultNS: 'common',
      backend: { loadPath: 'src/locales/{{lng}}/{{ns}}.json' },
      detection: {
        order: ['path'],
      },
      react: {
        bindStore: '',
      },
    });

@klis87 klis87 reopened this Jan 30, 2020
@jamuhl
Copy link
Member

jamuhl commented Jan 30, 2020

Hard to say...SSR works but is "hard" to get right...you might have a look at next-i18next for inspiration...

do you pass down initialLanguage, initialStore? https://react.i18next.com/latest/ssr

@klis87
Copy link
Author

klis87 commented Jan 30, 2020

yes, everything works apart from this Suspense triggering on lang change. So I guess I should prepare minimal reproducing repo right?

@jamuhl
Copy link
Member

jamuhl commented Jan 30, 2020

Would help...just currently do not expect a fast resolution as I will be offline more or less the next two weeks (starting on monday).

@klis87
Copy link
Author

klis87 commented Jan 30, 2020

Ok, no problem, thx for the info. I will prepare the demo in the meantime and try to see what's going on

@klis87
Copy link
Author

klis87 commented Feb 17, 2020

@jamuhl Here you can see minimal example, for now showing problem even with 1 namespace, instructions in readme -> https://github.com/klis87/react-i18n-ssr-example

Probably there is sth wrong with useSSR, as without it and SSR with the same client configuration suspence is not triggered on language change

@klis87
Copy link
Author

klis87 commented Feb 17, 2020

I will dig in the code, maybe I will find a potential culprit

@jamuhl
Copy link
Member

jamuhl commented Feb 17, 2020

thank you for providing the sample - will dig into it tomorrow - already late here now.

@klis87
Copy link
Author

klis87 commented Feb 17, 2020

Indeed it is quite late, so I am finishing for today too. At first glance I cannot see anything, but logically the following must be the issue:

https://github.com/i18next/react-i18next/blob/master/src/useTranslation.js#L41 for some reason must be false, so probably hasLoadedNamespace must return false for some reason, maybe 90f0e44#diff-2b4ca49d4bb0a774c4d4c1672d7aa781R61 doesnt take ssr case ino account

@jamuhl
Copy link
Member

jamuhl commented Feb 18, 2020

Can you retry with react-i18next@11.3.3

The languageChanged event was triggered too early as there was no namespace to load on languageChange -> there was a little mistake when copying namespaces over: 0828bfa#diff-3f70de7c1ba388c739ba6409eaaca576L1012

now with this change on languageChange loading should happen and the event for languageChanged will be triggered after having loaded the needed translations -> no Suspence triggered as hasLoadedNamespace will return true now

@klis87
Copy link
Author

klis87 commented Feb 18, 2020

@jamuhl looks like it works, even with multiple namespaces, thx!

@klis87 klis87 closed this as completed Feb 18, 2020
@jamuhl
Copy link
Member

jamuhl commented Feb 18, 2020

If you like this module don’t forget to star this repo. Make a tweet, share the word or have a look at our https://locize.com to support the devs of this project.

If you liked my support / work - I would enjoy a coffee sponsored using the “:purple_heart:Sponsor Button”.

There are many ways to help this project 🙏

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

No branches or pull requests

2 participants