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

Another approach for SSR with multiple namespaces #711

Closed
klis87 opened this issue Feb 2, 2019 · 15 comments
Closed

Another approach for SSR with multiple namespaces #711

klis87 opened this issue Feb 2, 2019 · 15 comments

Comments

@klis87
Copy link

@klis87 klis87 commented Feb 2, 2019

Is your feature request related to a problem? Please describe.
Current suggestion for SSR is in my opinion not complete and optimal, especially when having multiple namespaces. In https://react.i18next.com/misc/serverside-rendering#loadnamespaces-helper it is mentioned we could use match from React-router to know which namespaces are needed. This is really problematic because:

  • it works only with legacy router version 3
  • not everyone uses react-router, especially when using redux and pure redux routers like redux-first-router
  • even if you use React-router 3, then still this method assumes that children of Route components won't need dedicated namespaces, which forces us to decorade only route components with namespaces hocs, which is unflexible and often not optimal

Describe the solution you'd like
I suggest a simpler solution for SSR:

  • we should mention in the docs about possibility to preload all namespaces for all languages at server boot, on server it has to be done only once anyway, so it doesn't hurt to do it immediately
  • then, withNamespaces for a given React component on the server won't be actually used to load a given namespace for a given language (yes, it will load namespaces but this would be cached already), it would be used to know which namespaces are needed to be passed to the client in initialI18nStore

How we could achieve it? For each request we could create an object like set or [] (set would be better but maybe you would prefer array as set is not implemented in every browser) and pass it to provider like:

const usedNamespaces = new Set();
<I18nextProvider i18n={req.i18n} initialLanguage={language} usedNamespaces={usedNamespaces}>

Now, usedNamespaces could be added to i18next context, and in proper i18next hocs/components, like withNamespaces, usedNamespaces set could be mutated with used namespace/namespaces in a given component, for example usedNamespaces.add('someNamespaceName').

Then, after components are rendered, we could read usedNamespaces to see which namespaces were used for this specific request. Then, we can used this info to pass optimal initialI18nStore to the client.

Describe alternatives you've considered

  • using one namespace - not scalable
  • passing all namespaces to initialI18nStore - defeats the purpose of using namespaces in the first place
  • keeping separate configuration which namespaces are required for all routes - requires keeping separate config, while this can be easily computed as suggested automatically

Additional context
N/A

@jamuhl

This comment has been minimized.

Copy link
Member

@jamuhl jamuhl commented Feb 2, 2019

That's not a suggestion - it's one of the things you can do. It's impossible to write a documentation for every possible combination of things existing out there. So all the SSR stuff is giving hints, directions how to approach the problem.

The express-middleware has the preload set prominently: https://github.com/i18next/i18next-express-middleware#wire-up-i18next-to-request-object

Suggestion is heading to next-i18next: https://github.com/isaachinman/next-i18next

There is reportNS for what you like to achive i guess: https://github.com/i18next/react-i18next/blob/v9.x.x/src/I18nextProvider.js#L14 --> https://github.com/i18next/react-i18next/blob/v9.x.x/src/NamespacesConsumer.js#L44 (currently not documented --- and old v9 : same exists in v10 - but not yet had time to document that stuff).

PR to docs is more than welcome

@klis87

This comment has been minimized.

Copy link
Author

@klis87 klis87 commented Feb 2, 2019

@jamuhl thx for quick response, I will check whether I can connect all the dots. If yes, I could make a PR to docs, but I am wondering whether it wouldn't be better to wait until vs 10 is ready, which I guess is coming quite soon.

@klis87

This comment has been minimized.

Copy link
Author

@klis87 klis87 commented Feb 2, 2019

I just checked undocumented reportNS callback to provider, that's awesome, exactly what I needed, even more flexible than I suggested!

Please let me know if version 10 is going to be backward compatible regarding I18nextProvider, if yes, I could add doc PR plus I could add it to Typescript interface I18nextProviderProps, if not, I will wait until v 10 is there and do it then

@jamuhl

This comment has been minimized.

Copy link
Member

@jamuhl jamuhl commented Feb 2, 2019

v10 has a Provider: https://github.com/i18next/react-i18next/blob/master/src/I18nextProvider.js which only acts as a way to pass i18n instance down via context.

All useTranslation, withTranslation, Translation (hook, HOC, render prop) which are used to get t function (and load namespaces) report namespaces on the fly and that information can be read by https://github.com/i18next/react-i18next/blob/master/src/context.js#L36

@klis87

This comment has been minimized.

Copy link
Author

@klis87 klis87 commented Feb 2, 2019

I might be missing something, but from what I can see, this solution will cause issues compared to reportNS solution - usedNamespaces is global variable. By global I don't mean like window.usedNamespaces, but its state will be shared across all requests on the server. We need to have a new independent object for each provider instance. In current solution usedNamespaces will quickly contain all possible namespaces and stay like this for each request until server is reset. In theory we could clear it during i18nprovider initialization, but then it will still cause issues for React async renderers. Current solution reportNS in v 9 doesn't have those issues.

@jamuhl

This comment has been minimized.

Copy link
Member

@jamuhl jamuhl commented Feb 2, 2019

@klis87 you're right...will fix this asap. Best might be the same as in the last version - passing a function via context.

@klis87

This comment has been minimized.

Copy link
Author

@klis87 klis87 commented Feb 2, 2019

@jamuhl Yeah, I think reportNS is really good approach, it is super flexible and it will suits any use case.

@jamuhl

This comment has been minimized.

Copy link
Member

@jamuhl jamuhl commented Feb 3, 2019

might get to in on monday...will ping you as soon it's published

@jamuhl

This comment has been minimized.

Copy link
Member

@jamuhl jamuhl commented Feb 4, 2019

in react-i18next@10.0.0-alpha.3 it will append a https://github.com/i18next/react-i18next/blob/master/src/context.js#L29 to the i18n instance passed in (global one or the req.i18n via provider: https://github.com/i18next/react-i18next/blob/master/src/useTranslation.js#L35)

so you will be able to read them out like: https://github.com/i18next/react-i18next/blob/master/test/useTranslation.usedNamespaces.spec.js#L37

hope that works for you ?!?

@klis87

This comment has been minimized.

Copy link
Author

@klis87 klis87 commented Feb 4, 2019

@jamuhl I don't think It will. From what I understand i18n is a singletone object, which is perfect for server side rendering - we can just once load a namespace and share it for multiple requests - it doesn't make sense to load them for each request as translations are static, we can preload everything once, components will have access to anything, and as a last SSR step, we can filter translations to pass to client as initial i18nStore only those translations that were actually used, so they won't be downloaded again by XHR backend

Looking at this code, what would happen is that for the first useTranslation, new instance of ReportNamespaces will be appended to i18n instance. For any future useTranslation call, this will already exist, for the same request and for independent requests too. So it seems like this solution has the same issue like previous v10 solution.

In my opinion we have 2 solutions:

const usedNamespaces = {};
const provider =<I18nextProvider i18n={req.i18n} initialLanguage={language} usedNamespaces={usedNamespaces}>
// ... render components
// read usedNamespaces to see which namespaces were used for THIS request,
// this would work as we create new usedNamespaces for each request
@jamuhl

This comment has been minimized.

Copy link
Member

@jamuhl jamuhl commented Feb 4, 2019

on server side it should not be the global singleton - if doing so i promise you will run into concurrency issues in production where one request overwrites the language of another...

the express middleware creates a cloned instance on req.i18n (sharing the loaded namespaces but not the set language) -> that should be passed to I18nextProvider

@klis87

This comment has been minimized.

Copy link
Author

@klis87 klis87 commented Feb 4, 2019

Indeed, lang is property of i18n instance too, so mixed languages would be guaranteed :)

So I confirm it will work then! Thx for the quick fix and generally for awesome i18n library! I spent much time checking all alternatives and no one is even close to i18next!

I will wait until v 10 is released and create a PR for SSR docs

@jamuhl

This comment has been minimized.

Copy link
Member

@jamuhl jamuhl commented Feb 4, 2019

Thank you for taking the time - checking i18next and it's potential - most people just see other solutions are simpler - or comparing size of the library with their own solution (not providing more than a key lookup).

@klis87

This comment has been minimized.

Copy link
Author

@klis87 klis87 commented Feb 4, 2019

but there are also big libraries like react-intl of Yahoo, much more popular - currently 9k + stars, despite the fact that they don't even have proper loader of translations, not to mention namespaces (at the time I checked it at least), which generally is also harder to use than i18next

But what can you do, anything done by big company has marketing advantage. I wish you luck getting more and more popularity, you deserve it!

@jamuhl

This comment has been minimized.

Copy link
Member

@jamuhl jamuhl commented Feb 6, 2019

ok 16.8 was released .... will close this now and publish v10.0.0 to npm 🔥

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.