search

QuiiBz / next-international

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

[1.1.0-rc.x] Screen flickering on next/prev #208

Closed gustaveWPM closed 11 months ago

gustaveWPM commented 11 months ago

When I use: "next-international": "1.1.0-rc.1" I get screen flickering when I use the "next" and "previous" buttons of my browser.

https://github.com/QuiiBz/next-international/assets/118654172/bd8c89a8-22e7-4b55-962a-fbefea0286b3

This bug does not occur when I use: "next-international": "^1.0.1"

QuiiBz commented 11 months ago

Please provide a reproduction so I can debug the issue.

gustaveWPM commented 11 months ago

Please provide a reproduction so I can debug the issue.

Doesn't this happen with the examples from the repo?

QuiiBz commented 11 months ago

No, but the example is very minimal and doesn't have any CSS/complex layout. So I'm not sure if it's because of the Suspense boundary used by next-international or not.

gustaveWPM commented 11 months ago

Hmm, wait a minute please.

gustaveWPM commented 11 months ago

Idk if it is helpful? https://github.com/Tirraa/dashboard_rtm/tree/next-international-1.1.0-rc.1-screen-flickering-demo

QuiiBz commented 11 months ago

You're using I18nProvider twice (in src/app/[locale]/layout.tsx and src/components/shared/misc/HtmlElement). I've kept only the one in HtmlElement and moved it inside the body to avoid wrapping both html & body.

I added a fallback prop to I18nProviderClient and my assumption was correct: the fallback is triggered when navigating prev/next, and so replaces the whole layout. I'm not sure if this is the intended behaviour of React's use() hook.

gustaveWPM commented 11 months ago

You're using I18nProvider twice (in src/app/[locale]/layout.tsx and src/components/shared/misc/HtmlElement). I've kept only the one in HtmlElement and moved it inside the body to avoid wrapping both html & body.

Oops! You're right. I messed up when replacing useCurrentLocale with params.locale in my <html> element. Thanks.

I added a fallback prop to I18nProviderClient and my assumption was correct: the fallback is triggered when navigating prev/next, and so replaces the whole layout. I'm not sure if this is the intended behaviour of React's use() hook.

:/ Maybe the locales should be cached when lazy-loaded?

gustaveWPM commented 11 months ago

https://youtu.be/T3m-MZkuadU?t=544 Maybe this?

QuiiBz commented 11 months ago

I've released this change in next-international@1.1.0-rc.3: aa19612 (#205)

It seems to work but also seems a bit hacky.

gustaveWPM commented 11 months ago

It seems to work but also seems a bit hacky.

I tried it, and it seems that it works perfectly. No more screen flickering, still no issue in the staticly generated HTML files ( https://github.com/QuiiBz/next-international/issues/190 ).

I've released this change in next-international@1.1.0-rc.3: aa19612 (#205)

I don't understand the try/catch block in this commit.

gustaveWPM commented 11 months ago

Well... If I refresh the page, and then use next/prev buttons, the screens flickering are back. :/

(It only occurs when I browse a new page with the "prev" button in this scenario.)

QuiiBz commented 11 months ago

I don't understand the try/catch block

React's cache() function isn't available on all "environments" (node, dom...), and throws an error if used in one:

Screenshot 2023-10-01 at 19 21 58

I added a try catch to fallback to loading the locale as before, without using cache().

the screens flickering are back

Just noticed that, but it seems to only "flicker" (= trigger the Suspense' fallback) on the first forward/backward navigation. Going again to the same page doesn't cause a "flicker". This is probably the correct behaviour since the cache is busted because of the refresh.

gustaveWPM commented 11 months ago

I don't understand the try/catch block

React's cache() function isn't available on all "environments" (node, dom...), and throws an error if used in one:

Screenshot 2023-10-01 at 19 21 58

I added a try catch to fallback to loading the locale as before, without using cache().

the screens flickering are back

Just noticed that, but it seems to only "flicker" (= trigger the Suspense' fallback) on the first forward/backward navigation. Going again to the same page doesn't cause a "flicker". This is probably the correct behaviour since the cache is busted because of the refresh.

This is weird... The cache should "load" the same data, so there should be no flicker? :/

QuiiBz commented 11 months ago

Found the root cause: this useParams() seems to invalidate the cache somehow

https://github.com/QuiiBz/next-international/blob/e2821cd04791a56b9276a27eb6b5df33e514e990/packages/next-international/src/app/client/create-use-current-locale.ts#L9

I don't know how to fix this though. This also means we'll no longer need cache(), only use()

QuiiBz commented 11 months ago

I published next-international@1.1.0-rc.4 (see https://github.com/QuiiBz/next-international/pull/205/commits/ca9df6525455b2c47c4d658f6eb677de0084b97e) - it adds a required locale prop to I18nProviderClient that is the locale params segment.

gustaveWPM commented 11 months ago

I still get flashes when I refresh the page, then use the next/prev buttons. They seem a bit shorter.

Does the cache really changes nothing here?

gustaveWPM commented 11 months ago

Also, when I change the locale (using const changeLocale = useChangeLocale();, then changeLocale('it')) for the first time after loading the page in my web browser, I get a flash. Idk if it was the case before this RC?

QuiiBz commented 11 months ago

I still get flashes when I refresh the page, then use the next/prev buttons.

That is expected, similar to the explanation at the bottom. You shouldn't get any "flash" while navigating then going forward/backward during the same session (= not refreshing).

Does the cache really changes nothing here?

cache() is no longer used in the latest rc.

when I change the locale ... I get a flash

This is a drawback of using use(), which doesn't keep the previous value and immediately trigger the nearest Suspense boundary. On the other side, use() enables Static Rendering for Client Components.

gustaveWPM commented 11 months ago

Hello!

cache() is no longer used in the latest rc.

Ikr, and I'm curious to know if this doesn't change things for the worse.

which doesn't keep the previous value

So, why discontinue the use of cache()? I don't understand this point. :/

QuiiBz commented 11 months ago

cache() doesn't solve the issue when refreshing the page and going forward/backward. The latest commit essentially does the same thing as when using cache(), except that the code is cleaner and makes more sense.

To recap: the last issue is forward/backward navigation immediately after refreshing the page. Normal navigation and Static Rendering are both working as expected. Tbh, this last issue doesn't feel like a huge problem to me. I personally almost never refresh a website, then go forward/backward without navigating to a page.

gustaveWPM commented 11 months ago

cache() doesn't solve the issue when refreshing the page and going forward/backward. The latest commit essentially does the same thing as when using cache(), except that the code is cleaner and makes more sense.

Eeyup, I finally figured it out before I went to sleep, and finally I thought I'd misunderstood again.

Tbh, this last issue doesn't feel like a huge problem to me. I personally almost never refresh a website, then go forward/backward without navigating to a page.

I agree. But I think it might be worth to keep this problem in the issues, in case of a Eureka moment.

QuiiBz commented 11 months ago

Closing thanks to https://github.com/QuiiBz/next-international/pull/205, will be released during this week officialy.