i18next / react-i18next

Internationalization for react done right. Using the i18next i18n ecosystem.
https://react.i18next.com
MIT License
9.21k stars 1.02k forks source link

useTranslation breaking "Rules of Hooks" #735

Closed NeoLegends closed 5 years ago

NeoLegends commented 5 years ago

Describe the bug We're using the new useTranslation hook in our components. However, during development React complains that some hooks have been rendered conditionally and that the amount of hooks in a component has changed. When switching back to the Translation HOCs the error disappeared, so I assume it's because of this line here:

https://github.com/i18next/react-i18next/blob/ece439751c09b0cbe8170f07bfe79617ce589cb8/src/useTranslation.js#L14

Conditionally rendering hooks break the "Rules of Hooks", so this should be refactored to always render all hooks.

Our component does not render any hooks conditionally, I have checked thrice.

Occurs in react-i18next version 10.0.5

Expected behaviour No error to occur and no hooks rendering conditionally.

Screenshots image

OS (please complete the following information):

jamuhl commented 5 years ago

The condition https://github.com/i18next/react-i18next/blob/master/src/context.js#L17

only set for this is here: https://github.com/i18next/react-i18next/blob/master/src/I18nextProvider.js#L5

so either you used the Provider / or not -> so i'm rather sure must be something else

NeoLegends commented 5 years ago

Yeah so there is one provider wrapping the entire app. Is this wrong / deprecated?

jamuhl commented 5 years ago

It's not needed in cases having only one instance of i18next as i18next.use(initReactI18next) is sufficient for that case but also not wrong.

So that Provider sets the flag to true -> so useContext is triggered every render.

Just tested it on: https://github.com/i18next/react-i18next/tree/master/example/react

Removing the i18next.use(initReactI18next) from i18n.js and added the I18nextProvider to index.js and did not get that warning at all...

NeoLegends commented 5 years ago

AAAAAGH. Found it!

It never had anything to do with the provider or the .use(...).

The actual reason is: we're using the useTranslation-hook twice / not as the last hook in the "hook stack" of our component.

However, as you know, the hook sometimes throws a promise (when the translations haven't loaded yet), causing the subsequent hooks (in our component) to not be rendered. When it doesn't throw on subsequent invocations, the hooks after the useTranslation-hook will be rendered causing the issue.

Please remove this behavior. This will cause more interference in the future, when more people start using the hook with lazy-loaded translations.

NeoLegends commented 5 years ago

So this is the culprit: https://github.com/i18next/react-i18next/blob/ece439751c09b0cbe8170f07bfe79617ce589cb8/src/useTranslation.js#L65

This should probably be done using useEffect with [] as its second parameter?

NeoLegends commented 5 years ago

Just saw (in the docs) that the intended goal of this is to trigger suspense, but I'm sure the current way of doing this isn't "stable" yet. And it does cause real issues as you can see ;) At least I found nothing about throwing promises from your render functions inside the official react documentation, just in some talks about what might be there in the future.

jamuhl commented 5 years ago

So what is your suggestion? Loading translations in an useEffect and let the view render meanwhile with invalid content letting t function return the key?

How big is the chance suspense will change it's current behaviour in future and we can't adapt that behaviour?

We could add a suspense free mode which returns a third param ready you can handle yourself in each component using useTranslation? What do you think?

NeoLegends commented 5 years ago

How big is the chance suspense will change it's current behaviour in future and we can't adapt that behaviour?

I guess not very big, but the current way of doing things is also not the right way, I think (especially since none of the current behavior is actually documented & stable). I wonder what the react people think about this use case. Triggering suspense from a hook seems like an edge case that should be properly defined in the future.

We could add a suspense free mode which returns a third param ready you can handle yourself in each component using useTranslation? What do you think?

I like this better than the first option. It leaves the user the option to react accordingly, and removes the "quirky" behavior.

jamuhl commented 5 years ago

published in react-i18next@10.1.0

you can set:

i18next.init({
  // ...
  react: {
    useSuspense: false
  }
});

// in components
const { t, i18n, ready } = useTranslation();
NeoLegends commented 5 years ago

Oh wow, that's quick!

Notice I'll open an issue on the react repo about using hooks with suspense. I'll post the link here when the issue is there :)

jamuhl commented 5 years ago

just in case you're using the withTranslation HOC i forgot that -> will be supported in v10.1.1 and the ready prop will be named tReady

NeoLegends commented 5 years ago

I'm using the array syntax anyway, so the name doesn't really bother me :) but thanks for the heads-up anyway!

jamuhl commented 5 years ago

Only in the HOC hooks will be ready as is

NeoLegends commented 5 years ago

React issue about triggering suspense from hooks: https://github.com/facebook/react/issues/14848

jamuhl commented 5 years ago

Just tried adding const [x, setX] = useState({ x: 'foo' }); here after the useTranslation: https://github.com/i18next/react-i18next/blob/master/example/react/src/App.js#L27

not getting any warning...if i understand right Suspense does render fallback and rerender tree starting from itself again on resolve...so there should be no issue...or at least i can't reproduce it.

NeoLegends commented 5 years ago

Does the example lazy-load the translations?

jamuhl commented 5 years ago

yes using the XHR-backend -> triggers the Suspense

NeoLegends commented 5 years ago

Hmm and react isn't throwing any warnings in the console?

I mean there never was any displaying issue, just the warning.

jamuhl commented 5 years ago

no warning in the sample...but might be as it loads very fast...or idk...

jamuhl commented 5 years ago

reading https://github.com/facebook/react/issues/14563 again i see no issue at all -> the only issue could be you do heavy computation without useMemo meaning there is a chance of doing that more than once -> but same would be on any rerender were you do not use a cache mechanism i guess.

NeoLegends commented 5 years ago

Seems like https://github.com/facebook/react/issues/14848#issuecomment-463660005 just confirmed the warning is fired erroneosly. I think you could drop the isReady-behavior again, if you want to, if that's still possible.

jamuhl commented 5 years ago

would be possible...but guess it does not hurt - might be there are other cases where users prefer not using suspense yet (existing codebase)

mekhamata commented 2 years ago

Hello, so what is the solution? none of options above worked for me. this is the problem: https://stackoverflow.com/questions/70646847/usetranslation-not-working-with-useeffect-rendered-more-hooks-than-during-the-p

jamuhl commented 2 years ago

@mekhamata there is no such issue in current version I'm aware of - and this module is used widely...please provide a reproducible example on codesandbox

mekhamata commented 2 years ago

I don't know how to use code sandbox because I am new.

I updated my question and added the navigation part. can you check again and see if can help me please?

mekhamata commented 2 years ago

I have version 21.6.5

ymoran00 commented 2 years ago

useTranslation still breaks hooks - in a different way. in useTranslation.js ln. 11, if (!i18n) returns and ends the function. Then the useState after that isn't called. Why not initialize all the useX functions before that if clause?

jamuhl commented 2 years ago

@ymoran00 because you still have to fix your code - personally I prefer getting some issue then wondering "sometimes it works some time it doesn't" - but @adrai his pullrequest will address this...giving you no warning...but also won't fix your error in the code

Chatner-Dev commented 2 years ago

hi @jamuhl This issue still breaks my code, even after I do everything like your sandbox here: https://codesandbox.io/s/react-i18next-forked-s2z0pr?file=/src/i18n.js

Reading through your discussion, I still cant seem to find where the problem is. Anything else i might try?

adrai commented 2 years ago

hi @jamuhl This issue still breaks my code, even after I do everything like your sandbox here: https://codesandbox.io/s/react-i18next-forked-s2z0pr?file=/src/i18n.js

Reading through your discussion, I still cant seem to find where the problem is. Anything else i might try?

@Chatner-Dev provide a reproducible example and make sure you pass i18next.use(initReactI18next) or call setI18n(i18next)

mehraj43 commented 1 year ago

when using useTranslate hook On conditional rendering im getting render error stating: Rendered more hooks than during the previous render

jamuhl commented 1 year ago

@mehraj43 https://github.com/i18next/react-i18next/blob/master/src/useTranslation.js#L19 -> do what @adrai wrote just above your comment...

the only reason it can get that conditional rendering is when using useTranslation hook or withTranslation HOC without setting i18next instance to be used using i18next.use(initReactI18next) or call setI18n(i18next)

if that is not the problem...which I would wonder....please provide a sample to reproduce this

mehraj43 commented 1 year ago

image

mehraj43 commented 1 year ago

image

mehraj43 commented 1 year ago

in the second image i am showing scrollview on basis of condition like for 3 sec im showing skeleton loading on some state and after state becomes false the scrollview content should be shown and that's when error is thrown

mehraj43 commented 1 year ago

and how do i set i18next instance to be used using i18next.use(initReactI18next) or call setI18n(i18next)

adrai commented 1 year ago

Where is your i18next instance initialized? Does it look like this example? https://github.com/i18next/react-i18next/blob/master/example/react-native/App.js#L14

Please provide a minimal reproducible example, not just screenshots or code snippets.

mehraj43 commented 1 year ago

image

mehraj43 commented 1 year ago

see i have initialised it already even then its giving me error

adrai commented 1 year ago

Please provide a minimal reproducible example. Best would be an example based on expo for web: https://snack.expo.dev/@jonsamp/hello-expo