Context is broken with react-tree-walker

[isaachinman]

Creating this issue so that we have one place to discuss potential solutions.

I recently added react-tree-walker to this package so that we can traverse React component trees to determine the exact namespaces required to render that tree correctly. This allows us to do SSR in the most efficient way possible.

However, it seems that there are several context-related issues with react-tree-walker:

1. Context is poorly supported in general.
2. Users are passing custom context, which will end up being missed by the shallow render inside the app-with-translation HOC.

Instead of going down this road, I would really prefer if we can come up with a different way to do tree traversal that does not depend upon context. All we need to do is traverse a tree, checking for props.ns. If props.ns exists, we need to add it to an array. That's it.

Does anyone have any good ideas?

Otherwise, we'll probably need to revert to sending down all namespaces to the client. This is not ideal, especially for (very) large apps.

[kachkaev]

This might be stupid idea, but can we just traverse the tree until the first occurrence of withNamespaces and use those values as reference? Users will be advised that a top-level withNamespaces for a page has to enumerate all namespaces that are going to be used in the initial render. Or is even this impossible if someone's using context inside _app.js HOCs?

[isaachinman]

No, that's not what we'll do. Older versions of this project, which existed only as "examples" in the react-i18next repo, took a similar approach. I'd rather users be able to wrap any React component with that component's exact namespace deps, and have the tree deps worked out at runtime.

There are many cases where a tree's deps could change:

{loggedIn ? <LoggedIn /> : <LoggedOut />}

Wherein two very different trees with different deps can be rendered, and the parent shouldn't have to declare all namespaces for both possible renders.

[isaachinman]

Ideally I'd like to traverse the tree without rendering, but I'm not sure if such a thing is even possible.

[kachkaev]

My intuition suggests that something similar to Apollo's getDataFromTree is the only option really when it comes to proper tree traversal. Without a full-blown rendering there seems to be no way to resolve things like {loggedIn ? <LoggedIn /> : <LoggedOut />}, because loggedIn can come from context and it is not know which if the two components will be a child until render() is called for real.

Using an equivalent of getDataFromTree will mean a somewhat significant reduction in performance, which is not ideal. For projects that already use Apollo, each page will have to render three (or four?) times on the server – not sure that Google's Lighthouse will mark website performance very high in this case 🤔 As to a developer, my choice between the two evils will probably fall on maintaining an explicit list of namespaces (especially if the project is expected to be under a high load). However, I can't speak for everyone of course.

[revskill10]

This is my thoughts on this issue.
First of all, this is next-i18next , not react-i18next, so i'd love to have an implemenation which uses next.js features, like getInitialProps.
In my projects which uses Apollo, getDataFromTree always get around 1.6-2x slower performance in comparison to getInitialProps .
So the ideal solution might be something explicit like getInitialProps to resolve namespaces.
So in this case, every page which has a namespaces in their getInitialProps returning will get its namespaces to load.

[Nelrohd]

In case of someone wonders: Redux wasn't working with react-tree-walker so I had to revert to 0.9.0 to make my app working again.

Note: I'm posting it anyway. I saw the same suggestion from @isaachinman after I checked the changelog.

[Clebal]

@isaachinman I've just read what you commented. It's true that using 0.9.0 there's no problem. Take the time you need to fix it. Thank you for your rapid response and your patience.

[kachkaev]

Otherwise, we'll probably need to revert to sending down all namespaces to the client.

That's probably a very expensive compromise to make. When I was using react-intl, the main reason for me to switch to i18next was exactly the ability to have multiple namespaces and only fetch those that make sense on a given page. Pushing all i18n data to the client can add hundreds of kilobytes or even megabytes of gzipped javascript to the initial load and this number will have to double-up when a fallback locale needs loading. As a consequence, the scope of next-i18next will be limited to a subset of small apps, which would be a shame.

I understand that requiring to list all namespaces in the top-level withNamespaces (and thus leveraging getInitialProps) is not ideal. What is good about this approach though is that the devs could be given some handy tooling that would facilitate the correct usage. When a namespaces is forgotten and so the i18n store is not full during the first client-side render, next-i18next can throw an exception in development, asking to add particular namespaces (now the app just blinks). When there are too many namespaces, an optional warning can be shown in the browser console (this is a possible sign of translation over-fetching). When a page is dependent on too many namespaces and all of them are making up a rather long array, this is a good sign of that code refactoring is needed. Needless to say, this approach is way better for end users, because page rendering is much faster.

[isaachinman]

So the ideal solution might be something explicit like getInitialProps to resolve namespaces.
So in this case, every page which has a namespaces in their getInitialProps returning will get its namespaces to load.

This again misses the point... Having to declare namespaces explicitly is not something I want to force users to do. It will be a pain to maintain, and as shown in examples above, there are many cases where you'd end up loading namespaces you don't even need.

Pushing all i18n data to the client can add hundreds of kilobytes or even megabytes of gzipped javascript to the initial load and this number will have to double-up when a fallback locale needs loading.

It's JSON, not JavaScript, and it does not need to be executed. It is not my intention to have this package send down all translations - I really want to support "code splitting" for namespaces. That being said, I have built small projects where all namespaces are sent down initially. It's a fine approach if you understand what you're doing.

handy tooling that would facilitate the correct usage

It is my opinion that we need a robust solution in the core source code here, not bandaid tooling.

I am still actively thinking about how we can fix this problem. I have a hunch it's possible without tree walking, but it's an extremely tricky problem and will take time. Thank you everyone for your patience.

[isaachinman]

I've created a branch called namespace-dep-order-of-execution.

If you clone it and do yarn && yarn run-example:prod and load up localhost:3000, you'll see that the withNamespaces calls happen before the getInitialProps calls. That is because of the specific function signature of withNamespaces, and might be something we can use.

Basically - the data we need is "there" and ready to be consumed, I just have no idea how. It would be great if we could relate this namespace data from within the initial withNamespaces call to either:

1. The NextJs request ID (if such a thing exists)
2. The unique URL (from Express)

Either way would allow us to save the namespaces as withNamespaces executes, and then later reference the deps inside getInitialProps.

Assuming each unique URL always has the same namespace dependencies (I don't see how it couldn't, but please tell me if your use case is different), we could end up saving a cache-map of deps:

"https://www.myapp.com/cart": ["common", "cart"],
"https://www.myapp.com/cart?affiliate=true": ["common", "cart", "affiliate"]

Otherwise, if there is such a thing as a NextJs request ID, we would simply create the deps on an object, and then clean up that request ID once it's been served.

So, I need help making that link between withNamespaces and getInitialProps and would appreciate any help I can get!

[kachkaev]

Assuming each unique URL always has the same namespace dependencies (I don't see how it couldn't, but please tell me if your use case is different), we could end up saving a cache-map of deps

From your own recent comment:

There are many cases where a tree's deps could change:

{loggedIn ? <LoggedIn /> : <LoggedOut />}

[isaachinman]

Yup.

Given the choice between:

1. Tree walking, reduced performance
2. Manually declare namespace deps at page-level

What does everyone want? If we're not going to proceed with tree walking, there's no point trying to fix these context issues.

[isaachinman]

After having a think about it, I am going to end up asking users to define required namespaces on the page level after all. Unfortunately it seems like any other solution is going to make life unpredictable/buggy for end users, and extremely difficult for maintainers.

Most likely I'll be expecting users to return a namespacesRequired array from pageProps on their page-level components. If this array isn't returned, we'll send down all namespaces by default to preserve SSR.

Changes incoming today.

[kachkaev]

What if a modified version of withNamespaces() simply returns a component with getInitialProps method, which automatically populates namespacesRequired? This method will only be called for a page-level hoc because it's Next-specific.

Sending down all namespaces if some condition is not met does not solve the problem, it simply hides it. It may seem that this "default" mode can protect a newcomer from some extra cognitive load, but in fact it will make things more complex. Rather than learning a simple rule (i.e. include all namespaces in a top-level withNamespaces), they'll end up with questions like What is the difference between the two modes? At what point should I switch from one mode to another? etc.

[isaachinman]

What if a modified version of withNamespaces() simply returns a component with getInitialProps method? This method will only be called for a page-level hoc because it's Next-specific.

This is how the old example used to work, and is not something I want to do. A HOC should do one thing - they're already hard enough to reason about. The withNamespaces HOC shall declare a component's namespace dependencies. The new namespacesRequired array shall inform the server which namespaces to send down to the client. It would be confusing if withNamespaces had to perform both tasks.

Sending down all namespaces if some condition is not met does not solve the problem, it simply hides it.

No. It uncontroversially does solve the problem of SSR localisation. Sending down all namespaces with the initial request is not inherently bad. Doing full network roundtrips for a 5kb JSON file isn't inherently good. It will all depend on your use case. This approach gives users the option to "code split" their translations based on route if they feel it's appropriate, but prevents things from being broken out of the box.

[isaachinman]

Tree walking approach abandoned with v0.11.0 (5570346). See README for updated docs.

[MathiasKandelborg]

Having a SSoT for the different settings, on a per-app basis really should be a given and therefore it makes sense to have a file that dictates which combintations of namespaces are used.

For context: I'm implementing i18n, material-ui, Apollo and GA at the same time, my plan is to eventually make it OSS as a full-stack 'starter kit', as it's coupled with a local Prisma backend and I've created a whole CLI around it all. My point is, that basically everything works, with configurability and I'd love to have to explicitly give my settings or have (documented) defaults be the default. Otherwise I'll have to go back to own implementation for full flexibility for everybody.

The issue got closed as i was writing this, I'm glad I didn't have to argue but would still like to post an opinion.

[isaachinman]

@MathiasKandelborg Thanks for adding your opinion here. Although, I didn't really understand it... You're saying you are in support of declaring namespacesRequired?

User opinions are very much appreciated, and in fact influence the direction of this project as it's still quite young and there are some core issues to iron out.

[MathiasKandelborg]

I'm sorry for being unclear, I deleted some of the text, as the issue got closed, as it would be empty arguments. I tried cutting it down to the relevant parts but I'm notorious for not being particularly explicit.

Yes, I'm in favor of using namespacesRequired, actually so much so that I just cheered when I realized that I have a legit reason to use getInitialProps on all my pages!

Basically, my thoughts are: in order to accommodate as many as possible... one of the best approaches I've encountered has been to:

1. Have defaults for all settings
2. Use the most common/least strict settings as default
3. Clearly state (console.log & regular errors) how something is done, if it could/has caused confusion

E.g. in dev-mode I'd like to always get notified if no namespacesRequired is detected and explain that all namespaces then get sent - the icing on the cake would be getting notified along with a link to an issue or a webpage explaining the whole thing in detail.

[isaachinman]

E.g. in dev-mode I'd like to always get notified if no namespacesRequired is detected and explain that all namespaces then get sent - the icing on the cake would be getting notified along with a link to an issue or a webpage explaining the whole thing in detail.

7f7fc63

[kachkaev]

@isaachinman that's a useful addition, thanks! Also WDYT of warning users of missing namespaces when they've defined too few? At the moment, when a required namespace is missed, the page will flash, but it won't be clear why. A warning can be added when i18next renders things on the client for the first time (or within ≈1-5 seconds) and notices a need to fetch more namespaces. Of course, this can be a dev-only feature too.

[isaachinman]

@kachkaev That's a more complicated proposal, because we'd need to hook into withNamespaces, and we'd need to ensure that we're inside a hard load and not a route transition. I'd accept a PR for it if it seemed like a sound implementation but it's not something I'll personally look into at the moment.

[revskill10]

Thanks @isaachinman for this direction. I love this very much, as it's very easy to integrate i18n to any existing Next.JS project without breaking anything. Because it's just getInitialProps after all, no magic hidden for user to debug.

[MathiasKandelborg]

Oh, I just thought of something! Opt-out of the message, "some people like to see the world burn and hate to get reminded to get a fire extinguisher all the time".

It would be great to turn off the messages via a boolean setting. This would enable the exact dev experience one would like. Anyways, Opt-out seems like a good solution to this problem none has had yet.

[isaachinman]

@MathiasKandelborg Feel free to open an issue with a feature request. If anything, I'd want to start a "strict mode" where we throw errors and warnings, and it's "all or nothing". I don't want to start putting flags for every single thing.

[MathiasKandelborg]

Hmm, I'll see if I'm able to find some time to contribute (and can figure things out haha). This is a core feature of my own work and I'd love to help if possible.

I'll be sure to add a feature request no matter what, I'm just on my way out atm so it's first due later/tomorrow.

The strict mode seems like a great thing to have. I personally always go all-in on safeguarding everything I do (besides tests because I'm lazy and do crazy combinations with my code, Like apollo+mui+i18n+formik which seems daunting to test, also, most is tested anyways).

I love to learn how tech works while using it and coding with a strict mode on, often helps with proper understanding and lots of times even requires reading up on how things should work together. Being able to do this with i18n would be great!

[isaachinman]

If you open an issue, I can point you in the right direction. It'll just be a matter of adding a new bool to the config, and then protecting the console.warn with an if statement. (Actually, might be cool to override the console.warn function entirely, and hook into the config.)