Issue with global i18next instance in Universal / SSR

[vz-tl]

I18NextService is using a global static instance of i18next (i18next.default), which more or less is being wrapped by the service.

In Angular Universal (SSR) this can lead to race conditions, if there will be concurrent, longer-lasting hence overlapping render processes, executed by using different language configurations. In such case all render processes will use the same global i18next instance, but overwriting its configuration when configuring I18NextService in an APP_INITIALIZER. So if a render process takes longer to complete and already a new process is being started, the language configuration of the new process will also affect still running processes, which can lead to mixed / wrong language output in rendered result. So, in SSR I assess the way I18NextService is configuring underlying i18next instance as a bug.

A solution would be to provide a separate instance of i18next per each Angular app instantiation in app server module, created via i18next.createInstance(). Providing the instance can be done via InjectionToken:

// in I18NEXT_TOKENS.ts add
import * as i18n from 'i18next';

export const I18NEXT_INSTANCE = new InjectionToken<i18n.i18n>( 'I18NEXT_INSTANCE');

// in I18NextService.ts add

const i18next = i18n.default;

@Injectable()
export class I18NextService implements ITranslationService {
...
private readonly i18next: i18n.i18n; // <-- create a class member holding the i18next instance

constructor(
    @Inject(I18NEXT_ERROR_HANDLING_STRATEGY),
    @Optional() @Inject(I18NEXT_INSTANCE) i18nextInstance: i18n.i18n // <-- add a new constructor param for the injected i18next instance
  ) {
    ...
    this.i18next = i18nextInstance || i18next; // <-- if InjectionToken provided, use its value (server env), else fallback to the global instance (browser)
  }

  get language() {
    return this.i18next.language; // <- replace any usage of i18next in the service with class member this.i18next
  }

then, in Universal-specific app.server.module a new i18next instance could be provided:

@NgModule({
  imports: [AppModule, ServerModule],
  providers: [
    ...
    {
      provide: I18NEXT_INSTANCE,
      useFactory: () => i18next.createInstance() // provide new i18next instance to be used by I18NextService
    }
  ],
  bootstrap: [AppComponent]
})
export class AppServerModule {}

I would highly appreciate, the above proposal to be added to this awesome Angular package, as right now we have to add the changes via monkey-patching (using patch-package), which definitely isn't a long-term solution.

Thx in advance!

[Romanchuk]

Thank you for your issue! Yes, at the moment i wrote lib i couldn't find use case for i18next.createInstance(). So i used global one. @vz-tl So you have single AppServerModule for multiple running apps (on nodejs)? Please tell me more to have a full picture

[vz-tl]

My general app setup is pretty standard, comparable to what you get after adding @nguniversal packages. There's only one app, executed within a Node.js Express server instance, andI18NextModule.forRoot() has been added to AppModule imports. So it's not a matter of running multiple apps within the same server instance, but running one app only (anyway, amount of apps doesn't matter).

Each new request will execute a fresh instance of the Angular app, however because of angular-i18next using the static i18next singletone instance, the instance is shared for all subsequent app instantiations. You can observe this by console.logging the I18NextService.language value within APP_INITIALIZER right before I18NextService.use(...).init(...): At the very first request it's undefined. However, for subsequent requests language will already have the value of the previous i18next language configuration. This is because the underlying i18next instance is a static one, created in global scope of the persistent Node.js Express app.

As I already wrote above, sharing the global instance across multiple requests running in parallel, could potentially change language configuration of a previous Angular app instance if it's taking longer to complete, while a new app instance is being executed in parallel (or short after previous one) by a subsequent incoming request, configured for another language, before previous app execution has been finished. In order to avoid this, each Angular app executed in a Node.js context would require a fresh instance of i18next by calling i18next.createInstance().

[Romanchuk]

@vz-tl Thank you very much, got it! I'm going to reorganize repo first and then i'll start works on this issue

[Romanchuk]

@vz-tl Please check out https://github.com/Romanchuk/angular-i18next/pull/92 It's SSR app that uses i18next. It didn't work out with your code suggestion. But i did it via https://github.com/i18next/i18next-http-middleware You may test your use cases!

[vz-tl]

Hi @Romanchuk , Thanks a lot for spending your time in figuring out a solution, but I've some comments/question (honestly, without first running your proposal locally):

1. Could you explain, why you come to the conclusion my solution doesn't work? As I've stated above, currently I'm monkey-patching angular-i18next with my changes described above and it works flawlessly, as now each process will have its dedicated i18next instance.
2. For solving the issue with using a shared i18next instance on server side, I don't see a relation to the i18next-backend used for loading translation files. It's about the global i18next instance configuration being changed, affecting concurrently running processes, configured for different languages. Let me explain again: Process A starts with lng: 'en-UK' and presumably takes 10 secs. to complete because of a long-running http request fetching required data. Meanwhile a new process B will be executed, configured for lng: 'it-IT'. Process A already rendered half of its HTML in English, waiting for the http response. After request is being resolved, due to the fact that process B changed the language config of global i18next instance, process A will finalize rendering the rest of the HTML by incorrectly using Italian language instead of English.
3. In your approach you're configuring i18next in server.ts outside of the actual Angular app code, which seems not the right place to me. From my perspective, even on server-side the regular implementation approach for configuring i18next inside an Angular translation module using APP_INITIALIZER should be used, which works great without any issues.

Looking forward to your answer!

[Romanchuk]

@vz-tl 1) It won't work (and i tested it) because it's not enough I18NEXT_INSTANCE created in factory, because I18NextService would be initiated ones, so there always be same one instance. Also it not quite right to create instances always. We should have single instance per request. 2) Look closely to https://github.com/i18next/i18next-http-middleware it is not about loading resource files, it is official i18next plugin for SSR. Good use case, i'll try to test it.
3) Code written according to official angular ssr examples. Yes, code from server.ts can be moved to other place, it's not an issue right now. We need two different setups for node and browsers so APP_INITILIZER code any way should be divided.

[vz-tl]

Hi @Romanchuk ,

I don't know if I have a basic misunderstanding about how an angular app is being initialized in a node express context...

Correct me if I'm wrong, but from my humble perspective the Angular app itself will be executed, instantiated and initialized per each request, afterwards it dies. The surrounding node express environment instead will stay alive until next deployment or whatever it makes to die. Based on that, my believe is, that any Angular service, including I18NextService only lives as long the app is being executed to render the html output. Afterwards, app instance dies as well as all its service instances. So, in regards to its injected services, an Angular app always will receive pristine instances. However, everything which has been added to global scope will remain there, like the global i18next instance. Am I really wrong here? I don't think so, because by logging the injected I18NextService instance in app init provider, it will always be in its initial state (except from the wrapped global i18next instance). This can be easily tested by adding some actually non-existent prop to the service and check its presence on next requests, like:

{
    provide: APP_INITIALIZER,
    useFactory: (i18NextService: ITranslationService) => {

      // log below will always be undefined, as per each app render process
      // also a new service instance is being created. If same service instance would be reused
      // across subsequent requests, the added `instantiated` prop would evaluate to true.
     console.log((i18nextService as any).instantiated); 
     (i18nextService as any).instantiated = true;

     return () => i18nextService.init({ /*... all the config stuff */ })
    },
    deps: [I18NEXT_SERVICE],
    multi: true
},

I can only partially agree with your argument about it wouldn't be right to always create new instances. First, i18next provides the option to create new instances, so why it shouldn't be ok creating new ones? Second, same as above, I 'm also quite sure, a new instance created per each request, will be garbage collected the same as the whole app including its service instances, once the render process is done, as there aren't (shouldn't) be any references to it anymore. So where's the drawback using dedicated instances?

Anyway, thanks again for your answer, I will have a closer look in order to understand the approach you recommended, but would be glad to get your opinion about my above assumptions.

[vz-tl]

...me again ;-)

Had a closer look at your example. Indeed, i18next-http-middleware does the trick, however... :D

I skimmed their code and found the following line: https://github.com/i18next/i18next-http-middleware/blob/master/lib/index.js#L40

 const i18n = i18next.cloneInstance({ initImmediate: false })

According to i18next documentation (https://www.i18next.com/overview/api#cloneinstance) that will clone the current instance, which is similar to createInstance() but config, plugins and store will be inherited. So basically, it's still a separate new instance. Under the hood they do the same as I proposed, just with the overhead of having to pull in one more package dependency.

As you made me doubt about my proposal, using your example app l verified again if it works or not. I simplified the whole setup, so it only has the i18next initialization within APP_INITIALZER, but removed all i18next related stuff from server.ts. For testing the race condition, I added an observable with 10 secs timeout to simple-demo.component, which finally switches on the lower part of the view markup. When now requesting the app in one tab with language='ru', then waiting 5-8 secs and then requesting the app with language='en' in another tab, without using I18NEXT_INSTANCE InjectionToken, the delayed view part was in English but the rest of the page was in Russian. Once I added back the provider for I18NEXT_INSTANCE, returning i18next.createInstance(), then both page requests didn't affect each other anymore. Same result as with using i18next-http-middleware, but without need to split i18next configuration across files and requiring additional packages.

As in your example branch you also used I18NEXT_INSTANCE for passing on the i18next instance from the middleware to I18NextService, the changes made to I18NextService are basically still needed. So I would be glad if you could integrate them into your great package, increasing its flexibility for having the choice of using a specific i18next instance, no matter if a completely new or a cloned one or just continue using the global one.

[Romanchuk]

@vz-tl Yes, we need I18NEXT_INSTANCE as you proposed

There are two things: 1) If i instansiate i18next in APP_INITIALZER on nodejs - it falls on concurent request (says: "you inited me twice"). 2) And i can't achieve same config because HttpApi what i am using can't get .json's right... If i would have inmemory resources and/or if i didn't use lazy load for namespaces, i might not need other plugins and configs.... Looks like configuration is more up to developer and use cases. And it's not tight up to this package, so it's free to setup it however you want.

Anyway, it seems working. I'll release beta. If it's okay, then stable

[Romanchuk]

Not closed yet

[Romanchuk]

@vz-tl Please try: v14.2.0-0

[vz-tl]

@Romanchuk ,

tested v14.2.0-0 and the changes are working fine in my app. However, before I was able to successfully test the prerelease I first had to fix an error with new version when running app in browser:

Uncaught ReferenceError: require is not defined

this is because of https://github.com/Romanchuk/angular-i18next/blob/master/libs/angular-i18next/src/lib/index.ts#L54

const i18nextGlobal: i18n = require('i18next');

which should be replaced with

const i18nextGlobal: i18n.i18n = i18n.default;

same as it is in I18NextService.ts

Issue already has been reported in #91

Could you please fix that, so I can double-check once more? Thx!

[Romanchuk]

@vz-tl Oh, sorry missed one place 14.2.0-1

[vz-tl]

Hi @Romanchuk, works now, thx!

[Romanchuk]

@vz-tl Stable version 14.2.0 released!