Do not union any in Translation

[Ghostbird]

According to the lines below, the any union is necessary.

core/projects/ngx-translate/src/lib/translate.service.ts

Lines 19 to 26 in ee77f16

	export type Translation =
	string |
	Translation[] |
	TranslationObject |
	// required to prevent error "Type instantiation is excessively deep and possibly infinite."
	// eslint-disable-next-line @typescript-eslint/no-explicit-any
	any

However, this causes me issues, since the type of Translation is any because any overrides everything.
So I tried this in my local repo ("typescript": "^5.4.5") and I get no such recursion message.

// eslint-disable-next-line @typescript-eslint/consistent-indexed-object-style
interface TranslationObject {
  [key: string]: Translation;
}
type Translation = string | Translation[] | TranslationObject;

I can think of two possible causes (but there might be another reason):

• When this code was written, that restriction was in place, but it was removed in a later Typescript version.
• When this code was written, the restriction was detected, and circumvented by making TranslationObject an interface. However the | any was not removed by accident.

Can this | any be removed?

[CodeAndWeb]

Interesting.

Did you try to run the lint job?

[Ghostbird]

That's referenced in my second point:

When this code was written, the restriction was detected, and circumvented by making TranslationObject an interface.

The restriction can be circumvented by writing TranslationObject as an interface instead of a type assignment. However that requires // eslint-disable-next-line @typescript-eslint/consistent-indexed-object-style to be set to allow an interface to be used where a type assignment would have been more concise.

If you remove that comment, the linting fails.

[SvajkaJ]

I also kindly ask for this improvement.

[rbalet]

I'm on it :)

[rbalet]

A little update.

I've started pocking around to implement this fix and could already do the following PR

I was ready to remove the any from the Translation type, but the problem is the following

When I'm using translateService.instant('TITLE') without the any then it can be either one of those values : string | Translation[] | TranslationObject which would force me then to type safe it every time I call the method.

To simplify it, I'd say that the instant() method should only return string | null | undefined, or maybe only string.

Could I make the required changes, or @CodeAndWeb do you see something getting against it ?
(Not sure I fully understand what the instant could be used for outside returning a string.

Cheers

[CodeAndWeb]

I've made some more modifications and completely removed "any" from the interface now.

The issue why the "any" was required was the tests. Adding the "as string" fixed the issue. I think it came from toEqual using generics and not being able to clearly detect the type because of the recursion.

      expect(translatePipe.transform('TEST')).toEqual("This is a test");

      expect(translatePipe.transform('TEST') as string).toEqual("This is a test");

[rbalet]

@CodeAndWeb Be careful, this is not only the test, but also when we're using it in our own project.

It isn't able to detect the type because the type can really have different types (string, Translate[], ...).
Therefore the use of as string should be avoided (as this is basically the same as writing as any.

Se we would need to add the as string here

myTranslation?: string 

constructor() {
  this.myTranslation = this.translateService.instant('TEST') as string
}

Which IMO isn't a good option...

This is the reason why I asked about the if the instant() shouldn't always return a string.

[Ghostbird]

@rbalet That's already the case. Our code has quite a few calls like this.translate.instant('foo') as string. The alternative would probably to throw when the found value isn't a string, but that's undesirable too.

Why do you state that writing as string is basically the same as writing as any? One is understandable, the writer states that he's checked this, even if Typescript's type system could not check it. The other is the opposite. The writer states that no matter that Typescript gives some guarantees about the type, he insists on pretending they're not there. I use the former if necessary, but the latter is forbidden in our code base.

[rbalet]

@Ghostbird what I ment with that is

1. Can we take in account that the .instant() method is always returning a string?
2. If yes: they we change the type of the return method
3. If no: can we split the method in two? Like one go for the string, the other for string, object and array.

2 or 3 will let us then remove the any from the Translate interface

As I assume most people are only caring of the instant() method to go get a string this would required from them to add every where the as string. And for the users that would like to get the object, they could use the second method. (here again I do not understand why somebody would do so, and if anyone understand please explain it to me)

What do you think of it?

[CodeAndWeb]

From a technical standpoint, I completely agree with you.

I don't like the API for get() and instant() either - but I currently see no way to change it because it might make many users unhappy.

1. Passing string or string[] to get one or many translations is not really a nice way to handle the API. However using getOne() and getMany() would duplicate the API. (there's get, instant, getStreamOnTranslationChange)

2. The result value is also a problem. There's no guarantee that the return value is a string. I know many users who use the JSON files like this

{
   "languages":
   {
        "DE": "German",
        "EN": "English",
        "FR": "French",       
   }
}

and feed this e.g. into a language picker. translate.get("languages").
Changing it to string (what I really would like to do, btw.) would break their update path.

3. Translation is now StrictTranslation|any which is the return value for get, instant....

Currently you can still write this:

  let translationList = this.translate.instant("x");
  translationList.map(() => {
     ....
  })

Setting the result to StrictTranslation would break that code.

4. I thought about making it a generic function:

public get<T = Translation | TranslationObject>(
  key: string | string[],
  interpolateParams?: InterpolationParameters
): Observable<T>

That would allow writing get<string|undefined>()or similar. But what should happen if we have a type error? E.g. it would return an array? Throw an exception? Just return an empty string? Or just return it as T.

The result is at least string|undefined since we don't know if the translation for that key is loaded or not.

[CodeAndWeb]

What I would prefer is a way to configure the library like this - but this obviously can't exist in typescript:

if(ngxTanslateSticeMode) 
    export type Translation=StrictTranslation;
else
    export type Translation=StrictTranslation|any;

[rbalet]

@CodeAndWeb now that I think about it. It's could actually be doable with type generic.

But I'll make a proposition in that direction :)