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

feat(next-international): only trigger suspense on first load #234

Merged
merged 6 commits into from
Oct 12, 2023

Conversation

QuiiBz
Copy link
Owner

@QuiiBz QuiiBz commented Oct 9, 2023

Relates to #227

@QuiiBz QuiiBz added the enhancement New feature or request label Oct 9, 2023
@vercel
Copy link

vercel bot commented Oct 9, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
next-international ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 12, 2023 6:31am

let clientLocale: any = localesCache.get(locale);

if (!clientLocale) {
clientLocale = use(locales[locale as keyof typeof locales]()).default;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not related to your changes directly, but what if locales[locale] is undefined, should we handle this case instead of throwing type error like locales[locale] is not a function?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah absolutely, this can be done in a separate PR because it's used in multiple places.


if (!clientLocale) {
clientLocale = use(locales[locale as keyof typeof locales]()).default;
localesCache.set(locale, clientLocale);
Copy link
Contributor

@gustaveWPM gustaveWPM Oct 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can use a Proxy to always flatten the locales when set is called.

Something like:

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');

ℹ️ If the overhead due to using a Proxy is significant, employing a more straightforward utility function (like populateCache(languageFlag, currentLocaleVocabObj), which calls flattenLocale, then set) might be a better solution.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related to #234 (comment) - using a proxy is overkill and won't be needed later.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, sorry. I wasn't aware of the additional complexity (in terms of intermediate calculations) that a Proxy would introduce. I looked at benchmarks afterwards when I made my suggestion, and felt a bit ashamed.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine, no worries! Thanks for your help and suggestions.

Copy link
Contributor

@gustaveWPM gustaveWPM Oct 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, I was thinking that using a Proxy might have been a good idea to get rid of the problem of "using flattenLocale in many places".

Unfortunately, the overhead of proxies seems really substantial and I wasn't that aware of it (nor did I have any idea if this overhead is really annoying or not for this "usecase").

if (!clientLocale) {
clientLocale = use(locales[locale as keyof typeof locales]()).default;
localesCache.set(locale, clientLocale);
}

const value = useMemo(
Copy link
Contributor

@gustaveWPM gustaveWPM Oct 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the flattenLocales calls should be avoided in the useMemo here.

fallbackLanguage could be computed earlier (and could even be cached too, already flattened).
Maybe like this, using a unique Symbol?

const FallbackLng: unique symbol = Symbol();
const obj = {
  [FallbackLng]: flattenLocale({'something': 'funny here'})
};

clientLocale may be already flattened if we flatten the locales when we cache them, on the set function call via a Proxy, or a more straightforward utility function (like populateCache(languageFlag, currentLocaleVocabObj), which calls flattenLocale, then set).

I think this useMemo should be as primitive/pure as possible, like:

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

Maybe it could be even simplified with just a useState based on locale, which is a JS primitive and seems to be the most interesting value to track since it is the most significant state change here? Idk.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we'll have more work to do with flattenLocale() to dedupe and call it less often. It's used in many places in both the App & Pages lib. We definitely want to tackle this in another PR.

Comment on lines +33 to 39
locales[newLocale as keyof typeof locales]().then(module => {
localesCache.set(newLocale as string, module.default);

push(`/${newLocale}${pathWithoutLocale}${finalSearchParams}`);
refresh();
});
};
Copy link
Contributor

@gustaveWPM gustaveWPM Oct 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could use our Cache here (using get) instead of process the promise everytime? Overusing promises bothers me a bit. I don't find it very natural.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After the first import(), the next one won't re-fetch again the locale file because it's in the cache. The overhead of running the promise is very minimal.

Copy link
Contributor

@gustaveWPM gustaveWPM Oct 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, okay. Even if the promise may be never considered as "Consumed" in the code, rely on the browser cache is safe enough.

Copy link
Contributor

@gustaveWPM gustaveWPM Oct 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just thought again about my suggestion. To be honest, it made sense in my approach where I called flattenLocales when caching. But given the current implementation choices, it doesn't make much sense, indeed.

Please, ping me when the PR consisting of refactoring calls to the flattenLocales function will be drafted. I haven't looked at the lib code much, but from what little I've read of it to help achieve better behaviour when switching languages on the client side, I've really liked it, both in terms of the typing and the way the code is structured and concise.

I'm curious.

@QuiiBz QuiiBz marked this pull request as ready for review October 12, 2023 06:42
@QuiiBz QuiiBz merged commit 4fcbb84 into main Oct 12, 2023
5 checks passed
@QuiiBz QuiiBz deleted the fix/cache-client-locales branch October 12, 2023 06:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants