-
-
Notifications
You must be signed in to change notification settings - Fork 587
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve type definitions to allow strict type-checking of ctx.call, actions definitions and events #822
Comments
It doesn't do something similar? https://github.com/jarvify/moleculer-ts |
The first one requires a generation step and the second one seems way too complex. Moreover, I feel like 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. |
Today I continued and improved my proof of concept to create a
Edit: I updated the link with the latest version. |
Update: Actions definitions and calls are 100% strongly typed based on the mappingHere is the real situation where actions are automatically typed based on the mapping.
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 For example, in // 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 |
Update: Events definitions and emit/broadcast are 100% strongly typed based on a mappingBelow is the same example as the one used in my last comment but updated to add the type magic for events:
Same as above, people wishing not to use everything I added will not be forced to. Every default is 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 issueThere is one issue with the types for events:
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 A solution I was thinking about is to let the user provide a specific flag in order to completely disable I consider the issue to be solved with this final example even though two points are still open:
@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! |
I'm not a TS guy, so I can't review it, but pinging others. |
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 This event handler takes a single parameter: |
Well... I did something similar by encapsulating my types using typescript. 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. |
The legacy event handler signature is So if you got the events: {
"user.created": {
context: true,
handler(ctx) {
// ...
}
}
} |
Thank you very much, I hope everything will be fine!
The issue is that TypeScript 4.1 has not been released yet. I updated to
In my opinion, this is not a real issue: if you know you will call a specific 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 |
That is why it didn't work in my code: I destructured
Thank you, I guess I missed this in the documentation. This is something I can automatically detect with TypeScript to ensure I am still thinking about a special property in the events definition in order to completely disable either 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. |
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. |
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 It doesn't seem like a big deal to me but I may be missing something somewhere. |
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; |
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 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. |
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:
The meaning of this type is:
v1.chat
andv1.characters
.v1.chat
defines two actions:get
andlist
v1.characters
defines one actionget
v1.chat.get
takes a parameter containingtoto: string
and returns aboolean
v1.chat.list
takes no parameter and returns anumber
v1.characters.get
takes a parameter containingid: string
and returns astring
Given this mapping, we have every information we need.
I thought about using it the following way (take a look at the inheritance):
To properly type events, we also rely on a mapping of the following form:
Meaning "There is one event
event1
and its payload is an object containing anid
of typestring
."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:
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.
The text was updated successfully, but these errors were encountered: