-
Notifications
You must be signed in to change notification settings - Fork 102
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
Improve Type Customization #185
base: master
Are you sure you want to change the base?
Conversation
Hey, this looks very interesting. I will review it within some large codebases of mine to test for breaking changes. Do you know about any major / guaranteed breaking changes that will come along with these types? or do these types extend any / are open ended and will not break existing Typescript codebase types? |
No breaking changes, all types are based on standard Typescript mecanism/utility types. The only "complex" type I have written is the Pattern type but again, using standard Typescript. Furthermore, as the whole PR is based on generics, one can still uses hyper-express with typescript without typing the request parameters or body for more flexibility, it should be automatically inferred as any. |
Hey, apologize been a bit busy with some stuff. Will review this later this week and get merged. From a small project I tried your types in, no errors were thrown by TS. Will try in a few larger projects and then we should be good to go. |
No worries. As I stated in my first post, we could further enhanced type customization especially to allow Response type to be set. Shall I try to work on it and open a second PR after this one is merged ? |
Of course, feel free to do so. I think your current approach inspired by Fastify generics is probably the best course of action given a lot of Node.js developers prefer Fastify's type safety. |
@Geo25rey sorry I've been a bit overwhelmed in my life recently, I thought this PR had been merged already but we might misunderstood each other with @kartikk221 when I said I would also improve Response type, I thought afterwards. Anyway, I might find the time to complete this PR and fix conflicts in the next weeks ! |
I've finally found time to merge latest changes. Seems to not break anything. As I mentioned I would long time ago, I've added the possibility to type json Response. This types are still optional and only useful for Typescript usage. This additional type lies on the "Response" prop of the RouteOption Generic. Fastify uses the "Reply" keyword to avoid confusion with the actual Response object, I don't know if we should follow their footsteps ? I've also found out that most of response object method signatures return a Response object instead of this. Don't really know if its an issue nor voluntary |
No worries at all. I have been busy with work as well. Once you are done, I will be sure to test it out with some existing codebases to ensure types don't break. Thanks again! |
I think I'm done, only the vocabulary "Reply" / "Response" question remains, do you have a preference ? |
Maybe "ResponseFormat", "ResponseBody", or "ResponseBodyFormat" |
@kartikk221 Have you had time to test it on existing codebases ? 😄
I think I'll stick with "Response" for now, I found other proposal a bit long |
Hey! Any updates on this? I'd like to contribute, by the way... |
The PR is ready, we just need to wait for @kartikk221 to test with existing codebases |
Hello ! Any update on the tests ? Cheers |
Hey @Tyrenn apologize for the long delay. Had stepped away from open source for a bit due to being extremely busy with work / business. Will pull this PR to my internal repositories to see If I can catch any edge cases. Has the PR been working perfect for you in all of your projects? |
No worries ! It's part of the open source cycle 😄 I hadn't use it extensively nor for a while, but it was working at the time. Might wanna check edge cases though ! Cheers |
This PR is a proof of concept implementing some ideas from #131 to ease
hyper-express
usage with Typescript.All I did was changing the type declarations to allow a bit of customization to benefit from Typescript type checks and autocompletion. You'll find rough examples inside a new markdown page soberly called Typescript.
The customization is inspired by Fastify generics with a simple object having multiple properties : {Body, Params, Locals}. In my Fastify projects I frequently use this handy type customization, I find it really useful to avoid type errors.
If maintainers appreciate the idea, we may also push the customization further and allow Response type customization and so on. I would be happy to discuss further customization and choices about the Generic form.
I love this underrated package btw 🫶