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

useTranslation render multi-times and cause our app slow #1756

Open
arvinxx opened this issue May 28, 2024 · 15 comments
Open

useTranslation render multi-times and cause our app slow #1756

arvinxx opened this issue May 28, 2024 · 15 comments
Assignees

Comments

@arvinxx
Copy link

arvinxx commented May 28, 2024

🐛 Bug Report

Hi, I'm a member from LobeHub, we are using react-i18next to implement the i18n of LobeChat . And some of our users report the slow of input in our project.

After some dig, we find it re-renders due to the useTranslation's t. And there are some issues about it:

here is the demo of the t casued rerender (you can check it with this URL: https://lobe-chat-community-n4clmx832-lobehub.vercel.app):

rerender.mp4

my temporarily solution is to pin to [email protected] (PR: lobehub/lobe-chat#2691).

After the pin, we don't have re-render any more ( you can check it with this URL: https://lobe-chat-community-8a5yo72zq-lobehub.vercel.app )

no-rerender.mp4

Please fix this re-render issue , or it will caude critical performance issue with large projects

@jamuhl
Copy link
Member

jamuhl commented May 28, 2024

idk...wasn't part of all this changes...but if 14.0.2 works it seems the fix for that version done in #1720 does not fix the fix for everyone - as for some the first incorrect fix works...

plus I won't look at some samples that are more than a minimal reproducible sample...

@adrai
Copy link
Member

adrai commented May 29, 2024

//cc @timheilman @medihack

@timheilman
Copy link
Contributor

Thank you for CCing me on this issue. I'm still interested in keeping bug 1691 fixed without causing any other regressions, like the one seen here in 1756.

Unfortunately this issue, 1756, is not easily reproducible for me. I am not familiar with LobeChat. What made bug 1691 easily reproducible was the excellent research by @woodreamz that resulted in a minimal reproduction, as described in bug 1691.

Without a minimal reproduction, I cannot answer these questions for myself, so I will ask @arvinxx instead:

Does the performance issue occur with react-i18next pinned at version 14.0.4 ?
Does the performance issue occur with react-i18next pinned at version 14.0.5 ?
Does the performance issue occur with react-i18next pinned at version 14.0.7 ?
Does the performance issue occur with react-i18next pinned at version 14.0.8 ?

If #1720 is truly the cause of this bug 1756, then 14.0.4 will not have the performance issue, but 14.0.5 will have the performance issue.

I do not think #1720 is the source of this bug 1756, because #1720 should only have reduced the number of renders. However, the code is complicated and an unintentional effect is possible.

If #1731 is the source of this bug 1756, then 14.0.7 will not have the performance issue, but 14.0.8 will have the performance issue.

This seems more likely to me, but without a minimal reproduction of 1756, I cannot test it myself.

Verifying 1756 is fixed should also include that none of these bugs are reintroduced:

@arvinxx
Copy link
Author

arvinxx commented Jun 4, 2024

@timheilman I'm glad to help to check it. I have created some versions with different versions of react-i18next, the results are blow:

version Branch Test URL Result
14.0.4 reproduction/react-i18next-14.0.4 https://lobe-chat-community-4vqajrnas-lobehub.vercel.app ✅ occur
14.0.5 reproduction/react-i18next-14.0.5 https://lobe-chat-community-akwdjska4-lobehub.vercel.app ✅ occur
14.0.7 reproduction/react-i18next-14.0.7 https://lobe-chat-community-mrknejejm-lobehub.vercel.app ✅ occur
14.0.8 reproduction/react-i18next-14.0.8 https://lobe-chat-community-obiho0gw9-lobehub.vercel.app ✅ occur

Our core usage with react-i18next is here: https://github.com/lobehub/lobe-chat/blob/fa1409e85178bcfd4126ba9ca312fa51f7812b84/src/locales/create.ts#L16-L61

export const createI18nNext = (lang?: string) => {
  const instance = i18n
    .use(initReactI18next)
    .use(LanguageDetector)
    .use(
      resourcesToBackend(async (lng: string, ns: string) => {
        if (isDev && lng === 'zh-CN') return import(`./default/${ns}`);

        return import(`@/../locales/${normalizeLocale(lng)}/${ns}.json`);
      }),
    );
  // Dynamically set HTML direction on language change
  instance.on('languageChanged', (lng) => {
    if (typeof window !== 'undefined') {
      const direction = isRtlLang(lng) ? 'rtl' : 'ltr';
      document.documentElement.dir = direction;
    }
  });
  return {
    init: () =>
      instance.init({
        debug: debugMode,
        defaultNS: ['error', 'common', 'chat'],
        detection: {
          caches: ['cookie'],
          cookieMinutes: 60 * 24 * COOKIE_CACHE_DAYS,
          cookieOptions: {
            sameSite: 'lax',
          },
          lookupCookie: LOBE_LOCALE_COOKIE,
        },
        fallbackLng: DEFAULT_LANG,
        interpolation: {
          escapeValue: false,
        },
        lng: lang,
      }),
    instance,
  };
};

and here is the code how we init the i18n instance:

'use client';

import { ConfigProvider } from 'antd';
import { PropsWithChildren, memo, useEffect, useState } from 'react';
import { isRtlLang } from 'rtl-detect';

import { createI18nNext } from '@/locales/create';
import { isOnServerSide } from '@/utils/env';
import { getAntdLocale } from '@/utils/locale';

interface LocaleLayoutProps extends PropsWithChildren {
  antdLocale?: any;
  defaultLang?: string;
}

const Locale = memo<LocaleLayoutProps>(({ children, defaultLang, antdLocale }) => {
  const [i18n] = useState(createI18nNext(defaultLang));
  const [lang, setLang] = useState(defaultLang);
  const [locale, setLocale] = useState(antdLocale);

  // if run on server side, init i18n instance everytime
  if (isOnServerSide) {
    i18n.init();
  } else {
    // if on browser side, init i18n instance only once
    if (!i18n.instance.isInitialized)
      // console.debug('locale', lang);
      i18n.init().then(() => {
        // console.debug('inited.');
      });
  }

  // handle i18n instance language change
  useEffect(() => {
    const handleLang = async (lng: string) => {
      setLang(lng);

      if (lang === lng) return;

      const newLocale = await getAntdLocale(lng);
      setLocale(newLocale);
    };

    i18n.instance.on('languageChanged', handleLang);
    return () => {
      i18n.instance.off('languageChanged', handleLang);
    };
  }, [i18n, lang]);

  // detect document direction
  const documentDir = isRtlLang(lang!) ? 'rtl' : 'ltr';

  return (
    <ConfigProvider direction={documentDir} locale={locale}>
      {children}
    </ConfigProvider>
  );
});

Locale.displayName = 'Locale';

export default Locale;

@timheilman
Copy link
Contributor

timheilman commented Jun 9, 2024

I was confused about release versions, and did not ask for quite the right versions.

Could you, @arvinxx , test with version 14.0.1 ? My first involvement was with the change in 14.0.2.

Version 14.0.2 fixed #1691 but introduced #1718 .
Version 14.0.3 was a rollback of 14.0.2, and should be identical to 14.0.1
Version 14.0.4 was unrelated to these bugs
Version 14.0.5 fixed #1691 while keeping #1718 fixed, but may have introduced #1728
Versions 14.0.6 and 14.0.7 are unrelated
Version 14.0.8 fixed #1728 while keeping #1691 and #1718 fixed.

If version 14.0.1 has the performance issue, then your improved performance in version 14.0.2 comes at the cost of causing bug #1718. (I think lobeChat's issue 2724 might be the same bug #1718 here). In this case, this issue 1756 should be considered separate from #1691 , #1718 , and #1728 . It is an original performance issue needing its own investigation and fix.

If version 14.0.1 does not have the performance issue, then 1756 was somehow introduced by the fixes to #1691 , #1718, and #1728 , and is a regression based on those bugs' fixes.

@timheilman
Copy link
Contributor

Just following up. @arvinxx , do you believe lobeChat's issue 2724 is the same as #1718 here? Does version 14.0.1 have the performance issue?

@arvinxx
Copy link
Author

arvinxx commented Jun 12, 2024

@timheilman I will try , a little busy currently , wait for me.


14.0.1 also have performance issue. the branch: https://github.com/lobehub/lobe-chat/tree/reproduction/react-i18next-14.0.1 . the preivew URL: https://lobe-chat-community-h3par2u33-lobehub.vercel.app/


do you believe lobehub/lobe-chat#2724 is the same as #1718 here?

I think the 2724 issue is the same as #1718

@timheilman
Copy link
Contributor

timheilman commented Jun 29, 2024

Thank you @arvinxx !

Summary so far: #1756 (this issue) is unrelated to #1691 , #1718 , and #1728, and has been present since at least v14.0.1 and likely earlier.

v14.0.2 was a fix to #1691 that incidentally fixes #1756 (this issue) via PR #1716, but it causes #1718, and so was reverted in v14.0.3. Subsequent versions did not also incidentally fix #1756.

Still, the fact that PR #1716 incidentally fixes #1756 (this issue) will be a good hint at how to fix #1756 (this issue) while being careful to keep the other three bugs fixed.

#1756 (this issue) still will need a minimal reproduction to make progress. I see the videos in the original post, and they do show the rerenders which are a performance problem. One way to establish a minimal reproduction would be to make a very small project that just displays a text component and a button. The button should do something similar to what the slider is doing in the videos. The component should have a console.log message that is issued every time it renders. Clicking the button should cause this:

  • pinned at the latest version of i18next: the message appears on the console
  • pinned at v14.0.2: no message appears on the console

This type of minimal reproduction is similar to the one provided in #1691 .

@arvinxx
Copy link
Author

arvinxx commented Jul 1, 2024

I will try to give a minimal reproduction soon.

@hong-chan
Copy link

I face the same issue,
without StrictMode:

14.0.8 ~ 15.0.0
https://stackblitz.com/edit/vitejs-vite-ejjzng?file=src%2FApp.jsx,src%2Fmain.jsx,package.json&terminal=dev
"!!!!!! render App" logged 2 times after the first count button clicked

14.0.7
https://stackblitz.com/edit/vitejs-vite-kcdj8r?file=src%2FApp.jsx,src%2Fmain.jsx,package.json&terminal=dev
"!!!!!! render App" logged 1 time after the first count button clicked

with StrictMode, 14.0.7 will also log "!!!!!! render App", 4 (becoz of StrictMode) times after the first count button clicked, but I can bear with that as long as it is good in prod mode.

@runfaj
Copy link

runfaj commented Jul 31, 2024

I can't provide a reproducible example, but I'm also running into this (on all versions from 14.0 through 15.0). We have dynamic nodes rendering through react-flow. Each node is a component that renders between 10-15 translations via useTranslation. The more nodes we add, the more components have to render, thus the more translation instances appear. Given this useTranslation is a blocking action for react rendering, it causes everything to render slowly until everything loads.

Here's an example:
https://github.com/user-attachments/assets/ccfa5a37-89b1-4a65-9dfc-96008c50c924

You can see by the video length, this took about 6 seconds to fully render. I suspect on this view, roughly 100 translation strings loaded up on the view through various components.

Now, if I override the useTranslation just in these node components to just output the string instead of actually using the hook with this:

const useTranslation = () => ({ t: (str: string) => str });

Here's the difference.
https://github.com/user-attachments/assets/89fbc62d-67a6-4812-b989-53056ba92255

You can see this is like 1 second of rendering (or less) for the same components.

I should also note that in both these videos, this view actually appears in a popup where I have already loaded the english translation file for the namespaces that are involved in rendering this view via the parent. So there is no delay added due to network requests or imports since those json files have already loaded in the parent view before this popup has opened.

Unfortunately, this is going to be a big blocker for us soon. This example has 9 of these nodes, but production will potentially have up to 100 in the view. We've tried hoisting translations up at various component levels and attempted to preload in different ways. Looking through the hook itself (https://github.com/i18next/react-i18next/blob/master/src/useTranslation.js), it seems that maybe the caching/lookup for how the check on existing loaded namespaces/languages is really inefficient somewhere, but I'm not completely certain.

If it helps, here's additional info on how the json files are loaded:

// global i18n.ts file
const translationModules = mapKeys(
  import.meta.glob<boolean, string, object>('../../../node_modules/translations/**/*.json', { import: 'default' }),
  (_v, k) => k.replace('../../../node_modules/translations/dist/locales/', ''),
);

...

i18next
.use(resourcesToBackend(async (language: string, namespace: string) => {
    let result = {};
    try {
      result = await translationModules[`${language}/${namespace}.json`]() as object;
    } catch (e) {
      console.error('errored here', e);
    }
    return result;
  }))

@adrai
Copy link
Member

adrai commented Jul 31, 2024

@timheilman can you try to have a look at it?

@timheilman
Copy link
Contributor

@adrai I'm honored to be asked, but I've become busy with other professional and personal projects at the moment.

I appreciate the research that others have done on this issue, but the first step is going to be a minimal reproduction.

I do believe this performance issue is real as others have reported. I also believe it has been an issue from before I even knew this library existed. As explained above, v14.0.2 accidentally fixed this performance issue while trying to fix something else, but broke other things and was reverted. That may be a helpful data point, but the starting point is going to be a minimal reproduction.

See issue 1691 for what a minimal reproduction looks like.

@rjgotten
Copy link

rjgotten commented Sep 4, 2024

Your problems here are with reasonably high degree of likelihood in the fact that useTranslation is using a hodge-podge of refs, state and effects in attempts to manually bookkeep an initialization & update cycle that likely should instead be expressed with the useSyncExternalStore hook.

Rather than keep trying to poke at it with a stick / keep applying ever more bandaids to try and stop the haemorrhage, you could consider just taking the plunge and re-expressing the whole thing through that hook - which is the exact intended hook to use when patching in systems from the world outside of React and trying to observe them for changes.

@timheilman
Copy link
Contributor

Your problems here are with reasonably high degree of likelihood in the fact that useTranslation is using a hodge-podge of refs, state and effects in attempts to manually bookkeep an initialization & update cycle that likely should instead be expressed with the useSyncExternalStore hook.

Rather than keep trying to poke at it with a stick / keep applying ever more bandaids to try and stop the haemorrhage, you could consider just taking the plunge and re-expressing the whole thing through that hook - which is the exact intended hook to use when patching in systems from the world outside of React and trying to observe them for changes.

Colorful! I love the anthropomorphism of software.

I agree that there are many questionable architectural decisions in this package, and that the quality would be much improved by refactoring it, in the original sense as coined by Martin Fowler in his book of the same name: improving the design of existing code.

In my experience it is standard and normal that software I would rather not have to write myself, which has been created by others and generously licensed for free use (in this case under the permissive MIT license), has some serious design flaws and would benefit from refactoring. It's also standard that vast numbers of people feel similarly to the way I do and make use of permissively-licensed software with major design flaws. This package has over half a million downloads per week.

The state of the design of this package is perfectly normal! This is the real software world, as contrasted with some fantasy universe where what gets used is impeccably designed.

Because it is so widely used, there's great value in maintaining the existing behavior of the software, so the first step in improving the design of this existing code will be to get tests against the defects which improved design might remedy. Michael C. Feathers has some great ideas on how to work effectively with legacy code in his book, Working Effectively with Legacy Code. One way, @rjgotten, to work effectively with this legacy code, would be to provide a minimal reproduction of this performance issue, ideally in an automated test, so that the refactoring you recommend can be performed in a way that verifies it resolves the performance issue.

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

7 participants