Action should not have handler required in interface definition

[shawnmcknight]

Prerequisites

Please answer the following questions for yourself before submitting an issue.

• I am running the latest version
• I checked the documentation and found no answer
• I checked to make sure that this issue has not already been filed
• I'm reporting the issue to the correct repository

Expected Behavior

Allow action to be defined without a handler property in TypeScript.

Current Behavior

Interface definition for Action is:

interface Action {
  ...
  handler: ActionHandler;
  ...
}

This marks the handler as required.

Failure Information

Technically speaking, every action ultimately needs a handler. However, in my use case I have created a Mixin which defines the handler. The service is adding additional properties to augment the mixin (e.g. cache). In this manner the handler defined in the Mixin is reusable and allows the service to define properties dynamically.

Steps to Reproduce

1. Use TypeScript
2. Create a Mixin which defines an action with a handler.
3. Create a service which uses that Mixin.
4. Add a same-named action to the service. Add properties other than handler (e.g. cache).
5. TypeScript will error because it expects the service's action to have a handler.

At runtime, this configuration will work because the action definition is merged deeply into the mixin. However, TypeScript will not allow it to be compiled.

Reproduce code snippet

Mixin:

const mixin: ServiceSchema = {
  name: 'mixin',
  actions: {
    foo: () => {},
  },
};

Service:

const service: ServiceSchema = {
  name: 'service',
  actions: {
    foo: {
      cache: {
        ttl: 60,
      },
    },
  },
};

Context

• Moleculer version: 0.13.5
• NodeJS version: 10.15.0
• Operating System: Windows 10
• Typescript Version: 3.3.3

My thought process here is to simply mark the handler as an optional property of the Action interface. I'm happy to create a PR if there is agreement on that.

Thanks!

[icebob]

Good catch. Please create a fix PR. Thanks in advance!

[icebob]

The same issue with event handlers, I think...

[shawnmcknight]

The same issue with event handlers, I think...

You're probably right. It's not as obvious a use case since a ServiceEvent has far fewer properties than an Action, but it could result in the same problem. I'll add it to the PR when I submit it.

[Telokis]

Hey guys!

I'm heavily modifying the typings for Moleculer in order to provide perfect autocomplete and type-safety for both actions definitions and ctx.calls.

Since this requires several in-depth changes, I copy/pasted the .d.ts file and I'm directly modifying it to add the required features.

I just stumbled on a small problem caused (in part) by this issue:

The type ServiceActionsSchema is an union of ActionHandler and ActionSchema (and boolean though I don't know why, doesn't really matter)

The issue is that the definition of ActionSchema is very open (mainly due to this issue): absolutely every property is optional and its index signature uses any:

{
              name?: string;
              visibility?: ActionVisibility;
              params?: ActionParams;
              service?: Service;
              cache?: boolean | ActionCacheOptions;
              handler?: ActionHandler;
              tracing?: boolean | TracingActionOptions;
              bulkhead?: BulkheadOptions;
              circuitBreaker?: BrokerCircuitBreakerOptions;
              retryPolicy?: RetryPolicyOptions;
              fallback?: string | FallbackHandler;
              hooks?: ActionHooks;

              [key: string]: any;
}

One side-effect of this situation is that any function will match this type because any function has string properties with any as values.

Meaning it throws off my error detection because using a function that returns a boolean where a string is expected won't satisfy the constraint of ActionHandler but it will satisfy the constraint of ActionSchema (any function would).
I tried making handler required in ActionSchema (the very issue discussed in this thread) and it makes it work as I expected.

Since it's a pretty big deal for the perfect type-safety I'm working on, I'd like to find a workaround which would be satisfying for everyone.

The best solution I could think of was to be harsher on the type of the index signature and not use any.

After tinkering, I thought I could use a pretty generous union:

string | boolean | any[] | number | Record<any, any> | null | undefined

However, I'm not a moleculer god and aren't really aware of what strange patterns could be used in the shadows.
The goal is to specify something that would not allow a raw function to match but still be permissive enough for common usage.

@shawnmcknight @icebob Do you have any feedback regarding this specific situation?

Thanks in advance for reading!

[icebob]

You can't limit the action schema because it allows defining any other custom property. E.g. you can write a middleware which checks a custom property in all actions. If you don't allow it, many users can't use custom middleware/mixins.

The boolean action definition is a method to disable an action which comes from a mixin.

[Telokis]

I totally understand your point, yes.

I don't want it to be too constrained or it will be less convenient and that's why I wanted to use a very permissive union instead.

[Telokis]

For future reference, I asked a question about my comment on StackOverflow: https://stackoverflow.com/questions/64305700/prevent-very-permissive-object-type-from-matching-any-function