Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong typings for hooks on route options #4376

Closed
brettwillis opened this issue Oct 26, 2022 · 6 comments · Fixed by #4655
Closed

Wrong typings for hooks on route options #4376

brettwillis opened this issue Oct 26, 2022 · 6 comments · Fixed by #4655
Labels
bug Confirmed bug good first issue Good for newcomers typescript TypeScript related

Comments

@brettwillis
Copy link
Contributor

The in the snippet below, the hook types are a union of two identical types like onRequestHookHandler<...> | onRequestHookHandler<...> etc, and this excludes the async hook type, which should be valid to use here, right?

Are these supposed to be onRequestHookHandler<...> | onRequestAsyncHookHandler<...>, etc?

fastify/types/route.d.ts

Lines 48 to 56 in 5c75f49

onRequest?: onRequestHookHandler<RawServer, RawRequest, RawReply, RouteGeneric, ContextConfig, SchemaCompiler, TypeProvider, Logger> | onRequestHookHandler<RawServer, RawRequest, RawReply, RouteGeneric, ContextConfig, SchemaCompiler, TypeProvider, Logger>[];
preParsing?: preParsingHookHandler<RawServer, RawRequest, RawReply, RouteGeneric, ContextConfig, SchemaCompiler, TypeProvider, Logger> | preParsingHookHandler<RawServer, RawRequest, RawReply, RouteGeneric, ContextConfig, SchemaCompiler, TypeProvider, Logger>[];
preValidation?: preValidationHookHandler<RawServer, RawRequest, RawReply, RouteGeneric, ContextConfig, SchemaCompiler, TypeProvider, Logger> | preValidationHookHandler<RawServer, RawRequest, RawReply, RouteGeneric, ContextConfig, SchemaCompiler, TypeProvider, Logger>[];
preHandler?: preHandlerHookHandler<RawServer, RawRequest, RawReply, RouteGeneric, ContextConfig, SchemaCompiler, TypeProvider, Logger> | preHandlerHookHandler<RawServer, RawRequest, RawReply, RouteGeneric, ContextConfig, SchemaCompiler, TypeProvider, Logger>[];
preSerialization?: preSerializationHookHandler<unknown, RawServer, RawRequest, RawReply, RouteGeneric, ContextConfig, SchemaCompiler, TypeProvider, Logger> | preSerializationHookHandler<unknown, RawServer, RawRequest, RawReply, RouteGeneric, ContextConfig, SchemaCompiler, TypeProvider, Logger>[];
onSend?: onSendHookHandler<unknown, RawServer, RawRequest, RawReply, RouteGeneric, ContextConfig, SchemaCompiler, TypeProvider, Logger> | onSendHookHandler<unknown, RawServer, RawRequest, RawReply, RouteGeneric, ContextConfig, SchemaCompiler, TypeProvider, Logger>[];
onResponse?: onResponseHookHandler<RawServer, RawRequest, RawReply, RouteGeneric, ContextConfig, SchemaCompiler, TypeProvider, Logger> | onResponseHookHandler<RawServer, RawRequest, RawReply, RouteGeneric, ContextConfig, SchemaCompiler, TypeProvider, Logger>[];
onTimeout?: onTimeoutHookHandler<RawServer, RawRequest, RawReply, RouteGeneric, ContextConfig, SchemaCompiler, TypeProvider, Logger> | onTimeoutHookHandler<RawServer, RawRequest, RawReply, RouteGeneric, ContextConfig, SchemaCompiler, TypeProvider, Logger>[];
onError?: onErrorHookHandler<RawServer, RawRequest, RawReply, RouteGeneric, ContextConfig, FastifyError, SchemaCompiler, TypeProvider, Logger> | onErrorHookHandler<RawServer, RawRequest, RawReply, RouteGeneric, ContextConfig, FastifyError, SchemaCompiler, TypeProvider, Logger>[];

@mcollina
Copy link
Member

likely so!
Would you like to send a Pull Request to address this issue? Remember to add unit tests.

@mcollina mcollina added bug Confirmed bug good first issue Good for newcomers hacktoberfest typescript TypeScript related labels Oct 26, 2022
@aradwann
Copy link
Contributor

It's a union between a onRequestHookHandler<...> (single value ) and a onRequestHookHandler<..>[] (array of values )

@brettwillis
Copy link
Contributor Author

Ah yes you're right. @mcollina @aradwann do you still want a PR to include the async version in those types?

While we're at it, are there any thoughts about the non-standard camel-case naming of those types?

@mcollina
Copy link
Member

Ah yes you're right. @mcollina @aradwann do you still want a PR to include the async version in those types?

Given my understanding, yes, we should update this. Would you like to send a PR?

While we're at it, are there any thoughts about the non-standard camel-case naming of those types?

We probably didn't know there was a standard. Could you link?
I would not change any types name right now - maybe open a separate issue? We might consider it for the next major.

@aradwann
Copy link
Contributor

aradwann commented Oct 29, 2022

@mcollina i don't if google style guide is followed here, according to https://google.github.io/styleguide/tsguide.html#identifiers class / interface / type / enum / decorator / type parameters should use UpperCamelCase
I see that most types/interfaces are written in UpperCamelCase here so I think onSendHookHandler should be OnSendHookHandler for consistency

@ismailmmd
Copy link

Hey, I have created a PR for above issue. 🙏 please review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed bug good first issue Good for newcomers typescript TypeScript related
Projects
None yet
5 participants