Translations in shared layouts don't work anymore in 1.4.0

[duc-gp]

#743 introduces the same bug reported here already: #604

• also the author of Prevent infinite re-renders by memoizing the useTranslation hook #743 already mentioned that the change back then was reverted because of Translations in shared layouts don't work in 1.0.6, while they work in 1.0.5 #604 and now the change in 1.4.0 is introducing the issue again
• we experience the same issue

steps to reproduce:

• use repo Translations in shared layouts don't work in 1.0.6, while they work in 1.0.5 #604
• use latest 1.4.0 next-translate version in package.json
• install dependency
• start the project with yarn dev and click on the links

[gurkerl83]

Can you provide a small reproducer, including how you set up your layout in the pages and integrate your shared layout in _app. What is the latest working version? You are mentioning 1.4.0 does not work for you, does version 1.3.5 work. The translation memoized version of the translation hook was introduced in the canary versions of 1.3.6, 1.4.0 is the official version of those changes.

are you are doing something like _app

are you using the I18nProvider here manually and load namespaces in getStaticProps. 

const getLayout = Component.getLayout || ((page: ReactNode) => page);

return (
   <I18nProvider lang={__lang} namespaces={__namespaces}>
      {getLayout(
         <Component {...pageProps}/>
      )}
   </I18nProvider>
)

or are you using the appWithI18n hoc, and pass the app, which implicitly generates the I18nProvider?  

and in a page

Index.getLayout = (page: ReactElement) => {
   return (
      <YourLayout>{page}</Layout>
  );
};

[duc-gp]

Can you provide a small reproducer, including how you set up your layout in the pages and integrate your shared layout in _app. What is the latest working version? You are mentioning 1.4.0 does not work for you, does version 1.3.5 work. The translation memoized version of the translation hook was introduced in the canary versions of 1.3.6, 1.4.0 is the official version of those changes.

are you are doing something like _app

are you using the I18nProvider here manually and load namespaces in getStaticProps. 

const getLayout = Component.getLayout || ((page: ReactNode) => page);

return (
   <I18nProvider lang={__lang} namespaces={__namespaces}>
      {getLayout(
         <Component {...pageProps}/>
      )}
   </I18nProvider>
)

or are you using the appWithI18n hoc, and pass the app, which implicitly generates the I18nProvider?  

and in a page

Index.getLayout = (page: ReactElement) => {
   return (
      <YourLayout>{page}</Layout>
  );
};

Steps to reproduce:
You can clone this repo https://github.com/srflp/next-translate-bug
Then change to 1.4.0
Install deps and start.

Latest working next-translate version is 1.3.5 (where translate is not memoized)

[gurkerl83]

I can reproduce the problem with the repo you provided. I have updated Next and Next-Translate to the latest, respectively to 1.4.0.

The shared layout does indeed not update its translations on navigation. I have tracked the problem to a commit I made for a different issue, namely page exit transitions.

In the NextJs repository, a related discussion has been going on how a shared-layout can access the results of getStaticProps; like the rest of a page does.

E.g., Using 1.4.0, page translations get resolved (with the current memo implementation in place), but they don't get resolved in the respective shared layout.

Reverting the change I made in the following commit does indeed resolve the problem that translation gets resolved even in a shared layout, but I am not sure if this is a side effect.

Please see the commit message describing the problem why I have proposed instead of changing the entire context object as a dependable, limiting the context to the current language only.

Commit Message
#829

Change
https://github.com/vinissimus/next-translate/pull/829/files

Maybe not a good/scalable solution, but one which seems to work is to use the wildcard declaration for namespaces used in the shared layout.

pages: {
"*": ["home", "cat", "dog"],
}

I hope things can be sorted out resolving both of our problems.

Thx!

[duc-gp]

Using a wildcard for namespace declaration is indeed not a scalable solution and something we can not do because we have more than 10 namespaces across the same layout

I would propose to undo the commit for now or the whole memoization as it was undone before because it's introducing this bug that the translations are not working in shared layouts

[gurkerl83]

@aralroca Can you look at this issue please, maybe you can provide some feedback how to handle this properly. Thx!

[aralroca]

It's not the best solution because instead of a rerender it is a remount... But you can put a key to any layout as a workaround:

Cat.layout = (page) => <MainLayout key="cat">{page}</MainLayout>;
Dog.layout = (page) => <MainLayout key="dog">{page}</MainLayout>;
Home.layout = (page) => <MainLayout key="home">{page}</MainLayout>;

Or, as an alternative you can put the key depending the pathname on _app.js:

Replacing this:

function App({ Component, pageProps }) {
  const layout = Component.layout || (page => page);

  return (
    <>{layout(<Component {...pageProps} />)}</>
  );
}

export default App;

To:

import { Fragment } from 'react';
import { useRouter } from 'next/router';

function App({ Component, pageProps }) {
  const { pathname } = useRouter();
  const layout = Component.layout || (page => page);

  return (
    <Fragment key={pathname}>{layout(<Component {...pageProps} />)}</Fragment>
  );
}

export default App;

@gurkerl83 would it be a lot of trouble in memorizing t also put the router.pathname or lose the sense of memorization? I guess adding the router.pathname would avoid these errors.

[duc-gp]

hello, translation are now working but now we face other sideeffects like layout shifting.. e.g. we have an indicator which has a default state and takes some time to get the fetched state, previously when switching pages nothing changed and now it changes to the default state because of the unmount and takes some time to get the previous state again.

Also I think unmounting is contradictory to the idea of shared layouts

[aralroca]

Yes I understand, it's just as a temporal workaround. The best thing would be that the memorization of t would also take into account the router.pathname so the workaround would not be needed and it would be a rerender instead of a remount.

Before putting the router.pathname as a dependency I would like to confirm @gurkerl83 that it still fulfills the usage of the memoization that people need, since it was a very demanded feature (there are several issues asking for the same).

[gurkerl83]

Please be real careful with using NextJs router hook! We observed an increased re-rendering using this hook. I have opened the following issue describing the problem.

#833

Something from the router as a dependency; in dynamic pages, the pathname does not provide what its name suggests.

router.pathname has multiple representations: using dynamic pages of arbitrary depth it has [...slug], for depth 1 it has [slug] => no unique identifier

router.query.slug using dynamic pages of arbitrary depth slug is an array, for depth 1 it is a string.

Using the router through the respective router-hook in the useTranslation hook is no option.

I describe in the referenced issue a problem using only the hash for simple in-page navigations the router context changes (the asPath problematic), which results in the effect that every component tree re-renders where useTranslation gets utilized.

Will the memoization benefit from slug as a dependable: In theory, yes, but don`t do it; the useRouter hook creates a considerable number of unproductive re-renders just from using it.

My position has changed
Unless there is no extra work put into both the router hook and the context from the Next team, limit its usage to app components near the shell. Do not use it in components or custom hooks commonly and frequently used in the application; that includes the useTranslation hook.

Also I think re-mounting is contradictory to the idea of shared layouts

It should not re-mount, just re-render. I have to reproduce the re-mounting effect you experience when putting keys in-front of layout either in Fragment or in the Layout component but React re-mounts when it sees a different key for a section previously rendered with a different key.

About the change I made, maybe it was not necessary at all.
Properly supporting page exit animations set a requirement that a component's resolved translations of the "past-state" need to stay intact even when NextJs already has loaded the next page.

[Page A (namespaces loaded - rendered, everything is fine) -> got to page B
[Page A exit animation starts] ->
[Page B (namespaces of Page B loaded, namespaces of Page A lost] -> translations of Page A lost
[Page A exit animation finished]

I also experiment with a new context where both previous (Page A) and current namespaces (Page B) can get used in the Provider. In that case, the namespaces of the previous page survive new ones. The general loading of namespaces in getStaticProps will not be affected.

Thx!

[duc-gp]

@gurkerl83 So it sounds this commit #829 can be reverted ?

edit: I've created a PR #836

if this is not viable, I came up with another idea adding a second argument shouldMemo=true for useTranslation so users can decide if they want to have a memorized version of t

same would be needed for the Trans component

but i would prefer undoing passing ctx.lang to the dependency array in the first place since @gurkerl83 also said that his commit may be not necessary

[gurkerl83]

@duc-gp Before going forward please check 1.3.6-canary.1 with shared layout. This version was also released on NPM including the memo variant before I modified it.

https://github.com/vinissimus/next-translate/releases/tag/1.3.6-canary.1

Thx!

[duc-gp]

@gurkerl83 I checked out next-translate and replaced ctx.lang with ctx and with this change our app is working again, so this means before your commit it seems to work without problems

[duc-gp]

@gurkerl83 I double checked with 1.3.6-canary.1 again and there translations with shared layout is working

[gurkerl83]

Let's wait what @aralroca says, going back... he is one of the maintainers of the project.

[gurkerl83]

Ok, I have the first stable version local running (prev and current- namespaces); look at the description above. These are needed for page exit animations; since this is all placed in a new context, it doesn't need the ctx.lang constraint anymore, just the ctx as before.

I don't observe any re-rendering anymore; the translations survive the loading and animation conflict from before, which eventually led to the consideration to restrict ctx to ctx.lang.

After a few tests, I could also provide this change Next-Translate. I assume that I18nContext has to get reduced in its current implementation, which leads to increased re-rendering per se in the current implementation. I removed almost everything in my local version, and it still works.

So from my side, no more objections stand against the revert of my change.

[aralroca]

Perfect, I approve the change as well. What would be nice is to add the tests of both cases to ensure that in the future it does not break again. Thank you for your contributions

[duc-gp]

Perfect, I approve the change as well. What would be nice is to add the tests of both cases to ensure that in the future it does not break again. Thank you for your contributions

great news, thanks! who needs to add the tests? I havent really understood what the cases are.. maybe we can also revert first with this PR #836 and someone else delivers additional tests afterwards?

[rmlevangelio]

Hi everyone, I'm not sure if this has been fixed in latest version (1.5.0) but I can say that I'm facing similar issue.

Inside /pages folder, translations work fine. But for the components inside the page layout, it is undefined.

/pages/index.tsx

const Page = () => {
   const {t, lang} = useTranslation();
   console.log(lang) // this returns the correct locale
}

but if it has layout,

/components/Layout.tsx

const Layout = (children) => {
   const {t, lang} = useTranslation();
    console.log(lang) // this returns undefined
    
   return <div>{children}</div>
}

/pages/index.tsx

import Layout from '../components/Layout'

const Page = () => {
   const {t, lang} = useTranslation();
   console.log(lang) // this returns the correct locale
}

export default Page;

Page.getLayout  = (page) => <Layout>{page}<Layout>

I tried to add key to the layout as @aralroca suggested but it seems like it doesn't do the trick.

cc @gurkerl83

My i18n.js is properly configured

const hoistNonReactStatics = require('hoist-non-react-statics');

module.exports = {
    locales: ['en', 'es'],
    defaultLocale: 'es',
    pages: {
      '*': ['common'],
    },
    staticsHoc: hoistNonReactStatics,
    loadLocaleFrom: (locale, namespace) => import(`./src/locales/${locale}/${namespace}.json`).then((m) => m.default),
};

[gynekolog]

@rmlevangelio Are you using edge server-rendering?
next.config.js

const nextTranslate = require("next-translate");

/** @type {import('next').NextConfig} */
const nextConfig = {
  ...
  experimental: {
    runtime: "experimental-edge",
  },
  ...
};

module.exports = nextTranslate(nextConfig);

With enabled experimental-edge my app wasn't loaded locales on the pages with the custom getServerSideProps or with getLayout function.

[loiclouvet]

It seems that I reproduce the same issue in v1.5.0.

[hofman-p]

Bug is still here in 1.6.0

None of my layouts work when I have a getLayout function attached to pages

[av-aptd]

Same here with 1.6.0, is there any workaround ?