Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

language detection should not be done #10

Open
DmitryEfimenko opened this issue Aug 13, 2019 · 10 comments
Open

language detection should not be done #10

DmitryEfimenko opened this issue Aug 13, 2019 · 10 comments

Comments

@DmitryEfimenko
Copy link
Contributor

According to the ngx-translate examples, the translateService.use(lang) function should be called by the consumer application.

This makes sense because the functionality for getting the current language is a part of the business logic of the application. It can be different depending on the app's requirements.

For example, one application might use the short lang ids like en and ru. Another one might use longer locales: en-US and ru-RU. This means that this line of code will not work for these apps using longer locales. It will result in the TranslationLoader trying (and failing) to load en language when it needs to load en-US.

This is why I suggest removing the following lines completly from this fnction init:

const currentLang = this.getCachedLanguage() || this.translateService.getBrowserLang();
if (currentLang) { this.translateService.use(currentLang); }

Doing so will put the responsibility of calling translateService.use() on the developer of the app - just like it was initially intended by ngx-translate.

As a matter of fact, I don't think the init() function is necesary. The rest of its functionality could be easily placed in the constructor of the TranslateCacheService. This would simplify the API of the library while preserving the functionality.

In either case, this would be a breaking change. Although I understand the desire to avoid breaking changes, I believe this one is necessary.

What do you think?

@DmitryEfimenko
Copy link
Contributor Author

any word on this?

@abashmak
Copy link

+1, I'm also burned by this issue.

@DmitryEfimenko
Copy link
Contributor Author

well, it's time to switch to transloco...

@jgpacheco
Copy link
Owner

Reopening this issue since is not addressed by #9

@jgpacheco
Copy link
Owner

@DmitryEfimenko it will be a breaking change indeed.

@DmitryEfimenko
Copy link
Contributor Author

Are you OK with it? If so, I can create a PR

@jgpacheco
Copy link
Owner

It makes perfect sense to me. Since is a breaking change, please bump major version and update the README.

@diosney
Copy link

diosney commented Jun 19, 2020

Hi, any updates?

I use long locales ex: en-US and due the use of getBrowserLang at:

const currentLang = this.getCachedLanguage() || this.translateService.getBrowserLang();

this library is throwing an error looking from the wrong locale: en.

@DubeySandeep
Copy link

Hi @DmitryEfimenko, Are you working on this issue? Let me know I'll be happy to raise a PR if needed!

@DmitryEfimenko
Copy link
Contributor Author

no, go ahead, @DubeySandeep

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants