core icon indicating copy to clipboard operation
core copied to clipboard

feat(service): use of signal

Open rbalet opened this issue 11 months ago • 8 comments

Current behavior

We need to be working with subscription.

Expected behavior

Although this is good the way it is, it could be nicer to add additional signal alternative.

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

  1. It would let us work with computed variable directly
  2. It will lower the chances of forgotten unsubscription

How do you think that we should implement this?

I can do a PR if needed

I do see two options

1: Additional variable

Inside the translate.service.ts, add of additional signal for reach observable

// Exemple

  $onTranslationChange = toSignal(this.store.onTranslationChange)
  get onTranslationChange(): Observable<TranslationChangeEvent> {
    return this.store.onTranslationChange;
  }

2: Replacement of the former logic

I'd avoid this as this is quite a breaking change

Same as 1. but we remove the onTranslationChange() method. which let you write it as follow.

onTranslationChange = toSignal(this.store.onTranslationChange)

I do personally always use the $ in front of a signal, but standards aren't set yet for the nomenclature

rbalet avatar Mar 12 '25 15:03 rbalet

@rbalet You only addressed onTranslationChange, but to be consistent, I think get() should have a signal equivalent too - here's a method that works for me:

class ExtendedTranslateService extends TranslateService {
  translate(
        key: string | Signal<string>,
        params?: Record<string, string | number | null | undefined> | Signal<Record<string, string | number | null | undefined> | undefined>,
    ): Signal<string | undefined> {
        const key$ = toObservable(isSignal(key) ? key : signal(key));
        const params$ = toObservable(isSignal(params) ? params : signal(params));

        const translation$ = key$.pipe(switchMap((key) => params$.pipe(switchMap((params) => this.get(key, params)))));

        return toSignal(translation$);
   }
}

This can than be used like this for example:

class MyComponent {
  readonly amount = computed(() => this.someComputation());
  readonly someReactiveTranslation = this.extendedTranslateService.translate('myKey', computed(() => ({
    amount: this.amount()
  }));
}

floAtEbcont avatar Mar 17 '25 10:03 floAtEbcont

@floAtEbcont yes indeed, every method would have their equivalent.

It was just for the sake of simplicity

rbalet avatar Mar 17 '25 11:03 rbalet

Thank you for opening this issue @rbalet. Signals are essential for modern angular development and should be supported by ngx-translate asap.

This should also extend to actual translation. I propose to add a method similar to stream, that returns a signal which reacts on lang or translation change. We could simply call this method translate

homj avatar Mar 30 '25 13:03 homj

@homj it is true that, if you add the $ in front of the variable, you could not need the on...Change as it is per syntax already understandable

rbalet avatar Mar 30 '25 13:03 rbalet

@homj

This should also extend to actual translation. I propose to add a method similar to stream, that returns a signal which reacts on lang or translation change. We could simply call this method translate

This is exactly what I suggested and provided a workaround for further up, right?

floAtEbcont avatar Apr 07 '25 14:04 floAtEbcont

@rbalet is there a target branch for v18 already? I'm considering working on a PR for that. Any additional informations/hints you would like to place for the implementation?

EinfachHans avatar Oct 17 '25 07:10 EinfachHans

Hey @EinfachHans, thanks for the proposition.

We're sadly on pause till we find the best solution for the following discussion: https://github.com/ngx-translate/core/discussions/1599

So I wouldn't invest much time till we get done with it. But I'll text you back once we could make a stable version.

Don't hesitate to take part of the discussion, all the help are always welcome

rbalet avatar Oct 18 '25 08:10 rbalet