Add types to async fastify hooks

[RafaelGSS]

I did not add types for non-documented async hooks like: onRegister hook. I would like to adjust it in another PR.

This PR solve this kind of usage:

const preValidationExample: preValidationAsyncHookHandler = async (_request: FastifyRequest, _reply: FastifyReply) => {  };
preValidationExample({} as FastifyRequest, {} as FastifyReply); // don't need to pass something as third param

Checklist

• run npm run test and npm run benchmark
• tests and/or benchmarks are included
• documentation is changed or added
• commit message and code follows the Developer's Certification of Origin
  and the Code of conduct

[L2jLiga]

Can't we use overloads?

example:

/**
 * `onRequest` is the first hook to be executed in the request lifecycle. There was no previous hook, the next hook will be `preParsing`.
 *  Notice: in the `onRequest` hook, request.body will always be null, because the body parsing happens before the `preHandler` hook.
 */
export interface onRequestHookHandler<
  RawServer extends RawServerBase = RawServerDefault,
  RawRequest extends RawRequestDefaultExpression<RawServer> = RawRequestDefaultExpression<RawServer>,
  RawReply extends RawReplyDefaultExpression<RawServer> = RawReplyDefaultExpression<RawServer>,
  RouteGeneric extends RouteGenericInterface = RouteGenericInterface,
  ContextConfig = ContextConfigDefault
> {
  (
    request: FastifyRequest<RouteGeneric, RawServer, RawRequest>,
    reply: FastifyReply<RawServer, RawRequest, RawReply, RouteGeneric, ContextConfig>,
    done: HookHandlerDoneFunction
  ): void;

  (
    request: FastifyRequest<RouteGeneric, RawServer, RawRequest>,
    reply: FastifyReply<RawServer, RawRequest, RawReply, RouteGeneric, ContextConfig>,
  ): Promise<unknown>;
}

[zekth]

@L2jLiga unfortunately it doesn't seem to work :(

[fox1t]

@L2jLiga unfortunately it doesn't seem to work :(

What's the error?

[zekth]

@L2jLiga unfortunately it doesn't seem to work :(

What's the error?

Error was on the addHook method not taking the proper overload.

[L2jLiga]

Let's add tests first and then check what is better approach

[RafaelGSS]

I tried the approach of overload but seems not works. However, I'll update the PR with this approach to show.

[RafaelGSS]

@L2jLiga @fox1t Attempt added 1562086

[L2jLiga]

I just checked both approaches with TS playground, neither separated declaration nor inside one interface seems to work

update: seems like I found possible solution, @fox1t wdyt?

[zekth]

I just checked both approaches with TS playground, neither separated declaration nor inside one interface seems to work

update: seems like I found possible solution, @fox1t wdyt?

If you try this in the project the issue comes at the end of the different hooks. Like the preValidation hook has an error related to onClose which is totally out of scope. I'll reproduce and share the code.

Also in your example:

'req' is declared but its value is never read.
'res' is declared but its value is never read.
'req' is declared but its value is never read.
'res' is declared but its value is never read.
Argument of type '(req: Object, res: Object, done: DoneFn) => Promise<void>' is not assignable to parameter of type '(req: Object, res: Object) => Promise<void>'

Means like the type mapping is not working properly

[L2jLiga]

'req' is declared but its value is never read.
'res' is declared but its value is never read.
'req' is declared but its value is never read.
'res' is declared but its value is never read.

Yeah, it's just a warning that they're not used hence can be deleted

Argument of type '(req: Object, res: Object, done: DoneFn) => Promise<void>' is not assignable to parameter of type '(req: Object, res: Object) => Promise<void>'

Means like the type mapping is not working properly

This is expected due to documentation

Notice: the done callback is not available when using async/await or returning a Promise. If you do invoke a done callback in this situation unexpected behaviour may occur, e.g. duplicate invocation of handlers.

[L2jLiga]

I updated example in playground: link

update: link was updated

[zekth]

I updated example in playground: link

My bad you're right. This looks like a good alternative

[L2jLiga]

@RafaelGSS, @fox1t what is your opinion on it?

[fox1t]

@L2jLiga, this is a brilliant solution! Conditional types allow inferring the proper types instead of relying upon intersection/overloading.

As we (re)discovered here, we can't use overloading (type intersection in the types world) for fastify methods. We already encountered this same issue when we decided to deprecate the FastifyPlugin interface in favor of FastifyPluginCallback and FastifyPluginAsync.
@RafaelGSS, can you try to implement what @L2jLiga made in the last playground? I am curious if it works in our codebase. If so, we should start thinking about using it in other places.

[L2jLiga]

@fox1t I played a bit more with types and think you will like this: link

It's universal solution and you have to pass arguments as second generic to DonableFn<T, Args, DoneFn>

[fox1t]

@fox1t I played a bit more with types and think you will like this: link

It's universal solution and you have to pass arguments as second generic to DonableFn<T, Args, DoneFn>

I definitely like this. @Ethan-Arrowood what do you think?
We have to remember that this works only on TS >= 4.0.0, though.
Can you please exlpain line by line DonableFn type? I want to add this as reference somewhere. I am adding some comments by my own.

• Why T extends (...args: any[]) => void is passed insread of T extends (...args: any[]) => any?
• This should not be valid but it is valid impl2.addHook('onRequest', () => {}); No call to done() nor returning promise.

[Ethan-Arrowood]

Ooh very clever. Inferring the function type (callback v async) based on the arguments list. I like it!

[L2jLiga]

• Why T extends (...args: any[]) => void is passed insread of T extends (...args: any[]) => any?

this does not make much sense for example, so no answer 😅

• This should not be valid but it is valid impl2.addHook('onRequest', () => {}); No call to done() nor returning promise.

Unfortunately I have no idea how to force ts to check arguments length 😕

Can you please exlpain line by line DonableFn type? I want to add this as reference somewhere. I am adding some comments by my own.

I think would be better to describe it block by block, I will try provide description asap 🐱

[Ethan-Arrowood]

Unfortunately I have no idea how to force ts to check arguments length 😕

I don't think you can. I believe TS specifically doesn't do this for some other reasons. Honestly, I can't remember exactly but I do think I explored that rabbit hole in the past and came up empty.

[RafaelGSS]

Hey! I'll update the example tonight with the example provided by @L2jLiga! Seems very interesting!

[fox1t]

@L2jLiga what is addErrorHook suppose to be? Shouldn't it be https://www.fastify.io/docs/v3.2.x/Hooks/#onerror?

[L2jLiga]

I don't think you can. I believe TS specifically doesn't do this for some other reasons. Honestly, I can't remember exactly but I do think I explored that rabbit hole in the past and came up empty.

Then I suggest to don't worry about :)

Hey! I'll update the example tonight with the example provided by @L2jLiga! Seems very interesting!

Cool! Thanks dude!

@L2jLiga what is addErrorHook suppose to be? Shouldn't it be https://www.fastify.io/docs/v3.2.x/Hooks/#onerror?

Just an example how it can be used for hooks with different arguments length, does not related to fastify itself

[RafaelGSS]

@L2jLiga I have failed to update the onRequestHookHandler with these solutions. Can you update one type on this branch? Not sure about the solution

[fox1t]

@mcollina we are waiting for the description of the code provided by @L2jLiga. After that, we can try to integrate his solution in fastify types. If it works we finally have a way of expressing polymorphic methods and it would be an awesome achievement.
Again, this solution would be only compatibile with TS >= 4.0, though.

[RafaelGSS]

Fixed (Sorry for the late - busy week). If the implementation is accepted I'll change every hook.

[RafaelGSS]

@L2jLiga Unfortunately the implementation seems not to work on tsd checks. Check the tsd errors on callback-or-promise.d.ts and hooks.test-d.ts.

[L2jLiga]

@L2jLiga Unfortunately the implementation seems not to work on tsd checks. Check the tsd errors on callback-or-promise.d.ts and hooks.test-d.ts.

@RafaelGSS I mentioned this here and already thinking how we can fix it 🤔

[L2jLiga]

Here's fix:

tsdjs/tsd#83

[fox1t]

I think we should just wait for the TS 4.1 if we want to implement this. It is a breaking change for TS users.

[RafaelGSS]

@fox1t This does not change only on fastify side? IMHO we should land this fix without the overload/conditional type and after the tsd merge, we add support for it.

[fox1t]

Yes. Sorry for the confusion: I meant that we should wait to land the "overload/conditional type" part and go on with the current fix for the moment.

[L2jLiga]

Ok, works for me :)

[fox1t]

LGTM!

[zekth]

lgtm

[RafaelGSS]

@Ethan-Arrowood Have any concerns about? I will land tomorrow.

[Ethan-Arrowood]

lgtm :) great work everyone!

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.