-
-
Notifications
You must be signed in to change notification settings - Fork 57
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
types: improve typing to allow custom request and reply #211
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Request extends FastifyRequest = FastifyRequest, | ||
Reply extends FastifyReply = FastifyReply |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, looks not correct. Why is Request and Reply not depending on the Generic Types from FastifyInstance?
Request extends FastifyRequest = FastifyRequest, | |
Reply extends FastifyReply = FastifyReply | |
Request extends FastifyRequest = Request extends FastifyRequest = FastifyRequest<RawServerDefault, RawRequest, FastifySchema, TypeProvider, ContextConfigDefault, Logger>, | |
Reply extends FastifyReply = FastifyReply<RawServer, RawRequest, RawReply, RouteGenericInterface, ContextConfigDefault, FastifySchema, TypeProvider> |
Untested but I would expect this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The FastifyRequest
and FastifyReply
interfaces have changed from your example.
The default should be equal to the old value to prevent breaking the previous code.
Furthermore, now the FastifyRequest
accepts other generic types, for instance, RouteGeneric, and in this way, the users can pass the request as they prefer.
If you take a look at the tests, I tried to break nothing and include your request to leave the power to the user to choose the request type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check my comment.
@Uzlopak any news on this one? otherwise Matteo cannot merge the PR. |
@fastify/typescript Can somebody else have a look at it. I dont want to be a blocker, but I dont think that the arguments are valid as the generic types are not passed down to the FastifyReply and FastifyRequest. |
I dont want to block, but I am not convinced either.
That is true, but most of the time people use only the default server then it would be fine. |
Somebody has to decide if this gets merged and published or not. |
This PR closes #207
Checklist
npm run test
andnpm run benchmark
and the Code of conduct