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

Navigating between pages with or without translations causes _app remounts #1075

Closed
enzoferey opened this issue Mar 17, 2021 · 24 comments
Closed

Comments

@enzoferey
Copy link

Describe the bug

When navigating from a static page that is using serverSideTranslations to a page that is not using it (or the other way around), the _app remounts. This leads to unexpected behaviour as the _app is generally used to setup context providers or run one time browser-only initializations.

Occurs in next-i18next version

Version: ^8.1.2

Steps to reproduce

  1. Clone https://github.com/enzoferey/next-i18n-app-unmount.
  2. Start the app yarn dev.
  3. Open the console in your browser, you will see logs on _app mount and unmount.
  4. Navigate from home page via the links in the page.

Expected behaviour

You will see that navigating from the home page (no serverSideTranslations usage) to the other pages (serverSideTranslations usage) the _app remounts, however when navigating between the signup and about pages (both use serverSideTranslations), the _app doesn't remount as expected.

Screenshots

bug-remount

OS (please complete the following information)

  • Device: MacBook Pro (13-inch, 2017, Two Thunderbolt 3 ports)
  • Browser: Version 89.0.4389.90 (Official Build) (x86_64)

Notes

I'm quite sure there is something wrong around the appWithTranslation HOC. I do see that we are rendering different things based on locale existence and that could cause a whole remount, but I don't know Nextjs i18n routing internal props well enough to hint for a solution. It might actually be an expected behaviour on your side by design of the solution, I don't know.

@isaachinman
Copy link
Contributor

I believe this is fixed by #1064@skrivanos can you confirm?

@skrivanos
Copy link
Contributor

@isaachinman My PR won't fix this. I believe it's caused by this snippet:

https://github.com/isaachinman/next-i18next/blob/149b592b41c2286aaf04bf5b3a093023ac0722bd/src/appWithTranslation.tsx#L60-L74

It creates a different instance of the WrappedComponent (i.e. app) depending on if there's an i18n instance or not, and there won't be an instance if serverSideTranslations are not used. #1049 is related and would probably fix this.

Also related: the key is set to the locale, making the app also unmount and mount on locale changes. Is there a good reason for setting the key?

@enzoferey
Copy link
Author

enzoferey commented Mar 18, 2021

Thanks for pointing @skrivanos. The conditional rendering and the locale key are definitely the two elements that can force the _app to rerender.

I have patched the package on my local machine to test and removing the conditional rendering does not fix the issue. So I tried removing the locale key. Not a winner neither. However, then I tried removing both (as the i18n existence is tied to the _nextI18Next existence in the page props), and it did work as expected.

I was surprised because the example repo I linked in the description does not do any locale switch at any point, so I added a few logs around to find out what's up. It turns out that when navigating to the home page (the one that is not using serverSideTranslations), locale will be null when rendered. Meanwhile when rendering the other pages, it will be set to en as expected (default locale).

I tried then adding some links that would switch the locale to de to see if after removing both this things it would still do the locale switch properly. And it does !

Now, I don't know the side effects removing these two things could have as I don't know why there are there in the first place. From a fresh point of view, I would say that the conditional rendering can surely be removed. For the locale key I guess it depends how the useTranslation hook is implemented, but everything points that callers will rerender as expected when the locale changes.


#1049 is kind of related indeed, but much more niche. There we are talking about fallback rendering, which is a quite small portion of the usage (static + static paths + fallback true). Here, it's any page that doesn't use i18n in your app.


Click to see my diff
diff --git a/node_modules/next-i18next/dist/commonjs/appWithTranslation.js b/node_modules/next-i18next/dist/commonjs/appWithTranslation.js
index b573859..6ca28ae 100644
--- a/node_modules/next-i18next/dist/commonjs/appWithTranslation.js
+++ b/node_modules/next-i18next/dist/commonjs/appWithTranslation.js
@@ -94,13 +94,10 @@ var appWithTranslation = function appWithTranslation(WrappedComponent) {
       }, [i18n]);
     }
 
-    return i18n !== null ? __jsx(_reactI18next.I18nextProvider, {
+    // Passing i18n doesn't break anything
+    return __jsx(_reactI18next.I18nextProvider, {
       i18n: i18n
-    }, __jsx(WrappedComponent, (0, _extends2["default"])({
-      key: locale
-    }, props))) : __jsx(WrappedComponent, (0, _extends2["default"])({
-      key: locale
-    }, props));
+    }, __jsx(WrappedComponent, (0, _extends2["default"])(props))) 
   };
 
   return (0, _hoistNonReactStatics["default"])(AppWithTranslation, WrappedComponent);
diff --git a/node_modules/next-i18next/dist/es/appWithTranslation.js b/node_modules/next-i18next/dist/es/appWithTranslation.js
index 253916d..b313c85 100644
--- a/node_modules/next-i18next/dist/es/appWithTranslation.js
+++ b/node_modules/next-i18next/dist/es/appWithTranslation.js
@@ -51,13 +51,10 @@ export const appWithTranslation = (WrappedComponent, configOverride = null) => {
       }, [i18n]);
     }
 
-    return i18n !== null ? /*#__PURE__*/React.createElement(I18nextProvider, {
+    // Passing i18n doesn't break anything
+    return /*#__PURE__*/React.createElement(I18nextProvider, {
       i18n: i18n
-    }, /*#__PURE__*/React.createElement(WrappedComponent, _extends({
-      key: locale
-    }, props))) : /*#__PURE__*/React.createElement(WrappedComponent, _extends({
-      key: locale
-    }, props));
+    }, /*#__PURE__*/React.createElement(WrappedComponent, _extends( props))) 
   };
 
   return hoistNonReactStatics(AppWithTranslation, WrappedComponent);
diff --git a/node_modules/next-i18next/dist/esm/appWithTranslation.js b/node_modules/next-i18next/dist/esm/appWithTranslation.js
index 1550e0b..95a1423 100644
--- a/node_modules/next-i18next/dist/esm/appWithTranslation.js
+++ b/node_modules/next-i18next/dist/esm/appWithTranslation.js
@@ -57,13 +57,10 @@ export var appWithTranslation = function appWithTranslation(WrappedComponent) {
       }, [i18n]);
     }
 
-    return i18n !== null ? __jsx(I18nextProvider, {
+    // Passing i18n doesn't break anything
+    return __jsx(I18nextProvider, {
       i18n: i18n
-    }, __jsx(WrappedComponent, _extends({
-      key: locale
-    }, props))) : __jsx(WrappedComponent, _extends({
-      key: locale
-    }, props));
+    }, __jsx(WrappedComponent, _extends(props)))
   };
 
   return hoistNonReactStatics(AppWithTranslation, WrappedComponent);
diff --git a/node_modules/next-i18next/src/appWithTranslation.tsx b/node_modules/next-i18next/src/appWithTranslation.tsx
index 63fa28c..7aaa0b3 100644
--- a/node_modules/next-i18next/src/appWithTranslation.tsx
+++ b/node_modules/next-i18next/src/appWithTranslation.tsx
@@ -57,21 +57,16 @@ export const appWithTranslation = (
       }, [i18n])
     }
 
-    return i18n !== null ? (
+    // Passing i18n doesn't break anything
+    return (
       <I18nextProvider
         i18n={i18n}
       >
         <WrappedComponent
-          key={locale}
           {...props}
         />
       </I18nextProvider>
-    ) : (
-      <WrappedComponent
-        key={locale}
-        {...props}
-      />
-    )
+    );
   }
 
   return hoistNonReactStatics(

@isaachinman
Copy link
Contributor

Is there a good reason for setting the key?

It's necessary to force the app to re-render, although we may be able to remove that based on some of your proposed changes, @skrivanos.

@enzoferey What is your use case for translating some pages and not others? I'm open to any reasonable PR that might solve your issue, but do regard it as an edge case.

@enzoferey
Copy link
Author

@isaachinman My usage is about embedding pages from outside Next.js inside by page. As those pages will already come with i18n built-in, I want to wrap them in my Next.js applications but I don't need any translations from next-i18next in them. It's actually a common thing for businesses of a decent size that have both marketing and engineering teams working on their website. You have your marketing team working on landing pages generators, and the engineering teams integrating them and holding the infrastructure.

You can certainly hack your way around requesting empty namespaces or no namespaces at all, I haven't tried those solutions, they might work. I just think that as a user you don't expect that the fact of not using an add-on will break Next.js architecture principles like rerendering the _app.

@isaachinman
Copy link
Contributor

As I said, PRs welcome!

@zmarty
Copy link

zmarty commented Apr 13, 2021

In our case our header disappears when we switch locales, until we refresh the page. We think it's related to this bug.

@Layne101
Copy link

Hi all, We met this issue too. We want to use next-auth and next-i18next in our nextjs project. Here is a simple example which I combined the example of next-auth and the example of next-i18next. When navigating between pages with or without translations, the session of next-auth would be undefined until you manually refresh the page. I think it's related to the remounts of _app.

@Layne101
Copy link

next-auth-i18n-bug.mp4

attach repro video here

@rafaelpizzo
Copy link

rafaelpizzo commented Apr 24, 2021

In our app all pages have translations, but even though, changing from default locale / to same page in another locale /en would still trigger _app remount.

@enzoferey proposed solution also worked for me.

In appWithTranslation.tsx, by returning I18nextProvider HOC without the i18n !== null check and no key={locale} prop, it prevents App remounting when changing languages.
SSR and other features seems to still work fine.

Update:
Sorry, we just notice that without the i18n !== null, SSR fails given a few conditions.

Server Error
Error: ReactDOMServer does not yet support Suspense.

As long as there's no key={locale} in WrappedComponent, _app does not remount.

return i18n !== null ? (
      <I18nextProvider i18n={i18n}>
        <WrappedComponent {...props} />
      </I18nextProvider>
    ) : (
      <WrappedComponent {...props} />
    );

@GGrassiant
Copy link

Hi all, We met this issue too. We want to use next-auth and next-i18next in our nextjs project. Here is a simple example which I combined the example of next-auth and the example of next-i18next. When navigating between pages with or without translations, the session of next-auth would be undefined until you manually refresh the page. I think it's related to the remounts of _app.

@Layne101

We are in the same situation as yours, we are doing next-auth GitHub authentication and use next-i18next for French and English. Also tried with a simple repo similar to yours to the same avail.
I see there is an ongoing PR but in the meantime, we simply forced a refresh of the page like so and kept the original code commented out for future references.

 <a href={`/${router.locale === 'en' ? 'fr' : 'en'}${router.asPath}`}>
    <LanguageButton />
 </a>
 
 {/* <Link href={`${router.asPath}`} locale={router.locale === 'en' ? 'fr' : 'en'} passHref>
    <LanguageButton />
 </Link> */}

@hoangtrung99
Copy link

I have same issue.

@jln13x
Copy link

jln13x commented Feb 26, 2022

Any update on that?

@jln13x
Copy link

jln13x commented Feb 27, 2022

Created a new component:

Trans.tsx

import { appWithTranslation } from 'next-i18next';
import { AppProps } from 'next/app';
import React from 'react';

const Trans: React.FC<AppProps> = ({ Component, pageProps }) => {
  return <Component {...pageProps} />;
};

export default appWithTranslation(Trans);

_app.tsx

const App: React.FC<AppProps> = ({ Component, pageProps, router }) => {
  ...
  <Trans Component={Component} pageProps={pageProps} router={router} />
   ...
}

Seems to work fine

@isaachinman
Copy link
Contributor

@jln13x Sure, you are reiterating the same approach I've relayed to similar questions many times. Most recent example is: #1710 (comment).

@isaachinman
Copy link
Contributor

Fixed by #1631

@thienna
Copy link

thienna commented Mar 16, 2022

it took me a day to find this weird bug :)

@isaachinman
Copy link
Contributor

@thienna This issue is fixed in latest versions. Please let me know if you can reproduce after upgrading.

@thienna
Copy link

thienna commented Mar 16, 2022

i still face this issue :(
P/S: after upgrade, rm node, cache, next, etc...

my config:
next-i18next config js

const path = require('path')

module.exports = {
    i18n: {
        defaultLocale: 'en',
        locales: ['en', 'vi'],
    },
    localePath: path.resolve('./src/i18n/locales'),
}

in page A:

...(await serverSideTranslations(ctx.locale || 'en', ['common'])),

in page B (target route):
nothing, does not use SSR

@isaachinman
Copy link
Contributor

Please open a new issue, with a minimum reproducible repository linked.

@thienna
Copy link

thienna commented Mar 16, 2022

Please open a new issue, with a minimum reproducible repository linked.

i have edit my words

@ciceropablo
Copy link

@isaachinman, this problem still happens.
During page navigation, we are losing the global state via Context API.

I'm using "next-i18next": "^10.5.0".
Do we have anyone looking at this now?

@isaachinman
Copy link
Contributor

@ciceropablo Can you provide a minimal reproducible repo?

@kevin19940627
Copy link

I am still getting issues with _App unmounting even after using below screenshots. Am using next-i18next ^11.3.0
image

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

Successfully merging a pull request may close this issue.