next-i18next: Context is broken with react-tree-walker

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:

Context is poorly supported in general.
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.

About this issue

Original URL
State: closed
Created 6 years ago
Comments: 27 (25 by maintainers)

Most upvoted comments

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.

revskill10 on Dec 17, 2018

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

isaachinman on Dec 17, 2018

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.)

isaachinman on Dec 17, 2018

@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.

isaachinman on Dec 17, 2018

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.

isaachinman on Dec 17, 2018

Yup.

Given the choice between:

Tree walking, reduced performance
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 on Dec 16, 2018