Moleculer: TS - 0.14.0 - New possible contex param type of null is a breaking change

[kthompson23]

In the next branch, the TypeScript type for the params member in the Context class was changed from P to P | null.

params: P | null;

In ServiceBroker.call params are being defaulted to empty object if passed in as null so this isn't a breaking change code-wise but for TypeScript users, any place where they try to use params off of context after a call now have to either perform a null check or silence the warning with a null assertion operator.

Aside from the new context-based events where params may be null if no payload is passed in, there doesn't seem to be any place where an external consumer of Moleculer's context class could encounter a null params property.

Would it be possible to also default params to an empty object for context-based events and revert the params type to just be P instead of P | null ?

[icebob]

We should discuss it with other users, because I think a null value means other than an {} value. E.g.:

broker.broadcast("config.changed.server-name", "http://server:3000"); 
// OK: ctx.params: "http://server:3000"

broker.broadcast("config.changed.server-name", null); 
// Wrong, because ctx.params: {} which is not the desired value.

[shawnmcknight]

I think for the initial issue presented here, this can be solved fairly easily. Since with moleculer the only type-safety you can achieve is through diligent typing of your Context, call, emit, and broadcast, I think the params: P | null; would be better served as params: P; The programmer has declared a type (or used the default, more on that later) and written their code to work with that type. If null was what params would be, they could specify null as the type (or as part of a union type if it was one of a possible set of options). Although params will technically default to null, the programmer has explicitly typed it as something else and dropping the null from the type would eliminate the breaking change.

I also think the Context class could benefit from more rigorous typing than is presently in place, and a new version might be the ideal time to do that. I think it is _very_ poor practice for a TS programmer to accept the default generic types specified by Context, call, emit, and broadcast. Effectively all type safety is totally lost to the consumer if the defaults are accepted and, in my opinion, no TS programmer should want that. Its too easy in the current state to forget to provide types to these Generics and end up with no type safety at all. I'd recommend a change to Context that looks something like this, which includes the dropping of the null union mentioned above:

class Context<P = {}, M extends object = {}> {
...
  params: P;
  meta: M;
...
  call<T>(actionName: string): PromiseLike<T>;
  call<T, P>(actionName: string, params?: P, opts?: GenericObject): PromiseLike<T>;

  emit<D>(eventName: string, data: D, opts: GenericObject): void;
  emit<D>(eventName: string, data: D, groups: Array<string>): void;
  emit<D>(eventName: string, data: D, groups: string): void;
  emit<D>(eventName: string, data: D): void;
  emit(eventName: string): void;

  broadcast<D>(eventName: string, data: D, opts: GenericObject): void;
  broadcast<D>(eventName: string, data: D, groups: Array<string>): void;
  broadcast<D>(eventName: string, data: D, groups: string): void;
  broadcast<D>(eventName: string, data: D): void;
  broadcast(eventName: string): void;
...
}

Summarizing these proposed changes:

• Eliminate the default generics for params and meta being GenericObject (particularly since params can now be any serializable type at all)
• Defaulting the generics for params and meta to the Context class as empty object. I'd actually prefer to not provide any default at all, but that causes a major cascading effect on other types which reference Context which is an enormous challenge to mitigate. With empty object, you can't do a whole lot so its fairly similar to not providing any type at all.
• Adding an extends object constraint to meta since, to my knowledge, meta must be an object and any type provided here should be an object (correct me if I am mistaken).
• Eliminating the default return value generic for call. Return values would now have to be typed or TS will determine them to be unknown and any use of it will likely throw up a type error.
• Adding a new overload for call which will prohibit consumers from providing params that have not been typed, provided a type for the return value has also been provided (see previous bullet).
• Eliminating the default parameter generic for call. It appears that call will now (maybe always?) accept any type for params. The GenericObject constraint seems invalid and the defaulting of GenericObject isn't really providing any type safety.
• Eliminating the default generic for the type of data in broadcast and emit. TS will actually automatically infer the type here -- I don't think there is any way to stop it -- if one has not been provided. Therefore, defaulting this as any effectively has the same result as not providing a default.

I recognize that these changes might be an unpleasant conversion for anyone who hasn't provided concrete types. However, I can't really think of any reason why a TS programmer would want the defaults as is since they lose all type safety. You might as well be using vanilla JS with those defaults in place instead of TS. For those of us that are enforcing the discipline of providing types, it would reduce the risk of failing to provide types significantly.

I've probably veered off-topic a bit here, but I think its tangentially related and worth discussing in the same proposal.

[kthompson23]

I really like the changes that @shawnmcknight outlined and also agree that if you pass params as null then ctx.params should also be null. What do you think about removing the code to default params to empty object in ServiceBroker.call? Is that too much of a breaking change?

[icebob]

It was the original idea in 0.14, that I remove the empty object initialization in calls, as well, but many services (official services as well, like moleculer-db) relies on ctx.params isn't null. So I've put it back.

In brief, I can't remove the empty object setting in broker.call & ctx.call, but I should enable null value in event context. Moreover, Context is same for both function :)
So I don't know what is the correct Typescript definition for params.

[shawnmcknight]

It was the original idea in 0.14, that I remove the empty object initialization in calls, as well, but many services (official services as well, like moleculer-db) relies on ctx.params isn't null. So I've put it back.

Is there a difference if null is passed as the argument in call versus not providing it? That is, can an explicit null be allowed to pass through, but if ctx.params was undefined then set it to {}?

[icebob]

Interesting idea, I will check it.
It is the main code:
https://github.com/moleculerjs/moleculer/blob/62acd3441b9dba6b9ecafabaac3c1638d41a18cb/src/service-broker.js#L1021-L1023

If I change it to if (params === undefined), it will do something similar.

[shawnmcknight]

So I don't know what is the correct Typescript definition for params.

I do think that because Context allows the consumer to specify the expected type of params through the Generic that its unnecessary to have the union with null. If null is an expected possibility then the consumer can specify it. As it currently stands, even if a consumer specifies a generic, it still gets unioned with null so the consumer has to then do a null check every time.

I do think my other recommendations for handling of the Generics is worthwhile, but it would definitely be a breaking change for anyone not currently providing their own Generics because they would be forced to update. However, as mentioned above, I think having the defaults introduces their own problems -- namely the loss of type safety which defeats the purpose of using TS.

If there's a concern regarding the breaking change nature of my proposal, we could possibly split the difference. The default value for the params generic can be changed from P = GenericObject to P = GenericObject | null and then eliminate the type union on the params property and make it just P. This way, anyone that doesn't provide a generic would have the default typed more accurately than it currently is and the breaking change for consumers that are providing a generic would go away. This would still introduce a breaking change for consumers not providing a generic (they would have to do non-null checks). Since params can possibly be null at this point I don't think there is a way to avoid a breaking change for _someone_ and still have things typed somewhat accurately. Since there would have to be a breaking change, I think it would be better to encourage proper discipline and type safety.

To summarize, here are my two proposals:

Enforce type safety

class Context<P = {}, M extends object = {}> {
...
  params: P;
  meta: M;
...
  call<T>(actionName: string): PromiseLike<T>;
  call<T, P>(actionName: string, params?: P, opts?: GenericObject): PromiseLike<T>;

  emit<D>(eventName: string, data: D, opts: GenericObject): void;
  emit<D>(eventName: string, data: D, groups: Array<string>): void;
  emit<D>(eventName: string, data: D, groups: string): void;
  emit<D>(eventName: string, data: D): void;
  emit(eventName: string): void;

  broadcast<D>(eventName: string, data: D, opts: GenericObject): void;
  broadcast<D>(eventName: string, data: D, groups: Array<string>): void;
  broadcast<D>(eventName: string, data: D, groups: string): void;
  broadcast<D>(eventName: string, data: D): void;
  broadcast(eventName: string): void;
...
}

Type default generics more accurately

class Context<P extends object | null = GenericObject | null, M extends object = GenericObject> {
...
params: P;
...
call<T = any, P extends object = GenericObject>(actionName: string, params?: P, opts?: GenericObject): PromiseLike<T>;
...
}

If I change it to if (params === undefined), it will do something similar.

If call can be adjusted to allow null to pass through, then I'd change the constaint in the above from P extends object to P extends object | null.

As mentioned above, I think we're going to have a breaking change no matter what because the change to params is a breaking change from a TS perspective. I'd rather take the opportunity to enforce type discipline if there's going to be a breaking change either way, but it would be great to hear other's thoughts.

I will submit a PR on this if we can come to an agreement.

[icebob]

Thanks. The version 0.14 means it contains breaking changes, so this is the perfect time when we can add breaking changes in Typescript definitions, as well.

Would be good to hear other opinions from other Typescript developers.

Ping @rmccallum81 @faeron @fugufish @dani8art @ryanm-pm