Allow non-async function in service.started() and service.stopped()

[disce-omnes]

Is your feature request related to a problem? Please describe.

Type definitions for ServiceSchema.started() and ServiceSchema.stopped() (link) require async functions. However passing non-async functions there also works.

There are use cases when you don't need service lifecycle event handlers to be async. For instance, service A can call service B on startup, waiting for it, if needed, but without blocking the start of service A.

Additionally these functions are wrapped in Promise.method before execution in any case (started, stopped).

Describe the solution you'd like

Allow to pass non-async functions to ServiceSchema.started() and ServiceSchema.stopped()

Describe alternatives you've considered

You can always pass async functions there, however, in my experience, it makes it harder to reason about the code.

Additional context

I can submit a PR if this change is approved.

[intech]

For instance, service A can call service B on startup, waiting for it, if needed, but without blocking the start of service A.

Maybe the asynchronous method on events will suit you?

{
    name: "serviceA",
    events: {
        async "$broker.started"() {
           await this.broker.waitForServices("serviceB", 10000, 1000);
           await this.broker.call("serviceB.action");
        }
   }
}

[ccampanale]

@disce-omnes In my opinion, if the success of the service start-up depends on making a call to an external service, that service should be set as a dependency, a promise should be returned from the started life-cycle hook, and the result (whether it was able to do what was required) should be used to determine whether that promise resolves or rejects and in turn if the service started correctly.

If the service does not depend on whatever is being done in the startup lifecycle hook, it should be moved elsewhere. I personally like @intech's example above which listens for the built-in/local broker started event, waits for the dependent service to be available, then makes the call. Like the started hook, this event is broadcasted allowing each instance of the service to receive and act on the event.

A real world example: I developed a service in my teams system which depends on other services in order to perform a synchronization operation when it starts. I consider this a vital part of the service's life-cycle and an important part of the role it plays in the system, so I've implemented this "routine" in the started hook. This way, if the sync fails, I can fail the entire service quickly. In the right environment (assuming DevOps, monitoring, and alerts are in place) this should be quickly recognized and remediated (ideally) and I believe this is good practice.

Lastly, I would say, if returning a promise (which can be very simply either async started() {} or started() { return Promise.resolve().then() }, etc.) "makes it harder to reason about the code" then perhaps what you are doing there doesn't belong or you don't need the hook at all? Furthermore, I believe the specification allows for an undefined started hook so you could remove it entirely.

Just my two cents - happy coding!

[disce-omnes]

You're right, in this example $broker.started works and is a better place for this type of logic.

But I used it just as an example, my main point was that types should define what can be done with a code entity. Here passing non-async function works, but the type system prohibits it. So, to me, either the types should be changed to allow it, or passing non-async function should lead to a runtime error. Second option is a breaking change & obviously unnecessary, hence my suggestion.

It's as if a function is typed to accept only a string as a parameter, but passing it a number also works, because the function converts it to a string in its body.

Another possible use case is sending a metric on service startup that requires async action to prepare its data. It also should be non-blocking, because metrics shouldn't interfere with service's main logic. Or started() might be used to initiate some routine with setInterval, so you don't need async. These don't involve calling other services, so started() makes more sense than $broker.started.

[shawnmcknight]

This request is very similar to that of the change from #1099 . If Moleculer is wrapping the handler in a promise returning function, then the type definitions should likely allow consumers to provide a function which is not promise returning.