fix: better support for shallow route changes

[skrivanos]

• Memoize the i18n client and do not re-create it unless the route or
  locale has changed (fix Re-renders of memoized components on shallow route changes #1059).

• Let Next's router locale take precedence over initialLocale.

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/isaachinman/next-i18next/3bChh7HvMWM93jXnVYUbidpHSgRL
✅ Preview: https://next-i18next-git-fork-skrivanos-shallow-rout-2dcf5b-isaachinman.vercel.app

[skrivanos]

Reckon examples/simple/pages/index.ts should be changed to use shallow routing for the change locale button?

[isaachinman]

In general, these type of changes make me a little nervous, as it seems like even though it might fix shallow routing, it may also cause other issues.

Would something like this work as well? I'm not sure I'm keen on memoising the entire client.

Let me know your thoughts!

[skrivanos]

In general, these type of changes make me a little nervous, as it seems like even though it might fix shallow routing, it may also cause other issues.

Would something like this work as well? I'm not sure I'm keen on memoising the entire client.

Let me know your thoughts!

I can understand that. But no, that fix alone would not help (and it's also baked into this PR). One of the problems is that creating a new instance causes two re-renders of memoized components on every shallow route change, leading to extremely poor performance.

[isaachinman]

Sorry for a delayed response. Happy to keep this PR moving and get it merged in.

[skrivanos]

@isaachinman Noticed a slight hiccup when using this branch in our real application: switching the locale with a shallow route change will not load resources for the new language, because serverSideTranslations aren't re-run (expected). However, this cannot be fixed by including the HTTP backend either because it will not fetch missing resources if the resources config option is provided to i18next: https://www.i18next.com/how-to/add-or-load-translations#load-using-a-backend-plugin

The question is whether this should be solved in the context of this PR or not. Perhaps the scope of the PR should simply be changed (i.e. change commit message and rename it) to only cover #1059 rather than #1023 as well.

Edit:
Maybe partialBundledLanguages could solve this..?

[isaachinman]

@skrivanos Do you have an example use case for switching the locale and wanting shallow routing at the same time? That seems like somewhat of an edge case to me.

[skrivanos]

@skrivanos Do you have an example use case for switching the locale and wanting shallow routing at the same time? That seems like somewhat of an edge case to me.

@isaachinman No, I don't and I agree that it's an edge case. Although it might be considered unexpected that it doesn't work, but could perhaps just mention in the README.

[isaachinman]

Cool, then I agree it should be handled in a separate PR (if at all).

[isaachinman]

In general, we should be able to cease passing initialLocale via props, here, right?

[skrivanos]

In general, we should be able to cease passing initialLocale via props, here, right?

Yes, I believe so. I can update the PR to remove it, just very short on time lately.

[isaachinman]

Cool, no worries – I know the feeling.

[DanPurdy]

Hey both, just to confirm that initialLocale is safe to remove from the props returned by serverSideTranslations. Ive changed it in my PR too so once we're good to merge this I'll rebase and pull apart mine.

[skrivanos]

@isaachinman @DanPurdy finally had time to updated the MR now - I opted to exclude #1023 since this does not solve it completely.

[isaachinman]

@skrivanos Can you rebase against latest? This PR is my top priority and would love to get it in ASAP.

[skrivanos]

@skrivanos Can you rebase against latest? This PR is my top priority and would love to get it in ASAP.

@isaachinman Done!

[isaachinman]

@skrivanos @DanPurdy This PR has caused #1484. Any objections to leaving the memoisation, but removing the router.locale check?

[skrivanos]

@skrivanos @DanPurdy This PR has caused #1484. Any objections to leaving the memoisation, but removing the router.locale check?

@isaachinman Won't have time to dive into this again soon unfortunately since I'm a bit overcrowded with work at the moment. I can't quite remember why I changed it to use router.locale but there has to be a reason.