Add type for srv.on('error') and fix srv.send overload #9
Conversation
| @@ -171,7 +171,7 @@ export class Service extends QueryAPI { | |||
| <T = any>(details: { event: types.event; data?: object; headers?: object }): Promise<T> | |||
| <T = any>(details: { query: ConstructedQuery; data?: object; headers?: object }): Promise<T> | |||
| <T = any>(details: { method: types.eventName; path: string; data?: object; headers?: object }): Promise<T> | |||
| <T = any>(details: { event: types.eventName; entity: LinkedDefinition | string; data?: object; params?: object }): Promise<T> | |||
| <T = any>(details: { event: types.eventName; entity: LinkedDefinition | string; data?: object; params?: object; headers?: object }): Promise<T> | |||
There was a problem hiding this comment.
headers appears to be a recurring parameter type throughout the services definition. If I gather this from CAPire correctly, we are referring to general HTTP headers here, is that correct? If so, we could probably use import type { IncomingHttpHeaders } from 'node:http' type across all signatures. That would give us a nice, strict typing, consistency (currently we have a mix of object and {}), and code completion:
There was a problem hiding this comment.
For most use cases, I think that would be an options. However, the messages of headers are of a different format.
Here, we'd need to have additional type, I suppose.
There was a problem hiding this comment.
I see. I think for this PR where you are adding a missing signature it is absolutely fine.
For the long run I am strongly in favour of adding a dedicated type for these headers. Maybe we can use the existing type from node:http to derive ours. But let's postpone this as well.
| @@ -290,6 +291,10 @@ interface OnEventHandler { | |||
| (req: Request, next: Function): Promise<any> | any | void | |||
| } | |||
|
|
|||
| interface OnErrorHandler { | |||
| (err: Error, req: Request): any | void | |||
There was a problem hiding this comment.
Perfectly in line with all our other handler signatures, so this is absolutely fine. I am just wondering: can we stricten the return type for handlers in any way in general? Can it maybe be derived from the parameters?
There was a problem hiding this comment.
The return type does not matter for before/after/on('error').
Promises are awaited.
For regular on-handlers the return type might be undefined or an instance or array of "Books" or a promise resolving to one of them.
I don't know if that's somehow expressable.
There was a problem hiding this comment.
Yes, we should be able to express this using TypedRequest<T>. It would then look more or less like this:
interface FooHandler<T> {
(..., req: TypedRequest<T>): T | Promise<T>
}which would basically mean "T in, T out". But lets postpone this into a separate issue, as it seems to be beyond the scope of your actual fix.
No description provided.