Update types for Typescript - fixed GenericObject

[ipetrovic11]

Current GenericObject makes a lot of trouble when using Moleculer with Typescript.
Extending it allows any property on interface, but we are using Typescript because we want to use strict types.

📝 Description

💎 Type of change

• Bug fix (non-breaking change which fixes an issue)

[icebob]

#599 ?

[coveralls]

Pull Request Test Coverage Report for Build 2100

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 93.658%

---

Totals
Change from base Build 2093:	0.0%
Covered Lines:	7768
Relevant Lines:	8008

---

💛 - Coveralls

[icebob]

@kthompson23 @shawnmcknight what are your opinions?

[shawnmcknight]

I'd be interested in hearing more of a rationale on this as well as if #599 possibly solved that scenario. Can you provide any more details @ipetrovic11 ?

I am in agreement that in many places the use of GenericObject in the types eliminates type safety. However, the type that GenericObject defines is an index signature, and its common enough that later versions of TS include Record<K, T>. I would be hesitant to suggest that all uses of GenericObject in the declaration file are not valid and that possibly #599 or an extension thereof might be more appropriate.

Additionally, the reasons I disliked GenericObject are magnified when it comes to use of any. any is a free-for-all and all type safety is out the window (in both directions that values could be cast). I'm against use of any in all scenarios except those where the type of the value is truly irrelevant. I doubt that's the case for many of the current uses of GenericObject.

[kthompson23]

I agree with @shawnmcknight but would like to hear more about the rationale behind this change. Sometimes if a type is too restrictive to begin with it's hard to extend but like Shawn said GenericObject is very loose to begin with

[ipetrovic11]

@shawnmcknight @kthompson23 @icebob
Thanks for taking time to think about this and sorry for my late response.

On our project we are using "strict" mode in ts-config, which is what triggered the issue at the beginning and it doesn't feel like that is bade use case. We want to be sure which types/interfaces we are working with.

Error leads to:

Since it is defined as Context, it leads to Context<GenericObject, GenericObject> which is where incompatibility is actually happening. And it is not with those types that use Pick, Omit or Partial, it is with any interface that we define.

Or this could be other possible solution:

Or something similar at that point that you have in mind that could further enforce the right type.

[shawnmcknight]

On our project we are using "strict" mode in ts-config, which is what triggered the issue at the beginning and it doesn't feel like that is bade use case. We want to be sure which types/interfaces we are working with.

I was trying to figure out why we have not run into a similar issue as we also type all our params and meta for Context. After giving it some thought and trial and error, I found that we have all the strict options turned on with the exception of strictFunctionTypes. When I looked to enable that, I found it was causing too many errors (possibly even this specific one you're running into) and I decided that the benefits of that flag were very small and decided it was not worth the hassle.

With that said, I understand the desire to have things as strict as possible so I wouldn't suggest that the answer is "turn off that setting".

Looks like this could be solution as well:
type GenericObject = { [name: string]: any } | {};
So far, this is the one that I believe is the most adequate.

In looking at this, I couldn't figure out why it would solve the problem. Your parameter type is not provably compatible with either the index signature or empty object. I tried making my version of GenericObject look like this and I ended up with a similar error as before, but this time due to incompatibility with {}.

The ideal solution to this problem would be to eliminate the defaults for the generics in Context entirely. However, when I attempted to model something like that previously, it let to an enormous cascading effect in the declaration file because of the number of other definitions that used Context without providing types for the generics (ActionHandler being one of them). In order to eliminate the default and avoid just cascading defaults further upstream, generics would need to be provided all over the place and many of them being unpleasant because Context is often nested fairly low in nested interface definitions. I don't know of a way to specify a generic's type at that low a level -- to the best of my knowledge it has to be specified on the top-most interface and cascade down to the child interfaces (if you know of any other way this could be approached I'd be interested in hearing it).

So with all that said, although it feels a little off to me, I think the solution I'd recommend is that where the ActionHandler's ctx parameter gets a type of Context<any, any>. Type safety is a little out the window if someone doesn't type their own version of the function's ctx parameter, but that should be done anyway if you want type safety.

[ipetrovic11]

@shawnmcknight that was just haste comment, that made things work for me while I was in watch mode for a moment.

Switching ActionHandler params to (ctx: Context<any, any>) sounds good to me.

Looking further at types definitions not sure how much benefits we have from this class definition: Context<P = GenericObject, M = GenericObject>, maybe we can improve it there already to: Context<P = any, M = any>.

[icebob]

So any conclusion?

[icebob]

ping @ipetrovic11 @shawnmcknight

[ipetrovic11]

@icebob
I can make new PR to 0.14.
There are few minor improvements that I have on it.

About current issue I would got with improvement on Context definition:
Context<P = any, M = any>

[shawnmcknight]

About current issue I would got with improvement on Context definition:
Context<P = any, M = any>

In 0.14 the Context class was changed to:
class Context<P = {}, M extends object = {}> {

This was done specifically to essentially force TS developers to provide type definitions. In hindsight, I would have recommended this change actually be:
class Context<P = unknown, M extends object = {}> {
since unknown is more appropriate for params now that params does not have to be an object.

With that in mind, I'd be very hesitant to recommend allowing Context to be changed to any for its generics. This would return the default state of Context to one without any sort of type safety which I think is a mistake.

I'm still good with changing the ActionHandler definition to have ctx: Context<any, any>. I don't think there's really an effective way of specifying generics values for the handler that can make their way to the handler definition. I believe this would still resolve the issue at hand.