Transloco: Language fallback doesn't work properly

[zhongsp]

I'm submitting a...

[ ] Regression (a behavior that used to work and stopped working in a new release)
[x] Bug report  
[ ] Performance issue
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request
[ ] Other... Please describe:

Current behavior

Custom TranslocoFallbackStrategy not working properly. It will skip some of the fallback langs. Because the this.failedCounter is misused. the fallbacks variable is re-evaluate on each failure, because the parameter passed to this.fallbackStrategy.getNextLangs(lang) is the current failed language. It's not the original failed language. So when this.failedCounter's value increases to 1. It will ignore the first element in new fallbacks.

Given the original lang zh-Hans-CN:

1. Load zh-Hans-CN failed.
2. getNextLangs() returns ['zh-Hans', 'zh', 'en']. failedCounter is 0. It will try to load zh-Hans. And increase failedCounter to 1
3. Load zh-Hans failed
4. getNextLangs() returns ['zh', 'en'], because handleFailure receives zh-Hans as parameter instead of zh-Hans-CN. failedCounter is 1. It will try to load en. Error Here: 'zh' is skipped!

https://github.com/ngneat/transloco/blob/master/projects/ngneat/transloco/src/lib/transloco.service.ts#L558

  private handleFailure(lang: string, mergedOptions) {
    const splitted = lang.split('/');
    const fallbacks = mergedOptions.fallbackLangs || this.fallbackStrategy.getNextLangs(lang);
    const nextLang = fallbacks[this.failedCounter];
    this.failedLangs.add(lang);

Expected behavior

Given the original lang zh-Hans-CN, and getNextLangs() returns ['zh-Hans', 'zh', 'en'].

Transloco should try to load files in the following order:

1. zh-Hans-CN.json
2. zh-Hans.json
3. zh.json
4. en.json

In above scenario, zh.json will be skipped.

Minimal reproduction of the problem with instructions

For bug reports please provide the _STEPS TO REPRODUCE_ and if possible a _MINIMAL DEMO_ of the problem, for that you could use our stackblitz example

What is the motivation / use case for changing the behavior?

Environment

Angular version: X.Y.Z


Browser:
- [ ] Chrome (desktop) version XX
- [ ] Chrome (Android) version XX
- [ ] Chrome (iOS) version XX
- [ ] Firefox version XX
- [ ] Safari (desktop) version XX
- [ ] Safari (iOS) version XX
- [ ] IE version XX
- [ ] Edge version XX

For Tooling issues:
- Node version: XX  
- Platform:  

Others:

[zhongsp]

In the following source code, I didn't find where does fallbackLangs in mergedOptions.fallbackLangs come from? I searched the full source code, and didn't find the result..

https://github.com/ngneat/transloco/blob/master/projects/ngneat/transloco/src/lib/transloco.service.ts#L558

  private handleFailure(lang: string, mergedOptions) {
    const splitted = lang.split('/');
    const fallbacks = mergedOptions.fallbackLangs || this.fallbackStrategy.getNextLangs(lang);
    const nextLang = fallbacks[this.failedCounter];
    this.failedLangs.add(lang);

[imnista]

I also encountered the same problem, and is there a related fix plan?
@NetanelBasal , @shaharkazaz , @itayod , @zhongsp

Merry Christmas!

[shaharkazaz]

@imnista There is no fix plan, we resolve the issues when we have the time, but as you can see there aren't many issues open 🙂
@imnista @zhongsp Would you like to create a PR for it?

[zhongsp]

I can have a try.

[zhongsp]

@shaharkazaz I submit a PR for this.

[zhongsp]

@shaharkazaz Are you able to review my fixing PR? 😄

[shaharkazaz]

@zhongsp hey! Really busy times for me, I'll try to get to it this week, but no promises 🙏

Once I'll get some time I'll address it!

[shaharkazaz]

@zhongsp @imnista can you please share your implementation of the custom loader?
Why is not returning the same set of language array each time? what's causing the change?

[zhongsp]

@shaharkazaz I think you mean the custom fallback strategy. It's as follows:

export class LanguageFallbackStrategy implements TranslocoFallbackStrategy {
  /**
   * If `failedLang` is 'zh-Hans-CN', then it returns `['zh-Hans', 'zh', 'en']`.
   * If `failedLang` is 'zh-Hans', then it returns `['zh', 'en']`.
   * If `failedLang` is 'zh', then it returns `['en']`.
   */
   getNextLangs(failedLang: string): string[] {
    // algorithm to calculate langs as comments says
    return ...;
  }
}

If I set the above LanguageFallbackStrategy to Transloco, Transloco doesn't try fallback langs one by one, as I described in issue description.

My custom translation file loader is just a simple http loader.

Why the current unit tests of custom fallback strategy passed?

Please look at this line: https://github.com/ngneat/transloco/blob/master/projects/ngneat/transloco/src/lib/tests/service/fallbacks.spec.ts#L119

Because, It assumes getNextLangs() always returns the same fallback langs array no matter what the failedLang parameter value is.. But in real scenario, the API consumer wants to customize the fallback langs according to the failedLang parameter.
And this is my case, I returns different fallback langs according to failedLang, and the result is wrong.

[shaharkazaz]

@zhongsp Thanks for the response, maybe I'm missing something but how does that differ from the default fallback strategy?
Instead of returning a different array each time, you always return ['zh-Hans', 'zh', 'en'] and the failedCounter will iterate them one by one.

Have you tried this approach?

[zhongsp]

@shaharkazaz I think the reason is fallback langs are unknown at compile time. At runtime, the fallback langs are generated according to the failed lang dynamically. The known thing is the algorithm used to calculate fallback langs based on a failed lang.

['zh-Hans', 'zh', 'en'] is just one scenario of receiving failed zh-Hans-CN at runtime. It would any other valid BCP47 language tags, e.g. fr-CA, then the fallback is ['fr', 'en']

[zhongsp]

@shaharkazaz Thanks for paying attention to this issue. I want to confirm if it's by design that Transloco requires a user custom fallback language strategy function always returns the same language array? I'm not seeing this claim in doc.

Because my product is about to ship, so I need to resolve this issue, either via Transloco code update or via some workaround by myself.

https://github.com/ngneat/transloco/blob/master/projects/ngneat/transloco/src/lib/transloco-fallback-strategy.ts

export const TRANSLOCO_FALLBACK_STRATEGY = new InjectionToken<TranslocoFallbackStrategy>('TRANSLOCO_FALLBACK_STRATEGY');

export interface TranslocoFallbackStrategy {
  getNextLangs(failedLang: string): string[];
}

export class DefaultFallbackStrategy implements TranslocoFallbackStrategy {
  constructor(@Inject(TRANSLOCO_CONFIG) private userConfig: TranslocoConfig) {}

  getNextLangs(failedLang: string) {
    const fallbackLang = this.userConfig.fallbackLang;
    if (!fallbackLang) {
      throw new Error('When using the default fallback, a fallback language must be provided in the config!');
    }

    return Array.isArray(fallbackLang) ? fallbackLang : [fallbackLang];
  }
}

[shaharkazaz]

@zhongsp Maybe I'm still missing the use case where the fallback languages are changing.
I'm not sure that with the current fallback strategy you can achieve what you need, I think the issue here might be a bit deeper than the counter.

Can you please add a flow & code example for this:

['zh-Hans', 'zh', 'en'] is just one scenario of receiving failed zh-Hans-CN at runtime. It would any other valid BCP47 language tags, e.g. fr-CA, then the fallback is ['fr', 'en']

The reason you can't implement the current logic you want is that you are not aware of the original language that we tried to load and failed, if you did have that information you could always return the same original array and you won't have this issue, correct?

Let's resolve this 💪

[zhongsp]

@shaharkazaz Thanks very much for replying.

This is my custom TranslocoFallbackStrategy:

export class LanguageFallbackStrategy implements TranslocoFallbackStrategy {
  static readonly defaultLang = 'en';

  /**
   * @example
   * 
   * failedLang: 'zh-Hans-CN'
   * Returns: ['zh-Hans', 'zh', 'en']
   *
   * lang: 'fr-CA'
   * Returns: ['fr', 'en']
   */
  getNextLangs(failedLang: string): string[] {
    const langs = this.getAllLangs(failedLang);
    return failedLang === langs[0] ? langs.slice(1) : langs;
  }

  /**
   * @example
   * 
   * lang: 'zh-Hans-CN'
   * Returns: ['zh-Hans-CN', 'zh-Hans', 'zh', 'en']
   *
   * lang: 'fr-CA'
   * Returns: ['fr-CA', 'fr', 'en']
   */
  getAllLangs(lang: string): string[] {
    if (this.isLangValid(lang)) {
      const langs: string[] = [];

      // For example, `zh-Hans-CN` may next check for
      // `zh-Hans`, then if `zh-Hans` is not found, `zh`.
      const splitted = lang.split('-');
      splitted.reduce((acc, cur) => {
        const lang = acc ? `${acc}-${cur}` : `${cur}`;
        langs.unshift(lang);
        return lang;
      }, '');

      // append the default language to the end
      const lastLang = langs[langs.length - 1];
      if (lastLang !== LanguageFallbackStrategy.defaultLang) {
        langs.push(LanguageFallbackStrategy.defaultLang);
      }

      return langs;
    } else {
      return [LanguageFallbackStrategy.defaultLang];
    }
  }
}

And add it to root providers:

export const translocoFallbackStrategy = {
  provide: TRANSLOCO_FALLBACK_STRATEGY,
  useClass: LanguageFallbackStrategy,
};

@NgModule({
  imports: [TranslocoModule],
  exports: [TranslocoModule],
})
export class I18nModule {
  static forRoot(): ModuleWithProviders<I18nModule> {
    return {
      ngModule: I18nModule,
      providers: [
        acceptLanguageInterceptor,
        selectBrowserLanguageProvider,
        translocoConfig,
        translocoFallbackStrategy,
        translocoLoader,
      ],
    };
  }
}

I thought this is all I need to code to make the language fallback work. However, this is not working as I expected. :D

[shaharkazaz]

@zhongsp Thanks for providing the code, it's very helpful. let me think about what's the best approach here 👍

[shaharkazaz]

@zhongsp Ok, so I thought about this, and here are my thoughtts:

• As a fix for the current situation the getNextLangs should be called only once on the first language failure.
• In the future I think that getNextLangs should be replaced with getNextLang, so the strategy will decide individually for each failed lang what should be loaded next.

[zhongsp]

• As a fix for the current situation the getNextLangs should be called only once on the first language failure.

  • Totally agree. This is what I'm trying to do in the fix PR. The getNextLangs() is only called at the first time of loading translation failed. And the langs in NextLangs in tried one by one, until loading a translation successfully. In the meanwhile, getNextLangs() must not be evaluated again.

• In the future I think that getNextLangs should be replaced with getNextLang, so the strategy will decide individually for each failed lang what should be loaded next.

  • I really like this idea. From a API consumer perspective, provide a single fallback language according to the failed one is enough. It's simpler and easier to understand.
    From transloco perspective, it's easier to implement, I think.

@shaharkazaz

[zhongsp]

@shaharkazaz Previously, I submit a PR to address this. It's https://github.com/ngneat/transloco/pull/390.
Could you please have a look at it? Please feel free to modify the PR, or even close it and create a new one. Thanks a lot.

[shaharkazaz]

@zhongsp Left some comments, great work 👌

[zhongsp]

@shaharkazaz Thanks for reviewing! I added the fixes and some questions there, please help to review.

[shaharkazaz]

@zhongsp I'll take a look tomorrow 👍

[zhongsp]

@shaharkazaz I committed a fix commit and leave comments, could you please help to review that?

[shaharkazaz]

@zhongsp Released in v2.20.1, please verify 😄