Browser bundle includes namespaces that are unrelated to a given page

[kachkaev]

Not sure if it's an upstream bug or not, but I've noticed that the size of initialI18nStore grows as the website is warming up. Opening any page for the first time adds dependent namespaces to the source of all other pages viewed later. If a website consists of quite a few pages, the overall size of initialI18nStore can grow rather big and thus diminish the benefits of splitting translations into files. I'm quite sure that my projects did not suffer from this problem before I tried next-i18next, but switching to it also meant for me the upgrading of i18next.

Steps to reproduce

Worked both for 0.8.0 and the latest master)

1. cd example; yarn; yarn dev;
2. Open http://localhost:3000/
3. View page source, observe that initialI18nStore includes
   {"en": {"common": ..., "footer": ...}}
4. Go to http://localhost:3000/second-page and refresh the page (so that it got server-rendered)
5. Return to http://localhost:3000/, refresh the page again
6. View page source, observe that initialI18nStore includes
   {"en": {"common": ..., "footer": ..., "second-page": ...}}
   Expected content:
   {"en": {"common": ..., "footer": ...}}

[isaachinman]

Yes, I noticed this bug awhile back but haven't gotten around to it. Simply put, nsFromReactTree is being held in memory on the server instead of being cleared for each render. It's a simple fix: 0115cee.

Worth noting that you store you were receiving on /second-page was also erroneous in that it contained the common and footer namespaces when it should not have.

[kachkaev]

Thanks for a very quick fix @isaachinman 🎉 Would you be happy to release some new version so that I could give it a try? This can be 0.9.0-alpha.0 if you still want to play with something.

I wish I could just run your lib from master using yarn link, but as I mentioned previously, this does not work for some magic reason.

[isaachinman]

@kachkaev It would be fantastic if you could write some test cases to help this project develop and release quicker. I am nervous to release some of the recent changes without a proper second opinion.

[isaachinman]

See #18.

[kachkaev]

It's the end of the weekend and I'm running out of time to go for anything new, unfortunately 😪 I can help a bit with the tests next weekend if things go as planned. I would not worry too much about breaking things at such an early stage of the library lifecycle, especially when it comes to releasing alpha versions. Iteration over perfection 😉

[isaachinman]

It's not about perfection, but rather critical breaks. I'd rather not scare off new users. That would be very helpful if you do have time next weekend!

[isaachinman]

This approach is not going to work. In fact, the approach is fatally flawed to begin with - nsFromReactTree needs to be directly tied to the Express req object. Currently, concurrent requests and multiple duplicate requests break the initialI18nStore.

[isaachinman]

We need a way of to store each used namespace in a request-specific manner, and then load each namespace into the initialI18nStore.

Need to think on this, not sure yet how it will be possible.

[jamuhl]

using the req.i18n instance in that case https://github.com/isaachinman/next-i18next/blob/master/src/hocs/with-namespaces.js#L4 would solve it - as that instance is created per request

[isaachinman]

Hi @jamuhl - yes of course req.i18n is the beginning of the story. That's not the hard part.

The hard part is: how do we append an array onto req.i18n which we can push into from within the withNamespaces hoc (ie within a React tree) and also be able to access the filled array in the getInitialProps method of our appWithTranslation HOC?

I cannot think of a way to do this, yet.

It might be the case that the only reliable way to get the used namespaces for a React tree is to force a renderToString, which is terrible for performance.

[jamuhl]

Not sure but if remember it right the past sample had that built-in...

[jamuhl]

Anyway i'm sure you come up with an elegant solution

[isaachinman]

The past sample was broken in exactly the same way described here, because it took the same approach.

I appreciate your confidence! This one will be tricky.

[jamuhl]

Not sure but you got req.i18n there https://github.com/isaachinman/next-i18next/blob/master/src/hocs/app-with-translation.js#L57

And only issue is: not passing that here: https://github.com/isaachinman/next-i18next/blob/master/src/hocs/app-with-translation.js#L94

my guess would be initialProps handled so props.i18n would be req.i18n if set here: https://github.com/isaachinman/next-i18next/blob/master/src/hocs/app-with-translation.js#L78

but might be i understand it wrong and initialProps runs only on clientside....

[isaachinman]

Spent over an hour looking into this and didn't really get anywhere.

Finally caved in and asked for help. Hopefully @timneutkens is aware of something I haven't found - this might not even be specific to NextJs...

[isaachinman]

So far I've gotten as far as this:

const reactTree = (
  <I18nextProvider
    i18n={i18n}
    initialLanguage={initialLanguage}
    initialI18nStore={initialI18nStore}
  >
    <NextStaticProvider>
      <Component {...this.props} />
    </NextStaticProvider>
  </I18nextProvider>
)

const nsFromTree = []

const recurseTree = (component) => {
  if (component.type && component.type.namespaces) {
    nsFromTree.push(component.type.namespaces)
  }
  if (component.props && component.props.children) {
    recurseTree(component.props.children)
  }
}

recurseTree(reactTree)

This works, but only seems to go one level deep (in the example stopping at LoadNamespace(Homepage)). I don't yet know why that is, or how to traverse the whole tree. Never done this sort of thing with React before.

@jamuhl @timneutkens Any ideas?

[isaachinman]

Happy to report that I think I've found a much more sane, workable solution.

Fix comes with 2386bf2.

I finally stumbled across react-tree-walker, which seems to be well-written, well-maintained, and an absolute godsend for our use case here.

Instead of creating a modified withNamespaces HOC and trying to keep track of a component tree's namespace deps there (which is an inherently fraught idea), we will walk the component tree inside getInitialProps, as this will be request-specific, and we're provided with the page-level Component inside the first argument object, thanks to NextJs.

Now that we have the initialLanguage and can walk a component tree based on a specific request, we can reliably produce exactly the initialI18nStore required to render that specific request - nothing more and nothing less.

Performance is obviously the biggest concern here. During prod SSR of the example, I'm seeing walk times of 3ms, which isn't great considering how simple a tree it is.

Hopefully in the future, a method will solving this problem will come to my attention that completely avoids the issue of re-walking/re-rendering the tree. Until now, I am quite happy with this solution.

Another added benefit is that we no longer need to maintain a custom withNamespaces HOC, and can simply re-export directly from react-i18next.

[isaachinman]

@kachkaev Would you mind doing some quick testing to ensure that this solution does indeed solve the original problem.

We're coming ever closer to a production-ready release.

[isaachinman]

Released in v0.10.0.

[kachkaev]

Many thanks for looking into this @isaachinman! I'll try testing your changes but unfortunately can't promise to do this before the weekend. One closed-project in which I'm trying next-i18next has got pretty heavy pages, so it'd be interesting to estimate the overhead. What exactly were you measuring to get those 3ms result? If you still have that bit of benchmarking code, it'd be useful to know where it was placed to make the result comparison possible.

BTW is <Trans/> working in 0.10.0? If not, I'm afraid that none of my apps will render. All was fine in 0.8.0, but something broke in 0.9.0.

[isaachinman]

What exactly were you measuring to get those 3ms result?

Just printing a timestamp before and after executing. Do you have any large next-i18next codebases you can share publicly (or privately to me via email)?

Please open a bug issue for the Trans component if you feel it's the fault of next-i18next. We don't do anything specific with that component, though.

[kachkaev]

I don't have an opportunity to check <Trans /> from work, but you are free to add it to the example and figure out if it's broken in the latest version. Alternatively, I can do this myself later in free time.

If you are registered on GitLab, I can temporary invite you into a private project – just drop me an email with your login. My open-sourced mini-website also uses next-i18next, but React trees are rather small there.

[kachkaev]

I could not kick off my mini-website after upgrading from 0.8 to 0.10, unfortunately.

Here's the terminal output during SSR:

Warning: Rendering <Context.Consumer.Provider> is not supported and will be removed in a future major release. Did you mean to render <Context.Provider> instead?
Warning: Rendering <Context.Consumer.Consumer> is not supported and will be removed in a future major release. Did you mean to render <Context.Consumer> instead?
react-i18next:: You will need pass in an i18next instance either by props, using I18nextProvider or by using i18nextReactModule. Learn more https://react.i18next.com/components/ov
erview#getting-the-i-18-n-function-into-the-flow
TypeError: Cannot read property 'wait' of null
    at NamespacesConsumerComponent.render (/path/to/website/frontend/node_modules/react-i18next/dist/commonjs/NamespacesConsumer.js:213:33)
    at render (/path/to/website/frontend/node_modules/react-tree-walker/src/index.js:253:32)
    at render (/path/to/website/frontend/node_modules/react-tree-walker/src/index.js:157:40)
    at process._tickCallback (internal/process/next_tick.js:68:7)
TypeError: Cannot read property 'wait' of null
    at NamespacesConsumerComponent.render (/path/to/website/frontend/node_modules/react-i18next/dist/commonjs/NamespacesConsumer.js:213:33)
    at render (/path/to/website/frontend/node_modules/react-tree-walker/src/index.js:253:32)
    at render (/path/to/website/frontend/node_modules/react-tree-walker/src/index.js:157:40)
    at process._tickCallback (internal/process/next_tick.js:68:7)
TypeError: Cannot read property 'wait' of null
    at NamespacesConsumerComponent.render (/path/to/website/frontend/node_modules/react-i18next/dist/commonjs/NamespacesConsumer.js:213:33)
    at render (/path/to/website/frontend/node_modules/react-tree-walker/src/index.js:253:32)
    at render (/path/to/website/frontend/node_modules/react-tree-walker/src/index.js:157:40)
    at process._tickCallback (internal/process/next_tick.js:68:7)

Changes I've made: https://gitlab.com/kachkaev/website-frontend/merge_requests/6/diffs

All is fine on master 🤔

[isaachinman]

You should be importing the default export, not *.

[kachkaev]

That's what I was successfully doing in 0.8.0. In 0.10.0, import NextI18next from "next-i18next" gives me

/path/to/website/frontend/src/i18n.ts:53
const nextI18next = new NextI18next({
                    ^
TypeError: next_i18next_1.default is not a constructor
    at Object.<anonymous> (/path/to/website/frontend/src/i18n.ts:53:21)
    at Module._compile (internal/modules/cjs/loader.js:688:30)
    at Module.m._compile (/path/to/website/frontend/node_modules/ts-node/src/index.ts:439:23)
    at Module._extensions..js (internal/modules/cjs/loader.js:699:10)
    at Object.require.extensions.(anonymous function) [as .ts] (/path/to/website/frontend/node_modules/ts-node/src/index.ts:442:12)
    at Module.load (internal/modules/cjs/loader.js:598:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:537:12)
    at Function.Module._load (internal/modules/cjs/loader.js:529:3)
    at Module.require (internal/modules/cjs/loader.js:636:17)
    at require (internal/modules/cjs/helpers.js:20:18)

[isaachinman]

Example.

[kachkaev]

If I understand things correctly, CodeSandbox is client-side only. In my case, the exception is raised on the server. This may be to do with babel-plugin-add-module-exports introduced in da40a3e#diff-e56633f72ecc521128b3db6586074d2cR16 – I can't think of anything else that could contribute to this new error I witness 🤔

[isaachinman]

Without babel-plugin-add-module-exports:

exports.default = NextI18Next;

With babel-plugin-add-module-exports:

exports.default = NextI18Next;
module.exports = exports.default;

It's additive, not a replacement. If you believe this is the issue, please test by simply removing babel-plugin-add-module-exports from the .babelrc and running yarn build.

[isaachinman]

Again: please open a new issue for each new topic, instead of adding unrelated discussion to other issues.

[kachkaev]

The thing is that before we got into discussing import / require, I simply added a comment saying that I tried the new version as you asked and things don't work for me. The error did not point to import / require and I also did not emphasise it, because I am sure that import * as NextI18next from "next-i18next" is doing OK (I even tried const NextI18next = require("next-i18next").default || require("next-i18next"), which is s bullet-proof thing). I still think that there's something to do with react-tree-walker and import * is indeed unrelated. Please read our conversation again to see how we deviated from the original issue topic.

When I was trying 0.9.0 (regardless of the presence of * in import), the error was related to my usage of <Trans />, about which I warned you above. This is also a separate issue, which I was about to explore by submitting a PR with a broken MWE. Looking at the error stack shared above I still suspect that we can be dealing with some consequence of your changes in 2386bf2. Shall we open a new issue just because this issue is marked as closed? I'm not sure that it's strictly necessary (especially given the today's scale of the project).

I really appreciate what you're doing @isaachinman I'm happy to help as I see the potential value next-i18next can give to the community. Just please try to use a bit less of passive aggression in your comments (statements in bold, rhetorical questions without answers, locking discussions, etc.). I've been contributing to quite a few repos in the last few years and a the difference in the tone is quite noticeable now and then (not only in relation to myself). In the end of the day, it's all up to you, I'm just sharing some feedback because I don't want other contributors turn away from the idea of helping you simply because of the miscommunication. ✌️

[isaachinman]

If your problem is with v0.9.0, it is not related to this issue.

This is an open source project. If you find a bug or problem, you should:

1. Search issues for similar problems
2. Open a new issue if appropriate
3. Try to debug it yourself, if appropriate

I don't see what that error stack has to do with 2386bf2, you're going to need to explain that if you want to be understood.

[isaachinman]

Usage of Trans was reported as faulty apparently as far back as v0.8.0. It's very hard to maintain OSS when issues become ramble-y, cover too many topics, etc.

If you think there's a problem with usage of Trans, open an issue.

If you think there's a problem with the export syntax, open an issue.

If you think there's a problem with the tree-walking implementation that attempts to fix the namespace dep problem, this is the appropriate place to discuss it.

Any other topics are not appropriate for discussion. @kachkaev I also appreciate your enthusiastic interest in the project, but value is generated via concise bug reports or feature requests, and above all else, PRs.

[kachkaev]

Usage of Trans was reported as faulty apparently as far back as v0.8.0

I don't remember reporting an issue with Trans in 0.8.0, the comment you point to does not state that.

Here are the latest tests for yarn dev in https://gitlab.com/kachkaev/website-frontend/merge_requests/6 (assuming I'm using const NextI18next = require("next-i18next").default || require("next-i18next"), which seems to be a necessity after da40a3e7)

After upgrading 0.8.0 to 0.9.0 (next-i18next diff): no exception, even for <Trans />

After 0.9.0 → 0.10.0 (next-i18next diff):

Warning: Rendering <Context.Consumer.Provider> is not supported and will be removed in a future major release. Did you mean to render <Context.Provider> instead?
Warning: Rendering <Context.Consumer.Consumer> is not supported and will be removed in a future major release. Did you mean to render <Context.Consumer> instead?
react-i18next:: You will need pass in an i18next instance either by props, using I18nextProvider or by using i18nextReactModule. Learn more https://react.i18next.com/components/ov
erview#getting-the-i-18-n-function-into-the-flow
TypeError: Cannot read property 'wait' of null
    at NamespacesConsumerComponent.render (/path/to/website/frontend/node_modules/react-i18next/dist/commonjs/NamespacesConsumer.js:213:33)
    at render (/path/to/website/frontend/node_modules/react-tree-walker/src/index.js:253:32)
    at render (/path/to/website/frontend/node_modules/react-tree-walker/src/index.js:157:40)
    at process._tickCallback (internal/process/next_tick.js:68:7)
TypeError: Cannot read property 'wait' of null
    at NamespacesConsumerComponent.render (/path/to/website/frontend/node_modules/react-i18next/dist/commonjs/NamespacesConsumer.js:213:33)
    at render (/path/to/website/frontend/node_modules/react-tree-walker/src/index.js:253:32)
    at render (/path/to/website/frontend/node_modules/react-tree-walker/src/index.js:157:40)
    at process._tickCallback (internal/process/next_tick.js:68:7)
TypeError: Cannot read property 'wait' of null
    at NamespacesConsumerComponent.render (/path/to/website/frontend/node_modules/react-i18next/dist/commonjs/NamespacesConsumer.js:213:33)
    at render (/path/to/website/frontend/node_modules/react-tree-walker/src/index.js:253:32)
    at render (/path/to/website/frontend/node_modules/react-tree-walker/src/index.js:157:40)
    at process._tickCallback (internal/process/next_tick.js:68:7)

After 0.10.0 → 0.10.2 (next-i18next diff): no exception, but no data in initialI18nStore store again, the page flashes 🤷‍♂️

Was there some bug in 0.10.0 that is related to your fix of this issue, rather than <Trans/>? I believe so and this is exactly what I wanted help you with by providing a reproduction example. You asked to test your fix. Should we continue discussing an empty initialI18nStore in this thread? Probably not, it's a different kind of issue. Looking back, did I try to keep this conversation focused on the issue title? I believe so.

If you think there's a problem with the tree-walking implementation that attempts to fix the namespace dep problem, this is the appropriate place to discuss it.

[isaachinman]

Let's continue discussion in #54.