Do not pass language to the translate function

[fmauquiesg]

This fixes a corner case scenario we encountered on our project.

When changing language by doing $i18next.options.lng = 'fr' for example, the next AngularJS digest cycle will execute the different bindings with i18n filters. However, there is no way to ensure that the new translations are loaded before we hit the callback in $i18nextProvider.init() that defines the new t() function to use.

Since options.lng is set, i18next will see that we are trying to translate in another language, and that this language is not loaded yet. It will then try to load the language.

Since we are using a custom load function, we see at his time a huge number of calls on this function from i18next (corresponding to 2 * <number of namespaces> * <number of translated keys in the page>) that should actually wait for ng-i18next's init() method to be called and returned successfully.

By not passing the language when translating, i18next will translate in the language set by the current t() function during this digest cycle. When the init() method returns, its callback will initiate a new digest cycle, and translate the page into the newly set language.

[efolio]

Nice point.

Wouldn't it be better to modify the reInitmethod to take the language (in the new options) as a parameter? Then it would itself call init() and set globalOptions in the callback?

[fmauquiesg]

It would, but I tried to have as little impact as possible, since we try to not modify upstream libs locally.

This fixes the bug without an API change.

I think that even if you upgrade the reInit() method, the language should not be passed to the translate options, since it would still be a configuration parameter (we use it as such when we remember the preferred language in localStorage).

[efolio]

Passing the language to the translate function may be necessary to develop language administration tools. This is not something that shocks me, and meddling with it does change the API's behavior.

reInit has been developed exactly for language change, in order to re-fetch translations from the back-end. So my guess is that your fix should be there…

[bugwelle]

I also think your code has one problem:

Look at line 100 in proider.js

mergedOptions.lng is only set when explicitly setting a language when using the ng-i18next provider, filter or directive (e.g. <p ng-i18next="[i18next]({lng:'de-DE'})hello"></p>). This means in most cases we use translations['auto'][key] which is not what we want. When using e.g. german, it should be translations['de-DE'][key] and so on.

Something like this could work

var mergedOptions = !!options ? angular.extend({}, translateOptions, options) : translateOptions;

mergedOptions.lng = !!options.lng ? options.lng : globalOptions.lng;

...

Regards, Andre

[fmauquiesg]

Actually it doesn't change the API's behavior: if you specify a language in the translation's options like ng-i18next="[i18next]({lng:'dev'})hello (in test Unit: "jm.i18next - Directive / passing options"), they are still passed through to t(). I made sure I didn't break a test. And changing $i18next.options.lng still loads the new language, as specified in the docs.

The fix just makes sure translations are not fetched multiple times for one run.

That said, I do agree that using a dedicated method call to change the language would be cleaner.

[fmauquiesg]

Agreed, Andre.

[fmauquiesg]

Hm, actually, not agreed... Passing globalOptions.lng would reopen the bug, since globalOptions.lng has changed before the re-init. It would work if globalOptions is only set in init()'s callback.

[bugwelle]

@fmauquiesg Haven't tested, yet. ;)

I'm currently not using my developer computer. So either I'll look over this again on weekend or @efolio can hopefully help you. :)

Sorry for that. But thank you for this PR. Never thought about this szenario. :+1:

Regards, Andre

[fmauquiesg]

Thanks ! We didn't either, we had to get a big perf problem on IE8 and a lot of debugging done to get down to these 3 lines of fix. It actually behaves quite well in recent browsers as it is...

And thanks for the lib btw. We love it, and we do lots of crazy stuff with it :)

[bugwelle]

:+1: Glad to hear that.

I have to tell you that we don't officially support IE8 anymore (see README ), neither does Angular 1.3. At the moment there is only one function that is not supported by IE8. It's .trim(). Using a polyfill should help. :)

[fmauquiesg]

Thanks. Don't worry about that, we still have to put up with some legacy for a limited time, which means for now staying to Angular 1.2 and using lots of polyfills :)

It actually works quite well, and we are on the good track to getting rid of it.

[jamuhl]

the latest version of i18next has a replacement shim for trim added. but not sure when i will remove the support for ie8 - as still to much people are forced to support that in their legacy apps.

[bugwelle]

Right, i18next has a lot of polyfills :+1: Trim: https://github.com/i18next/i18next/blob/master/i18next.js#L73-L77

[anwalkers]

1.0.0 now supports the latest version of i18next.