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

A way to get the t-function outside of components #983

Closed
skrivanos opened this issue Feb 24, 2021 · 16 comments · Fixed by #986
Closed

A way to get the t-function outside of components #983

skrivanos opened this issue Feb 24, 2021 · 16 comments · Fixed by #986

Comments

@skrivanos
Copy link
Contributor

Is your feature request related to a problem? Please describe.

Maybe I'm missing something, but it does not currently seem possible to translate things outside of components/hooks without passing down the t function as an argument manually which is quite a hassle. For instance, we have various functions that open toasts on errors and those have to be translated.

In previous versions of the library we could get the global instance via the module that initialised next-i18next.

Describe the solution you'd like

Perhaps something like this:

https://github.com/vinissimus/next-translate#gett

@isaachinman
Copy link
Contributor

Hey @skrivanos, thanks for the feedback.

There isn't a global singleton in next-i18next@8.0.0 which you can just import and use as there was in previous versions. However, you always have access to:

const { i18n } = useTranslation()

And can do whatever you like with it, including assigning it to a global singleton.

Let me know if you have any thoughts or suggestions!

@isaachinman
Copy link
Contributor

Hey @skrivanos, I've had a think about this, and have come up with a way of exporting a global singleton. I will put together a PR and have this released within the next ~24 hours.

@skrivanos
Copy link
Contributor Author

@isaachinman Thanks for the extremely fast reply (and potential fix - I didn't even have time to respond!)

I'm impressed 👍

@isaachinman
Copy link
Contributor

@skrivanos #986 should fix this. Do you mind testing the PR against your use case? You will just need to clone the repo, checkout the branch, do yarn build, and yarn link the dep.

@felixmosh
Copy link
Contributor

felixmosh commented Feb 25, 2021

I've encountered a similar use case, I need an access to i18n object (to change a plural form of some lang).
Your PR is not ideal, since it depends on the place & time of where AppWithTranslation is invoked (react / next will decide this).

Other point, creating a new instance of i18next inside a component render function is not a good idea, there is no reason to do so.

@isaachinman
Copy link
Contributor

Hey @felixmosh. Not sure it's logically possible to export a global singleton which returns identically, from a package that is in turn imported by its user and invoked with dynamic config options.

Also, the i18n object comes from context and is not created.

Open to any and all suggestions!

@felixmosh
Copy link
Contributor

felixmosh commented Feb 25, 2021

createClient creates a new instance of i18next each time _app component is rendered.

Since the config is not dynamic, only the lang & it's resources...
Maybe we can create an instance with the static config, pass it to appWithTranslation which will use the passed translations
and setup them using addResourceBundle (if it is not loaded already) and then invoke changeLanguage if the current languages is not set. This approach suitable for client side.

For server side, it should still generate a new instance each render, or load all languages & namespaces, and just change the lang by what next tells

react-i18next has useSSR hook which connects to the provider and initialize the store + lang

WDYT?

@isaachinman
Copy link
Contributor

createClient creates a new instance of i18next each time _app component is rendered.

Correct, but that's rather the point, isn't it?

The _app file will only be executed once per page in SSG, and once per request in SSR. What issue are you trying to solve for with the addResourceBundle approach?

@skrivanos
Copy link
Contributor Author

skrivanos commented Feb 25, 2021

@isaachinman I cannot test your MR against our code since we are yet to actually upgrade to Next 10 (discovered this problem while investigating compatibility of our current dependencies). With that said, I've had a look and I think it should work considering our use case is translating things after a user action. A bit annoying to have to check for null on the export (due to TS) but I suppose we could live with that.

However, I think @felixmosh concerns are very valid and should be taken into consideration before merging. It is indeed worrying that the instance is being re-created every app render. It seems like

  1. The app could potentially re-render multiple times on the initial page load (which happens in our case).
  2. The app re-renders every time Next switches route, which could for instance be when changing query params with shallow routing (we do this a lot).

Unless I am mistaken, if the instance is re-created every time the context value will change causing a cascaded update of every single component that uses the context / useTranslation?

@isaachinman
Copy link
Contributor

Unless I am mistaken, if the instance is re-created every time

This is a completely independent concern of the globalI18n feature being discussed. Yes, appWithTranslation creates a new i18next client on every render. Whether that causes a rerender of child components depends on:

  1. How you manage your key props
  2. Whether you're using shallow routing or not

The _app component should not rerender multiple times on page load. Not sure what would be causing that in your case, but that is not expected.

@skrivanos
Copy link
Contributor Author

Unless I am mistaken, if the instance is re-created every time

This is a completely independent concern of the globalI18n feature being discussed.

While somewhat true, it closely relates to the feature being implemented as well (or well, to @felixmosh proposed implementation at least). It would be a shame to merge something that has to be changed later since it's an extension of the public API.

Yes, appWithTranslation creates a new i18next client on every render. Whether that causes a re-render of child components depends on:

  1. How you manage your key props

Can you elaborate?

  1. Whether you're using shallow routing or not

The _app is re-rendering every time there's a shallow route change. I'm not sure if there's something wrong in our code base causing this, but I just tested it before typing up my reply.

Normally this is fine, since performance critical components are memoized. However, if using useTranslation would trigger re-renders of memoized components as well there's a problem.

The _app component should not rerender multiple times on page load. Not sure what would be causing that in your case, but that is not expected.

My mistake, this was due to shallow routing as well (query params being updated on load), apologies.

@isaachinman
Copy link
Contributor

Can you elaborate?

If you add a key attribute to a React component, and its props don't change, the component won't rerender.

Let's step back and think about this from a broad perspective:

  1. The i18next client is created by appWithTranslation and therefore does not exist until _app runs
  2. _app should only ever run once per page/request/navigation

@skrivanos
Copy link
Contributor Author

I created a quick PoC to demonstrate the problem:

https://github.com/skrivanos/next-i18next-rerender-poc

As you can see, the component using useTranslation is re-rendered every single time the query changes despite being wrapped in memo. Now imagine that you have an advanced page with potentially hundreds of components using useTranslation and you also have the need to update the query, perhaps because of filtering or whatever. This will quickly become a performance issue and I'd say this is a serious problem (would definitely prevent our upgrade, at least).

I suppose the easiest solution would be to wrap the created i18n instance in useMemo or simply feed the context with the global instance if it's set?

@isaachinman
Copy link
Contributor

@skrivanos Yeah, I think it would be an easy issue to solve. Would you be interested in working on a PR?

@isaachinman
Copy link
Contributor

@skrivanos Any update? Would be great to get this merged soon.

@skrivanos
Copy link
Contributor Author

@isaachinman I am very swamped at work right now because of an upcoming deadline so I don't have time to do much else right now unfortunately. However, I feel like if you're OK with the API you can go ahead and merge your MR. The only other solution that I think might make sense is so allow you to pass a custom i18n instance to appWithTranslation like @felixmosh proposed, since this would also take care of the "i18n might be null"-problem. But I can see how that might be complex in some ways...

The memoization problem could be solved as part of a separate issue like you mentioned before.

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.

3 participants