Do not union `any` in `Translation`

[Ghostbird]

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

https://github.com/ngx-translate/core/blob/ee77f1651171d523d8202274a08f8dd690bdaf81/projects/ngx-translate/src/lib/translate.service.ts#L19-L26

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 :)

[rbalet]

@Ghostbird Sorry for the delay, we needed to take care of other matter beforehand.

I couldn't figure out any solution to go without the any while not breaking the app for everybody else.

Therefore I've proposed this PR: https://github.com/ngx-translate/core/pull/1569

it would let you do the following

translateService.instant<string>('FOO')

Would this satisfy your issue ?

Note: it will be understood as string | undefined | null

[Ghostbird]

I'm not sure why this would break things for other people. The result should never be any, right? It should be string | Translation[] | TranslationObject. The current union is string | Translation[] | TranslationObject | any.

My only proposal was to remove any from that union, as according to the comment it was only added to circumvent a limitation of older Typescript versions. I already showed that it isn't necessary when using modern Typescript.

[Ghostbird]

Clarification. I'm not asking for any type narrowing between string, Translation[] and TranslationObject. That was discussed here, but seems a tangent unrelated to this issue.

[rbalet]

I'm not sure why this would break things for other people. The result should never be any, right? It should be string | Translation[] | TranslationObject. The current union is string | Translation[] | TranslationObject | any.

Actually, when you remove the any, the instant returns a possible set of type (string | Translation[] | TranslationObject).

If you assign this value to a variable, you won't be able to use it as a string directly as this could be an array, therefore you'll get a compiler error. (Same if you'd like to use the array, ...)

Having any let you do such things.

A lot of apps depend on this behaviour already, and if we remove the any, everyone will have to declare what tipe is expected to be returned.

This is the reason why I've proposed this PR, It let you get rid of the any, if you wish, but also let you clearly define what type you wish to get, while giving you back the undefined and null possibility.

I hope it makes sense '^^

[Ghostbird]

In all my TypeScript projects, any is always forbidden, so I forgot that that's possible (and it's exactly why it's forbidden). Now I understand why that any is there. I of course already use as string everywhere.

Now I understand your approach. I'm not sure whether it's the way to go though. It hides the cast, which is kind of icky. If I call a library method that returns a T, I expect it to return a T, but this casts the result to T, but might actually return something else.

I think the idealistic approach would be to add new methods that return the non-any-unioned type, and mark the current methods for deprecation. But I guess that the required refactoring would be hard to handle for the users that don't use (proper) Typescript. For most Typescript users that would want the new methods, it's an automatically executable refactoring, because you have all the necessary type information already.

I don't really see a good solution. I don't think using the generic type as a cast is a valid strategy. Maybe it's an option to add run-time type-checking to the result? Then you can throw explicitly if you do instant<string> but it would return Translation[] or TranslationObject or null or undefined. I agree with your remark that as string isn't good because it can be null or undefined too. We use as string because we haven't made our codebase compatible with strictNullChecks yet. once we do that, we'll make it as string | undefined or something similar.

Barring a fundamental public API change (e.g. a different method that returns a different type union) or runtime type checking, I don't see a valid solution. However, that might be a conclusion in and of itself, in which case this issue might be closed with a won't fix decision.

[rbalet]

@Ghostbird Thanks for the openness behind this discussion.

I'm agreeing with everything you say, but therefore will keep the issue open as it underlines a problem with the actual architecture.

That said, I would merge the proposed PR as it would be a nice addition for the strictNullChecks without any cost related to the existing implementation.

Cheers

[Ghostbird]

Yeah, that's true. I don't have anything against the PR as is. I just don't see our code base using the feature, as we'd prefer to cast "visibly".

[netoctone]

@Ghostbird @rbalet Typescript is pretty bad with overloaded functions. Pretty much the only way to avoid any (or other unusable union return type) is to deprecate get and introduce separate functions like getKey(key: string, interpolateParams?: InterpolationParameters): Observable<string> getKeys(keys: string[], interpolateParams?: InterpolationParameters): Observable<Record<string, string>>

[rbalet]

@netoctone Yes, that seems correct and it was what we where thinking about.

We've started other, more important topic though, so this will have to wait, or if anybody is motivated, they could try out, PR a welcome : )

I would let the get() though, as I assume many people use it the "dirty" way and it would require a to big of a refactoring for them..