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 · 18 comments
Closed

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

klis87 opened this issue Oct 23, 2019 · 18 comments

Comments

@klis87
Copy link

@klis87 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

This comment has been minimized.

Copy link
Author

@klis87 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

This comment has been minimized.

Copy link
Member

@jamuhl 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

This comment has been minimized.

Copy link
Author

@klis87 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

This comment has been minimized.

Copy link
Member

@jamuhl jamuhl commented Oct 24, 2019

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

@jamuhl

This comment has been minimized.

Copy link
Member

@jamuhl jamuhl commented Oct 24, 2019

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

@klis87

This comment has been minimized.

Copy link
Author

@klis87 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

This comment has been minimized.

Copy link
Author

@klis87 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

This comment has been minimized.

Copy link
Member

@jamuhl 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

This comment has been minimized.

Copy link
Author

@klis87 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

This comment has been minimized.

Copy link
Member

@jamuhl 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

This comment has been minimized.

Copy link
Author

@klis87 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

This comment has been minimized.

Copy link
Member

@jamuhl jamuhl commented Oct 24, 2019

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

@jamuhl

This comment has been minimized.

Copy link
Member

@jamuhl jamuhl commented Oct 24, 2019

Can you try with:

react-i18next@11.0.0
i18next@18.0.0

@klis87

This comment has been minimized.

Copy link
Author

@klis87 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

This comment has been minimized.

Copy link
Member

@jamuhl jamuhl commented Oct 25, 2019

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

@klis87

This comment has been minimized.

Copy link
Author

@klis87 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 Oct 25, 2019
@jamuhl

This comment has been minimized.

Copy link
Member

@jamuhl 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

This comment has been minimized.

Copy link
Author

@klis87 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.