feat: Expand the type for ServiceSchema to allow for typed lifecycle handlers

marceliwac commented 9 months ago

This change expands the typings for the ServiceSchema, allowing it to accept the "service-this" type used by the lifecycle handlers in the typescript template. Please see this issue for a more complete explanation.

:memo: Description

Change the type of the ServiceSchema to accept two generic types, latter of which is new and used to provide types for the lifecycle methods (created, started, stopped).

The typescript template will require a minor change if this change is accepted.

:dart: Relevant issues

moleculer-template-project-typescript #70

:gem: Type of change

[x] New feature (non-breaking change which adds functionality)

:vertical_traffic_light: How Has This Been Tested?

The example code to verify that this change works is outlined in moleculer-template-project-typescript #70.

:checkered_flag: Checklist:

[x] My code follows the style guidelines of this project
[x] I have performed a self-review of my own code
[x] I have added tests that prove my fix is effective or that my feature works
[x] New and existing unit tests pass locally with my changes
[x] I have commented my code, particularly in hard-to-understand areas

marceliwac commented 7 months ago

@icebob , Apologies for any confusion!

I updated the test/typescript/hello-world/greeter.service.ts (i.e. service schema) to the one used in the typescript template repository, since I couldn't find any in-code uses of the hooks or any tests related to that schema. I must be missing something, most likely the actual test that use this code. Could you please point me to the tests you're referring to?

I tried running all the tests specified in package.json which all pass fine (with the exception of memory leak tests, which I am guessing are unrelated).

icebob commented 7 months ago

I've checked again the code and it looks the test can be fine, but you removed a lot of things from the original greeter service, e.g. the hooks which uppercaseing the context params. it would be good if you keep the same logic in the service file.

If you should modify the test files, it means the changes contains breaking changes.

marceliwac commented 6 months ago

@icebob You're absolutely right, sorry for that!

I reverted the deletions and tweaked them to comply with the service definition. I also added the implementation for the uppercase() method (which was declared as existing on the service but didn't) and replaced the direct call to toUpperCase() to a call to that method. Finally, I added the missing type delcaration for this where appropriate.

icebob commented 6 months ago

Great, thanks!

shawnmcknight commented 3 months ago

@marceliwac I've just updated moleculer versions and I'm encountering type errors as a result of this change.

Can you clarify why the ServiceSchema generic has a default of void while the lifecycle handlers have a default of Service<S>? This causes a conflict in scenarios where both types are being used along with their defaults. Is there a reason why ServiceSchema didn't have the same default as the lifecycle handlers?

marceliwac commented 3 months ago

@shawnmcknight

Apologies for the late reply and thanks for bringing this up!

You're right, these should in fact use Service<S> rather than void. The S type also seems to be redundant:

type ServiceSyncLifecycleHandler<T> = (this: T) => void;
type ServiceAsyncLifecycleHandler<T> = (this: T) => void | Promise<void>;

interface ServiceSchema<S = ServiceSettingSchema, T = Service<S>> {
    name: string;
    version?: string | number;
    settings?: S;
    dependencies?: string | ServiceDependency | (string | ServiceDependency)[];
    metadata?: any;
    actions?: ServiceActionsSchema;
    mixins?: Partial<ServiceSchema>[];
    methods?: ServiceMethods;
    hooks?: ServiceHooks;

    events?: ServiceEvents;
    created?: ServiceSyncLifecycleHandler<T> | ServiceSyncLifecycleHandler<T>[];
    started?: ServiceAsyncLifecycleHandler<T> | ServiceAsyncLifecycleHandler<T>[];
    stopped?: ServiceAsyncLifecycleHandler<T> | ServiceAsyncLifecycleHandler<T>[];

    [name: string]: any;
}

Ideally, if lifecycle handlers were nested within a separate property there would be a much cleaner solution that uses ThisType<T> utility like so:

type ServiceSyncLifecycleHandler = () => void;
type ServiceAsyncLifecycleHandler = () => void | Promise<void>;

type ServiceLifecycle<T> = {
    created?: ServiceSyncLifecycleHandler | ServiceSyncLifecycleHandler[];
    started?: ServiceAsyncLifecycleHandler | ServiceAsyncLifecycleHandler[];
    stopped?: ServiceAsyncLifecycleHandler | ServiceAsyncLifecycleHandler[];
};

interface ServiceSchema<S = ServiceSettingSchema, T = Service<S>> {
    name: string;
    version?: string | number;
    settings?: S;
    dependencies?: string | ServiceDependency | (string | ServiceDependency)[];
    metadata?: any;
    actions?: ServiceActionsSchema;
    mixins?: Partial<ServiceSchema>[];
    methods?: ServiceMethods;
    hooks?: ServiceHooks;

    events?: ServiceEvents;
        lifecycle?: ServiceLifecycle & ThisType<T>;

    [name: string]: any;
}

But because the lifecycle handlers are direct properties of the service, this makes things slightly messier. ServiceSchema is an interface and extending it with a type of LifecycleHandlers<T> (which is legal in TS >= 2.2) doesn't seem to apply the ThisType<T> correctly:

type ServiceSyncLifecycleHandler = () => void;
type ServiceAsyncLifecycleHandler = () => void | Promise<void>;

type ServiceLifecycle<T> = {
    created?: ServiceSyncLifecycleHandler | ServiceSyncLifecycleHandler[];
    started?: ServiceAsyncLifecycleHandler | ServiceAsyncLifecycleHandler[];
    stopped?: ServiceAsyncLifecycleHandler | ServiceAsyncLifecycleHandler[];
} & ThisType<T>;

interface ServiceSchema<S = ServiceSettingSchema, T = Service<S>> extends ServiceLifecycle<T> {
    name: string;
    version?: string | number;
    settings?: S;
    dependencies?: string | ServiceDependency | (string | ServiceDependency)[];
    metadata?: any;
    actions?: ServiceActionsSchema;
    mixins?: Partial<ServiceSchema>[];
    methods?: ServiceMethods;
    hooks?: ServiceHooks;

        // `created`, `started` and `stopped` are defined, but don't have the correct type for `this`

    events?: ServiceEvents;
    [name: string]: any;
}

On a side note, I also think that the generic T should be passed to the methods, events and handlers so we don't have to explicitly set this in each of the handlers. That matters less, because it still needs to be done if the function is defined outside of schema definition.

@icebob Any thoughts on this? Should I submit a PR?

icebob commented 2 months ago

I have no idea due to lack of TS experience. @shawnmcknight ?

MichaelErmer commented 2 months ago

what @marceliwac suggested would resolve our issues with the current version

marceliwac commented 2 months ago

@shawnmcknight, @icebob, @MichaelErmer I've pushed the PR to address this. I would appreciate a second pair of eyes sanity checking it if you could find some time. Hopefully, the added TS tests should prevent similar issues in the future.

I'm not 100% sure how to verify that this type of the lifecycle handlers (created, started and stopped) actually poitns to the instance of Service type, hence the use surrogate types (ServiceSyncLifecycleHandler and ServiceAsyncLifecycleHandler) in the tests. If you have better ideas, I'm open to suggestions.

moleculerjs / moleculer