search

QuiiBz / next-international

Type-safe internationalization (i18n) for Next.js
https://next-international.vercel.app
MIT License
1.25k stars 59 forks source link

Suspense triggered when changing the locale after upgrading to 1.1.1 #227

Closed hipdev closed 10 months ago

hipdev commented 11 months ago

Describe the bug After updated to the latest version 1.1.1 when changing the locale the site is doing a fullpage reload with a white screen.

The only change I did from v 1.0.1 was adding the locale param:

      <I18nProviderClient locale={params.locale} >
      ...
      </I18nProviderClient>

To Reproduce Steps to reproduce the behavior:

  1. Go to '...'
  2. Click on '....'
  3. Scroll down to '....'
  4. See error

Expected behavior I don't want a full reload, just a change in the locales like before

Screenshots If applicable, add screenshots to help explain your problem.

About (please complete the following information):

QuiiBz commented 11 months ago

I see that you're not using the fallback prop in I18nProviderClient, which is used by a Suspense boundary internally.

To give more context, it's not a full-page reload, but rather this Suspense boundary that is triggered when changing the locale while the new one loads.

gustaveWPM commented 11 months ago

I see that you're not using the fallback prop in I18nProviderClient, which is used by a Suspense boundary internally.

To give more context, it's not a full-page reload, but rather this Suspense boundary that is triggered when changing the locale while the new one loads.

Could it be possible to create an option to configure whether we want to load all the locales during the initialization phase, or lazily load them for the client-side part? (In the createI18nClient parameters).

hipdev commented 11 months ago

@QuiiBz ok, I put the fallback prop but it is not the expected functionality, now what it does is show that component when changing a language, but I want it to work as it was before, I don't want to show a component that replaces the entire page, just change the language 🤔

QuiiBz commented 11 months ago

Yeah I know that it doesn't "solve" this issue. The thing is that I have no idea how to:

1) keep lazy-loading the locale 2) allow Static Rendering 3) never show a loading state

all at once. The current solution uses the use() function from React, which unlocked 2) while keeping 1), but now triggers a Suspense boundary instead of keeping the old value when changing the locale. I'll ask on Twitter today but I've already been struggling for many hours on this problem.

QuiiBz commented 11 months ago

I've published next-international@1.1.2-rc.0 (#234) which might fix the issue, but it's a very ugly hack. Let me know if that works for you.

ApacheEx commented 10 months ago

Hi guys

I had similar issue, when I open a random page which basically renders app/not-found.tsx with const t = await getI18n();

somehow I saw blank screen (no errors in console, except 404 page not found). The issue was gone when I removed I18nProviderClient. So, I've upgraded to next-international@1.1.2-rc.0 and whooo it works now.

gustaveWPM commented 10 months ago

I've published next-international@1.1.2-rc.0 (#234) which might fix the issue, but it's a very ugly hack. Let me know if that works for you.

Personally, I still have issues when I refresh a page then use my browser next/prev buttons.

I don't understand where the locales are stored once imported.

EDIT: I've looked at the code a bit more, and in fact it's based on the principle that if the promise has already been consumed, the result is returned directly.

It almost makes me want to fork and see if my approach with a "Store" (hideous, admittedly, but a store nonetheless) wouldn't allow more flexibility...

Maybe another ugly hack would be to "Cache" the imported locales in a Namespace? (Which acts as a Global Object.) EDIT: Map does it better.

hipdev commented 10 months ago

@QuiiBz I tried with the next-international@1.1.2-rc.0 version but is not working, I'm still seeing the issue.

QuiiBz commented 10 months ago

We've had a conversation with Dan on X (Twitter), and it doesn't seem possible right now: https://twitter.com/dan_abramov/status/1711417357183098974

I've tried many many things, and none works for these 3 points mentioned above. I'm kinda running out of ideas.

If anyone wants to help, here are the steps:

andelkocvjetkovic commented 10 months ago

You could create server actions to change the locale and then pass them as props to the client component. This way, the <I18nProviderClient fallback={...}></I18nProviderClient> wouldn't be needed at all, and everything could be done in a single request trip.

QuiiBz commented 10 months ago

Thanks for the suggestion. However, I have a few concerns:

I don't get how a Server Action would help removing the need for I18nProviderClient. We still have to load the current locale on the client, not only when changing the locale but also on the first load. Additionally, Server Actions are in beta, and that would also mean we could no longer have a fully static output.

andelkocvjetkovic commented 10 months ago

The main reason why I have to use the I18nProviderClient is to change the locale state. I handle all translation in server components.

gustaveWPM commented 10 months ago

The main reason why I have to use the I18nProviderClient is to change the locale state. I handle all translation in server components.

It seems to me that some UI libs sometimes force to put a "use client" instruction in a component, and this means that you can have to use client-side translations as well in this context. :/

(Even if things are improving and some UI libs are becoming tailor-made for Next and its App Router over time.)

gustaveWPM commented 10 months ago

We've had a conversation with Dan on X (Twitter), and it doesn't seem possible right now: https://twitter.com/dan_abramov/status/1711417357183098974

I've tried many many things, and none works for these 3 points mentioned above. I'm kinda running out of ideas.

If anyone wants to help, here are the steps:

I think I would have done it more like this. I'm a bit too stupid to know how to really test properly if it works "Better". (Sorry.) https://github.com/gustaveWPM/next-international/commit/321d433197594a2e938fa54b735e0172fd79bd91

QuiiBz commented 10 months ago

Thanks for sharing! Hovewer, this part will still trigger the component to suspend and show the fallback when changing to a locale that isn't in cache:

Screenshot 2023-10-11 at 09 16 51

gustaveWPM commented 10 months ago

Thanks for sharing! Hovewer, this part will still trigger the component to suspend and show the fallback when changing to a locale that isn't in cache:

Screenshot 2023-10-11 at 09 16 51

Yes. Idk what to do. :/ Load and cache all the locales as soon createI18nProviderClient is called?

Doesn't seem to be the right thing to do. :/

On the other hand, the fact that the suspense is triggered during each lazy loading seems to me to be "logical"? Perhaps we could conditionally populate the cache before changing the route in useChangeLocale?

gustaveWPM commented 10 months ago

https://github.com/gustaveWPM/next-international/commit/e986b64f8847979ac4b6e6a6dccf2951242d5d10 ( in addition of this: https://github.com/gustaveWPM/next-international/commit/321d433197594a2e938fa54b735e0172fd79bd91 )

It seems to work properly. I let you check this. I have only implemented this for the app router (no pages impl, because I have absolutely no idea how this paradigm works).

QuiiBz commented 10 months ago

I think that's actually a great idea to load the locale manually within usechangeLocale, and then navigate to the new route. I'll try later today in #234

gustaveWPM commented 10 months ago

I think that's actually a great idea to load the locale manually within usechangeLocale, and then navigate to the new route. I'll try later today in #234

That's what I've done just now in the commit of the bisous. ( Just here: https://github.com/QuiiBz/next-international/issues/227#issuecomment-1757169729 )

QuiiBz commented 10 months ago

Yeah that's precisely what I was mentioning, great idea!

gustaveWPM commented 10 months ago

Yeah that's precisely what I was mentioning, great idea!

I'd just forgotten an "else" in my spaghetti code. I have done an amend on it just now.

QuiiBz commented 10 months ago

@gustaveWPM @hipdev @ApacheEx I've published next-international@1.1.2-rc.1, could you update and let me know if it works as expected? I've tried and it seems to work great on my side.

gustaveWPM commented 10 months ago

@gustaveWPM @hipdev @ApacheEx I've published next-international@1.1.2-rc.1, could you update and let me know if it works as expected? I've tried and it seems to work great on my side.

Gonna try it rn. :) Could I see the changelog?

Here, I guess? https://github.com/QuiiBz/next-international/pull/234/commits/98f40222b3a28a57ecc94d317d6abc057459d48b

It's actually a much better idea to use a Map for caching. What's more, that's exactly what the people I respect do. I don't know why I do my dumb things with namespaces... :sweat_smile:

Well, I still have some "Too much" fallbacks when I use the next/prev buttons. (I thought it wasn't the case with my ugly code?)

It also seems to not perfectly work when we use those buttons + the rewriteDefault strategy. But, even though I'm a very pessimistic person, I think we've made progress? The language change seems to be going better.

I'm looking forward to read hipdev again. :)

I just think that we could avoid a lot of calls to the "use" function.

It would be a good thing to do some caching here too rather than juggling between the Map and use (until there's finally some caching with a call to changeCurrentLocale). Or maybe I'm wrong?

(I don't have a lot of confidence in this function given that it's still in beta, I don't know what it does exactly, and I'm not comfortable with 'abusing' it.)

const clientLocale = localesCache.get(locale) ?? use(locales[locale as keyof typeof locales]()).default;

https://github.com/QuiiBz/next-international/commit/98f40222b3a28a57ecc94d317d6abc057459d48b#diff-17f758f6c25222646825815d84f2fdb41479ed4cf0ca8ea861068333290bbd6bR23

I was doing something like:

(So, we could also use this cache instead of always process the promise in changeLocale, which feels odd to me too.)

return function changeLocale(newLocale: LocalesKeys) {
  locales[newLocale as keyof typeof locales]().then(module => { ...

https://github.com/QuiiBz/next-international/commit/98f40222b3a28a57ecc94d317d6abc057459d48b#diff-91d14a1ec6fe793b18e18d38c0d9225212a78bbea8fbe659d4ffceef54917136R33

Also, why not immediatly call flattenLocale when caching? (Perhaps the nonsense I was saying about Proxy would finally make sense if it would be relevant to intercept the "set" calls and intermediately call flattenLocale?)

Maybe something like this:

function flattenLocale(obj) {
  // * ...
  return flattenedObj;
}

const cache = new Map();

export const cacheProxy = new Proxy(cache, {
  set: function(target, key, value) {
    const flattenedLocale = flattenLocale(value);
    target.set(key, flattenedLocale);
    return true;
  }
});

cacheProxy.set('en', { nested: { prop: 'value' } });
cacheProxy.get('en');

I'm wondering if this kind of additional "on-the-fly" calculation doesn't cause slowdowns (even very slight), which could lead to very brief fallback displays when I'm browsing some pages on my prototype website.

Moreover, I liked the fact that this code:

const value = useMemo(
  () => ({
    localeContent: flattenLocale<Locale>(clientLocale),
    fallbackLocale: fallbackLocale ? flattenLocale<Locale>(fallbackLocale) : undefined,
    locale: locale as string
  }),
  [clientLocale, locale]
);

Was just reduced to:

const value = useMemo(
  () => ({
    localeContent: clientLocale,
    fallbackLocale,
    locale: locale as string
  }),
  [locale, clientLocale]
);

Because I think this useMemo function should be as fast as possible.

EDIT: sorry for the multi-posts.

QuiiBz commented 10 months ago

Just published next-international@1.1.2-rc.2 that adds more caching as suggested. Navigating in the example, I'm no longer seeing any suspense fallback when refreshing, changing the locale or going backward/forward.

I'm waiting for more folks to test and confirm that it's fixed before publishing an official release.

ApacheEx commented 10 months ago

Hi guys, just tried the last two releases:

1) next-international@1.1.1 / next-international@1.1.2-rc.1 - I always see the "fallback" of Suspense when I land to "not-found" page. It's stuck and not resolving. 2) next-international@1.1.2-rc.2 - works as expected, I see fallback only once and then page content.

p.s the way of how you solved it looks much better.

hipdev commented 10 months ago

I tried next-international@1.1.2-rc.1 and next-international@1.1.2-rc.2, I'm still seeing a white page reload 😭

Just in case, here is my code:

      ...
      <I18nProviderClient locale={params.locale}>
        <NextUIProvider>
          <Toaster />
          {children}
        </NextUIProvider>
      </I18nProviderClient>
      ...
QuiiBz commented 10 months ago

I tried next-international@1.1.2-rc.1 and next-international@1.1.2-rc.2, I'm still seeing a white page reload 😭

Could you use the fallback prop from I18nProviderClient to make sure it's coming from next-international? Otherwise, I think we'll need a minimal reproduction as the issue is fixed for 3 of us.

hipdev commented 10 months ago

I tried next-international@1.1.2-rc.1 and next-international@1.1.2-rc.2, I'm still seeing a white page reload 😭

Could you use the fallback prop from I18nProviderClient to make sure it's coming from next-international? Otherwise, I think we'll need a minimal reproduction as the issue is fixed for 3 of us.

Ok, here is a minimal reproduction issue: https://github.com/hipdev/next-international-issue And the preview: https://next-international-issue.vercel.app/en

This is the expected behavior I want. Here is using next-international@1.0.1: https://www.uhh.club/

Now the issue with next-international@1.1.2-rc.2 https://uhh-git-test-next-international-issue-juliancho.vercel.app/

Just in case, the language switcher is in the bottom right.

I tried with codesandbox but looks like the middleware is not supported, or maybe I'm missing something https://codesandbox.io/p/sandbox/goofy-roentgen-rch36n?file=/middleware.ts:17,3

Thank you for the hard work on this amazing library @QuiiBz , I hope this information helps.

QuiiBz commented 10 months ago

Thanks a lot for the reproduction and deployments. I'm unable to reproduce the issue every single time I switch the locale, only around once every 10-20. Do you also have this behavior?

https://github.com/QuiiBz/next-international/assets/43268759/a8ad14ad-98e1-4c6e-9113-cdeddf294b58

hipdev commented 10 months ago

Thanks a lot for the reproduction and deployments. I'm unable to reproduce the issue every single time I switch the locale, only around once every 10-20. Do you also have this behavior?

Screen.Recording.2023-10-11.at.20.19.56.mov

With the minimal reproduction example is like that, with my page is happening more often, maybe because it's with more components?

gustaveWPM commented 10 months ago

Thanks a lot for the reproduction and deployments. I'm unable to reproduce the issue every single time I switch the locale, only around once every 10-20. Do you also have this behavior?

I've tried setting ridiculously large vocabulary objects to stress test and see if it wasn't coming from potentially too systematic calls to flattenLocales and it doesn't seem to be coming from there. (I admit I didn't put very much emphasis on depth levels. Btw, I should have used this instead of a poorly hand made JSON generator: https://fakerjs.dev/)

Personally, I can't reproduce the bug on my machine with this code. :/

https://github.com/gustaveWPM/next-international-issue/commit/a1c2840529a0c75815b9bc0a0a7ba7be6137bfa4

Where could I have the source code of https://uhh-git-test-next-international-issue-juliancho.vercel.app/, just to investigate a bit more and try some dumb and harder stress tests?

I'm also wondering if the source code doesn't overuse client-side rendering a little and couldn't rely a little more on SSG... idk.

QuiiBz commented 10 months ago

I'll go ahead and close this initial issue, as the current behaviour is already much better than the previous release. I've published next-international@1.1.2 which includes #234 and more small fixes!

@hipdev feel free to open a new issue so we can take a more in-depth look at your specific problem.

gustaveWPM commented 10 months ago

Hello @hipdev,

I was thinking about your problem today (the one related to NextUI, which we didn't dig so far: https://uhh-git-test-next-international-issue-juliancho.vercel.app/).

Has it been resolved since then?

I was wondering if it might be due to your components tree, which maybe nest the i18nProviderClient a bit "too early". In that case, when the language changes, all its children are maybe re-rendered (including NextUIProvider, and then cause an "horrible" white-page flickering). Maybe that's what is causing your problem?

Likewise, if we just go from:

<I18nProviderClient locale={params.locale}>
  <NextUIProvider>
    <Toaster />
    {children}
  </NextUIProvider>
</I18nProviderClient>

(As written in https://github.com/QuiiBz/next-international/issues/227#issuecomment-1757924579)

To:

<NextUIProvider>
  <I18nProviderClient locale={params.locale}>
    <Toaster />
    {children}
  </I18nProviderClient>
</NextUIProvider>

Does it change something?

I hope you don't mind that I'm asking this question even though the current issue is closed. As I might start using NextUI myself in a few days, I'm curious.

If this doesn't solve the problem, I'd be interested in looking into it in more detail, in a new issue. :slightly_smiling_face:

hipdev commented 10 months ago

Hello @hipdev,

I was thinking about your problem today (the one related to NextUI, which we didn't dig so far: https://uhh-git-test-next-international-issue-juliancho.vercel.app/).

Has it been resolved since then?

I was wondering if it might be due to your components tree, which maybe nest the i18nProviderClient a bit "too early". In that case, when the language changes, all its children are maybe re-rendered (including NextUIProvider, and then cause an "horrible" white-page flickering). Maybe that's what is causing your problem?

Likewise, if we just go from:

<I18nProviderClient locale={params.locale}>
  <NextUIProvider>
    <Toaster />
    {children}
  </NextUIProvider>
</I18nProviderClient>

(As written in #227 (comment))

To:

<NextUIProvider>
  <I18nProviderClient locale={params.locale}>
    <Toaster />
    {children}
  </I18nProviderClient>
</NextUIProvider>

Does it change something?

I hope you don't mind that I'm asking this question even though the current issue is closed. As I might start using NextUI myself in a few days, I'm curious.

If this doesn't solve the problem, I'd be interested in looking into it in more detail, in a new issue. 🙂

Hey @gustaveWPM !, I tried it and still had the same issue. Finally I ended using next-intl and the issue is solved.

QuiiBz commented 10 months ago

I tried it and still had the same issue. Finally I ended using next-intl and the issue is solved.

Sorry to hear that, this specific issue seemed to be fixed for many of us in the latest release. I guess it's too late now, but again, feel free to open an issue with more details of your setup.

gustaveWPM commented 10 months ago

I tried it and still had the same issue. Finally I ended using next-intl and the issue is solved.

Sorry to hear that, this specific issue seemed to be fixed for many of us in the latest release. I guess it's too late now, but again, feel free to open an issue with more details of your setup.

Hello!

I'm also seeing very slight unwanted apparitions of the Suspense, and I think more generous caching (with the FlattenLocales call included during caching, as I mentioned earlier) might fix this.

QuiiBz commented 10 months ago

flattenLocales isn't related to any suspense boundary, there might be something else in your app that triggers it (e.g. data fetching)

gustaveWPM commented 10 months ago

flattenLocales isn't related to any suspense boundary, there might be something else in your app that triggers it (e.g. data fetching)

Hmmm, I haven't done any data fetching for now. I should try to clone again the lib and edit it with some of my horrible lines of code to see...