moleculerjs / moleculer

:rocket: Progressive microservices framework for Node.js
https://moleculer.services/
MIT License
6.17k stars 587 forks source link

added service type as generic type argument #1291

Open prasadmadanayake opened 5 months ago

prasadmadanayake commented 5 months ago

Added service type as generic type argument

:memo: Description

updated this type definitions for

:gem: Type of change

:scroll: Example code


    export interface IMySettings extends ServiceSettingSchema{}

    export interface IMyService extends Service<IMySettings>{
        custom: string
    }

    export class MyService implements Partial<ServiceSchema<IMySettings, IMyService>>{
        name: string = ""
        actions: ServiceActionsSchema<IMySettings, IMyService> = {
            "aa":{
                async handler(cy){
                    console.log(this.custom)
                }
            }
        }
        methods: ServiceMethods<IMySettings, IMyService> = {
              async a2(){
                  console.log(this.custom)  
              }
        }

        events: EventSchemas<IMySettings, IMyService> = {
            "a3":{
                async handler(cy){
                    console.log(this.custom)
                }
            }
        }

    }

:vertical_traffic_light: How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

:checkered_flag: Checklist:

ccampanale commented 5 months ago

I like this idea. We've created and used custom types this way in our system for years but since upgrading to moleculer@0.14.26 we have type errors in all packages. As it exists, refactoring would be huge adjustment (effectively moving all custom service members to module members) but something like this would definitely make this a much smaller effort while being able to once again benefit from our type checking for custom service properties. I'm all for a change like this.

shawnmcknight commented 3 months ago

I'm having some trouble understanding how the example works. This is creating a new class, but that class doesn't extend the base Service class which I don't believe would work with moleculer-runner. I suppose if this class was instantiated manually and passed to broker.createService() it would just think it was a service schema object, but I'm not seeing anything in the createService workflow which would copy extra properties to the Service instance.

I might be missing something, but I don't see how that custom property would be available at runtime. Can you fill in the blanks for me?

prasadmadanayake commented 3 months ago

Sorry for the confusion with the example. This is just the type declaration extension for service schema. Implementation will be the same as in moleculer service schema implementation. So we need to set the custom property in created lifecycle hook.


export interface IMySettings extends ServiceSettingSchema{}

export interface IMyService extends Service<IMySettings>{
    custom: string
}

export class MyService implements Partial<ServiceSchema<IMySettings, IMyService>>{
    name: string = ""
    actions: ServiceActionsSchema<IMySettings, IMyService> = {
        "aa":{
            async handler(ctx){
                console.log(this.custom) // this resolves type correctly
            }

        }

    }
    created = function(this: IMyService){
        this.custom = "initialized"

    }
}
shawnmcknight commented 3 months ago

I have a couple of high-level thoughts with regards to this PR:

  1. It seems like the S generic which is used for for the settings schema is no longer really valuable since it would be available on the service type. If you're already supplying the Service type that just means the consumer needs to specify both things while the settings can be inferred from the Service. Since this is the next branch, it seems like now would be a good time to introduce a breaking change to these types.
  2. For these generics where Service is the default value, it seems like there should be a constraint applied to ensure the generic value is Service. I can't think of a scenario where you'd want to allow someone to specify a type that wasn't constrained by Service.
  3. It would be good to use a more descriptive generic than T for these service generics. Generally, I prefer to see generics using a T prefix with a more meaningful name behind it -- in this case something like TService.
  4. The formatting (spacing, etc.) is inconsistent in this PR. Please make sure to let prettier autofix this formatting for consistency purposes.