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

appWithTranslation is causing the React state to not consist when doing client-side page navigation. #2259

Closed
pgburrack opened this issue Feb 16, 2024 · 7 comments

Comments

@pgburrack
Copy link
Contributor

🐛 Bug Report

Wrapper _.app.js with the appWithTranslation HOC is causing the React state to not consist when doing client-side page navigation.

To Reproduce

I created this repo with a simple example that you can clone and try:
https://github.com/pgburrack/next-i18next-issue

In the example repo I created a DummyProvider with the initial state.

const [state, setState] = useState({
    a: null
  });

Steps to reproduce:

  1. start repo by running npm run dev + open http://localhost:3000/
  2. Open console developer tools. I would expect to see:
{a: null}
{a: null}
{a: 'foo'}
{a: 'foo'}

👆 this is the state that is in the DummyProvider.
3. Click on "Go to about page" (while keeping the console open)
image

You will see that the state in DummyProiver is initiated again.

{
  a: null
}

Note: if you comment out appWithTranslation and just export the App component.
you will see that the state is being kept on client-side navigation.

Expected behavior

When navigating to the /about page i would expect the DummayProvider to be

{a: 'foo'}

Your Environment

  • runtime version: Node, v18.15.0, testing on chrome
  • i18next version: "23.7.16"
  • "next-i18next": "15.0.0"
  • "react-i18next": "13.4.1"
  • "next": "12.2.4",
  • os: Mac
  • any other relevant information

P.S i found that if I pass options to appWithTranslation it will work as expected.

Example:

export default appWithTranslation(MyApp, {
  i18n: {
    defaultLocale: DefaultLocale,
    locales: SupportedLocales,
  })

The docs are not saying to do that as well, as we have that information in next-i18next.config.js so i am not sure why i would need to do that.

I suspect that the following line of code is causing the issue

https://github.com/i18next/next-i18next/blob/master/src/appWithTranslation.tsx#L120

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

I saw that when doing the navigation will disappear.

Could use help with this 🙏

@pgburrack
Copy link
Contributor Author

I also noticed that the issue is only happening when one page is translated and the other is not.

@adrai
Copy link
Member

adrai commented Feb 16, 2024

I also noticed that the issue is only happening when one page is translated and the other is not.

That's exactly the problem...
if a page is not using next-i18next, there is nothing that will correctly initialize next-i18next...
So you can either use next-i18next on every page or pass the nextI18nextConfig config via appWithTranslation... (btw: you don't need to create a new "empty" config, you can just pass in the original config)

import nextI18nextConfig from '../../next-i18next.config';
//...
export default appWithTranslation(MyApp, nextI18nextConfig);

@pgburrack
Copy link
Contributor Author

Hi @adrai

Thanks for the quick reply and the suggestion!

To give you some context on my original issue which made create this example repo and this Github issue.

I started to add next-i18next to a few page in an existing NextJs website that has a lot of pages.
I started seeing bugs which made me start investigate and find the issue with next-i18next.

I am wondering if there is a way next-i18next can always make sure to wrap an app with the <I18nextProvider i18n={i18n}> provider and either default i18n to an empty object to avoid react state from getting cleared? not sure what are the implications of that?

OR

update the README/docs to mention the issue here and how to solve it?

basically to make sure other people are not facing the same issue 😃

Maybe something like:

Partially translated website

OR

Start using next-i18next Incrementally

If you want to only use next-i18next on some pages make sure to do the following

import nextI18nextConfig from '../../next-i18next.config';
//...
export default appWithTranslation(MyApp, nextI18nextConfig);

@adrai
Copy link
Member

adrai commented Feb 16, 2024

Feel free to provide a PR to update the readme

@pgburrack
Copy link
Contributor Author

@adrai sure 😄

just confirm: you don't think it is feasible or good idea to always wrap the app with a Provider? and if no, why? 🙏

@adrai
Copy link
Member

adrai commented Feb 16, 2024

no, we tried this in the past, and it resulted in other errors

@pgburrack
Copy link
Contributor Author

Opened a PR
#2260

@adrai adrai closed this as completed Feb 16, 2024
Greenheart added a commit to Klimatbyran/klimatkollen that referenced this issue Apr 8, 2024
Greenheart added a commit to Klimatbyran/klimatkollen that referenced this issue Apr 23, 2024
* Init i18next

* Remove unused file

* Switch to next-i18next

* Add configuration and translate cookie banner

* Fix Next.js warning "API handler should not return a value, received object"

* Translate footer and add eslint rule to avoid import mistake

* Ensure SSR and CSR bundles provide consistent i18n config. See i18next/next-i18next#2259 for details

* Remove unused page

* Remove unused property

* Add top-level translations for StartPage

* No need for translations when redirecting

* Add 404 translations

* Simplify DropDown to remove unused classes

* Fix formatting error in Header

* Remove Footer tests since they 1) it wasn't worth adapting them to the new i18n structure, and 2) because they tested the content rather than cricital functionality

* Combine footer and common translations into a single namespace

* Fix formatting issue

* Add basic i18n to remaining pages

* Use markdown to simplify i18n for dataset definitions

* Style Markdown content according to the theme, with the option to override components

* Use markdown for `body` field of DataDescriptions

* Simplify date formatting

* Prevent styled-components from rendering invalid HTML props

* Remove unused components

* Add translations for common components

* Fix layout issue

* Begin adding municipality translations

* Add translations for municipality numbers

* Allow ScorecardSections to use consistent formattig even with markdown content

* Add municipality ScoreCard translations

* Use HTML details element to make ScorecardSection more accessible and easier to use

* Styling tweaks

* Add missing key attribute

* Fix React hydration error caused by missing <thead> element

* Add solutions translations

* Improve styles for Markdown

* Replace JSX with Markdown formatting

* Use markdown formatting instead of JS override

* Translate utsläppsberakningar and use Markdown for simplified formatting

* Translate partierna, and update next/image props

* Translate Källor och Metod

* Improve grid styling on about page

* Translate about page

* Improve spacing

* Clarify dataset task ideas

* Translate sitemap

* Update tests to use mocked i18n and test against expected keys rather than values

* Begin translating datasets

* Translate the easy parts of dataset definitions

* Clarify naming

* Replace deprecated method String#substr with String#slice

* Refactor to prepare for translated dataDescriptions

* Add translations for datasets

* WIP: hack around broken SSR translations

* Cleanup SSR translations

* Separate hardcoded dataset keys from the UI labels, to allow translations

* Update tests to reflect fully translated datasets

* Reduce margin-top for FactSection

* Fix translation error

* Fit images in container width

* Fix modal positioning. Also improve UX by closing on backdrop click. Long term our custom Modal should be replaced with a standard Modal solution.

* Ensure special data for climate plans and procurements renders correctly. Also improve TypeScript types to prevent this error in the future.

* Fix type error

* Revert spacing change for CementClarification
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

2 participants