Improve type definitions to allow strict type-checking of ctx.call, actions definitions and events

[Telokis]

Is your feature request related to a problem? Please describe.
The current type definitions does not provide any type-safety regarding ctx.call, ctx.emit, actions definitions in service and events handlers in services.

Describe the solution you'd like
We could use several meta-types and a user-defined mapping allowing us to properly type everything.
In particular, I was thinking about the following for user-defined mappings related to actions:

type ServiceActions = {
    "v1.chat": {
        get: (arg: { toto: string }) => boolean;
        list: () => number;
    };
    "v1.characters": {
        get: (arg: { id: string }) => string;
    };
};

The meaning of this type is:

• We have two services available: v1.chat and v1.characters.
• The service v1.chat defines two actions: get and list
• The service v1.characters defines one action get
• The action v1.chat.get takes a parameter containing toto: string and returns a boolean
• The action v1.chat.list takes no parameter and returns a number
• The action v1.characters.get takes a parameter containing id: string and returns a string

Given this mapping, we have every information we need.

I thought about using it the following way (take a look at the inheritance):

class ChatService extends Service<ServiceActions, "v1.chat"> {
    constructor(broker: ServiceBroker) {
        super(broker);

        this.parseServiceSchema({
            name: "chat",
            version: "v1",
            actions: {
                async get(ctx) {
                    const str = await ctx.call("v1.characters.get"); // Type error: `id: string` was expected as parameter

                    return ctx.params.toto; // Triggers a type error because it should return a boolean
                },
                list: {
                    handler(ctx) {
                        return "plop"; // Triggers a type error because it should return a number
                    },
                },
            },
        });
    }
}

To properly type events, we also rely on a mapping of the following form:

type ServiceEvents = {
  "event1": { id: string };
};

Meaning "There is one event event1 and its payload is an object containing an id of type string."

We don't need anything else in order to have strongly-types events. This mapping would need to be passed as a third template parameter to the Service class, though.

Describe alternatives you've considered
I haven't considered any alternative, this is the only proper solution I could think of that would cover all my requirements.

Additional context
A potential issue with the implemention I think about is that it relies on TypeScript 4.1 (Specifically this issue). This is used to generate v1.characters.get from the keys of the mapping.

This feature is a lifesaver since it lets us stay DRY (In the action definition we don't use the service name but we use it in the action call)

I've already started working on the implementation because I think it's mandatory when using such a library in TypeScript.
I currently have the action implementation strongly typed and am working on ctx.call.

I can show the code and explain everything if it is relevant.
The following code was the proof of concept used to find the way to go, if someone is curious:

interface Actions {
    "v1.auth": {
        get: (toto: string) => boolean;
        list: () => number;
    };
    "v1.characters": {
        get: (id: string) => string;
    }
}

type Func = (...arg...

Playground Link

Just to clarify: I intend to implement this. I just thought I could open an issue to discuss the used interface because it might be useful for people. It could become a PR if the solution I implement is satisfying for everyone.

And I stumbled on this "problem" while tinkering: #467 (comment). It's not a problem without this feature but gets in the way of the implementation I'm working on.

[icebob]

It doesn't do something similar? https://github.com/jarvify/moleculer-ts

[icebob]

Or this: https://github.com/bytetechnology/moleculer-service-ts

[Telokis]

The first one requires a generation step and the second one seems way too complex.
This can be done with nothing that complicated.

Moreover, I feel like actions itself should be type-safe based on the mapping (it's easier and, IMO, more robust than inferring the proper types from the action)

I also feel like this is an important enough feature that it should be "official".

If you want to take a look, I can post the updates typings in order to make the actions definitions typesafe, it's not a big change.

[Telokis]

Today I continued and improved my proof of concept to create a Call<Mapping> type which perfectly types the call function for both the broker and context.
A toy is available below if you want to take a look:

type Actions = {
    "v1.auth": {
        get: (toto: string) => boolean;
        list: () => number;
    };
    "v1.characters": {
        get: (id: string) => string;
    }
}

// Type used...

Playground Link

Edit: I updated the link with the latest version.

[Telokis]

Update: Actions definitions and calls are 100% strongly typed based on the mapping

Here is the real situation where actions are automatically typed based on the mapping.
You can skip to line 1756 to see the example. Everything above this line is the modified moleculer type definitions.

import { EventEmitter2 } from "eventemitter2";

declare namespace Moleculer {
    /**
     * Moleculer uses global.Promise as the default promise library
     * If you are using a third-party pro...

Playground Link

My goal is to not break anything in existing codebases. Though I'd like to be honest, I'm not sure if I managed to do it, some tests will be required.

I use tricks like never as default generic arguments in order to use the old behavior if not set.

For example, in Context, I did something like so:

// Not strictly typed call function
type LegacyCall = (
    (<T>(actionName: string)=> Promise<T>) &
    (<T, P>(actionName: string, params: P, opts?: CallingOptions)=> Promise<T>)
);

class Context<
    P = unknown,
    M extends object = {},
    ActionsMapping extends ActionsMappingBase = never // Added this generic parameter
> {
    // If the mapping is not provided, we fallback to the old definition
    // This ensures minimal support but still allows proper type-safety if provided.
    call: ActionsMapping extends never ? LegacyCall : Call<ActionsMapping>;
}

Next step: Same thing but for events

[Telokis]

Update: Events definitions and emit/broadcast are 100% strongly typed based on a mapping

Below is the same example as the one used in my last comment but updated to add the type magic for events:

import { EventEmitter2 } from "eventemitter2";

declare namespace Moleculer {
    /**
     * Moleculer uses global.Promise as the default promise library
     * If you are using a third-party pro...

Playground Link

Same as above, people wishing not to use everything I added will not be forced to. Every default is never and there are multiple checks made against never to fallback to the previous types:

        call: [ActionsMapping] extends [never] ? LegacyCall : Call<ActionsMapping>;
        emit: [EventsMapping] extends [never] ? LegacyEmit : Emit<EventsMapping>;
        broadcast: [EventsMapping] extends [never] ? LegacyEmit : Emit<EventsMapping>;
        broadcastLocal: [EventsMapping] extends [never] ? LegacyEmit : Emit<EventsMapping>;

ServiceEvent function deduction issue

There is one issue with the types for events:

ServiceEventLegacyHandler conflicts with ServiceEventHandler. They are both functions and TypeScript is not able to automatically detect that ServiceEventHandler is used if we define a function taking a single parameter:

events: {
  "MyEvent"(ctx) {
    // 'ctx' is deduced as 'any' because there are two possibilities.
  }
}

You can force the deduction by taking more than one parameter, it will collapse the ambiguity and TypeScript will properly detect that ServiceEventLegacyHandler is being used.
But this is not ideal since ServiceEventLegacyHandler is legacy and shouldn't be the preferred prototype (I suppose).

A solution I was thinking about is to let the user provide a specific flag in order to completely disable ServiceEventLegacyHandler so that the only possibility is ServiceEventHandler.
I thought about a specific key/value inside the events definition. Something like __disableLegacyHandler: true.
This is not implemented in the above example because I wished to discuss the matter with you, first. I don't see any drawback to this solution but it's still a design choice to make. (Even though it's only TypeScript-related.)

I consider the issue to be solved with this final example even though two points are still open:

• The too permissive type mentionned in this comment
• The ServiceEventLegacyHandler/ServiceEventHandler ambiguity mentioned in the last paragraph of this comment.

@icebob May I ask for your feedback regarding everything I said in this thread and my final implementation, please?

Thanks in advance for taking the time to read though my reports and work. I'll respond to questions/feedback as quickly as possible!

[icebob]

I'm not a TS guy, so I can't review it, but pinging others.
@shawnmcknight @kthompson23 @Wallacyf could you review this idea? How will it affect the existing TS projects?

[Telokis]

Yes, I understand, thanks for pinging!

Regarding the existing TS projects my goal was to not break anything (hence the multiple conditional types to make sure the defaults are the same as today)

I have a question non-TS, though: on my project where I use this type-definition, I tried to wanted to use only ServiceEventHandler so I commented ServiceEventLegacyHandler to force TypeScript to match the proper one.

This event handler takes a single parameter: ctx. However, when using it, I noticed it didn't work and the first parameter was always the payload of my event. Is there some kind of discrepancy in the type definitions? I admit I didn't check the code in depth to take a look at it.

[Wallacy]

Well... I did something similar by encapsulating my types using typescript.
This current implementation looks very promissing, I'm going to do a test tomorrow on the project that I use the moleculer to look at this issue of backwards compatibility. But at first glance the idea seems very nice, and limiting to TypeScript 4.1 is not a big problem, at least for me.

But is import to note that molecule has support to dynamic services load. So, sometimes we need to call a service that was not available at build time. Anything in that direction must take this into account. For sure this can be configurable at typescript level.

[icebob]

I have a question non-TS, though: on my project where I use this type-definition, I tried to wanted to use only ServiceEventHandler so I commented ServiceEventLegacyHandler to force TypeScript to match the proper one.

The legacy event handler signature is (payload, sender, eventName) but after 0.14 there is a new with (ctx). But for backward compatibility the parseServiceSchema tries to find out which signature you are using in your event handler. If it find ctx it calls with the Context instance, otherwise it calls based on the legacy signature. Here is the relevant code

So if you got the payload in first parameter, it means the service can't detect the signature. But you can force the new signature if you set context: true in the event schema, like this:

events: {
    "user.created": {
        context: true,
        handler(ctx) {
            // ...
        }
    }
}

[Telokis]

@Wallacy

I'm going to do a test tomorrow on the project that I use the moleculer to look at this issue of backwards compatibility.

Thank you very much, I hope everything will be fine!

limiting to TypeScript 4.1 is not a big problem, at least for me.

The issue is that TypeScript 4.1 has not been released yet. I updated to beta for my project but it breaks Eslint (typescript-eslint/typescript-eslint#2583) which is a bit annoying. If we decide this can be merged into Moleculer, we should wait for TypeScript 4.1 to be properly released, at least.

But is import to note that molecule has support to dynamic services load. So, sometimes we need to call a service that was not available at build time. Anything in that direction must take this into account. For sure this can be configurable at typescript level.

In my opinion, this is not a real issue: if you know you will call a specific service at build time, you know what arguments you will provide and what type you expect to receive. This means you can add it to the mapping. Because a service is defined in the mapping doesn't mean you are required to implement it,
I tried to be flexible because of things like moleculer-db which declare actions that we don't implement ourselves but still use. In my real project, I did something like the following:

type DBService<Model> = {
    list: () => Array<Model>;
    get: (id: string) => Model;
};

type ServiceActions = {
    "v1.characters": DBService<CharactersModel> & {
        getByUser: (param: {userId: string}) => Array<CharactersModel>
    };
    "v1.users": DBService<UsersModel>;
};

It works fine even though I never implemented v1.users.get myself. As long as the mapping knows it exists, I can ctx.call it.

[Telokis]

@icebob

The legacy event handler signature is (payload, sender, eventName) but after 0.14 there is a new with (ctx). But for backward compatibility the parseServiceSchema tries to find out which signature you are using in your event handler. If it find ctx it calls with the Context instance, otherwise it calls based on the legacy signature. Here is the relevant code

That is why it didn't work in my code: I destructured ctx meaning there was neither ctx nor context for moleculer to detect the expected signature.

So if you got the payload in first parameter, it means the service can't detect the signature. But you can force the new signature if you set context: true in the event schema, like this:

events: {
    "user.created": {
        context: true,
        handler(ctx) {
            // ...
        }
    }
}

Thank you, I guess I missed this in the documentation. This is something I can automatically detect with TypeScript to ensure handler is never deduced as LegacyEventHandler if context: true is set.

I am still thinking about a special property in the events definition in order to completely disable either LegacyEventHandler or EventHandler if the user wishes to enforce a specific style in its project.
But this might be considered out of scope for moleculer, I don't know.

I'll be frank, I never really used moleculer. When I discovered it I thought it was promising and decided to convert my project to use it. My project is in TypeScript and when I noticed the actions and events weren't strongly typed, I started working on this issue. This means that I'm not yet familiar with the possible intricacies of moleculer, sorry if I miss obvious things.

[Wallacy]

In my opinion, this is not a real issue: if you know you will call a specific service at build time, you know what arguments you will provide and what type you expect to receive. This means you can add it to the mapping. Because a service is defined in the mapping doesn't mean you are required to implement it,
...
It works fine even though I never implemented v1.users.get myself. As long as the mapping knows it exists, I can ctx.call it.

Thats my point. On Moleculer you can call something that you dont know at build time! The Repl for example has the option to load services from file/folder; Thats not a problem on my apps, but if someone relies on this dynamic behaviour will be a breaking change.

[Telokis]

It won't be a breaking change if they don't intentionally "enable" the strong type-checking by providing an action mapping and/or event mapping. (If I properly kept the backwards-compatibility, of course)

For the specific case you mention, I see your point. I feel like it is a compromise and a // @ts-ignore will probably be traded for better type-checking everywhere else.

It doesn't seem like a big deal to me but I may be missing something somewhere.

[Wallacy]

So, just did a small test here... i did not have much time to spend on that, but apear to be no downside in this solution (at least based on the last Playground);

I think you can open a PR and then, we can review the that directly on the project (more easy to test anyway);

And yes, we can hold the PR until 4.1 be released;

[shawnmcknight]

I have to admit, much of this is over my head at this point. Conceptually, I love the idea of automatic inference of types on remote services. As long as there is some potential for compatibility with the existing method of asserting types, it sounds like a fantastic idea. The biggest reason I can see for opt-out would be for the rather large number of moleculer users who don't use a single repo but still communicate with other services in other repos.
I'll be happy to try this out in our repo though once TS 4.1 is settled.

[Telokis]

I have to admit, much of this is over my head at this point. Conceptually, I love the idea of automatic inference of types on remote services. As long as there is some potential for compatibility with the existing method of asserting types, it sounds like a fantastic idea. The biggest reason I can see for opt-out would be for the rather large number of moleculer users who don't use a single repo but still communicate with other services in other repos.
I'll be happy to try this out in our repo though once TS 4.1 is settled.

I totally understand and the system is currently opt-in, it is not enforced on users and current code won't even notice the types changed.
One potential issue might be that there is no real middleground yet. Either everything is typed or nothing is.
I'm not really sure I could find a solution that would allow us to have this behavior, though.