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

Improve Type Customization #185

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Improve Type Customization #185

wants to merge 6 commits into from

Conversation

Tyrenn
Copy link

@Tyrenn Tyrenn commented Aug 9, 2023

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 🫶

@kartikk221
Copy link
Owner

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?

@Tyrenn
Copy link
Author

Tyrenn commented Aug 13, 2023

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.

@kartikk221
Copy link
Owner

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.

@Tyrenn
Copy link
Author

Tyrenn commented Aug 22, 2023

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 ?

@kartikk221
Copy link
Owner

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
Copy link
Contributor

@Tyrenn Looks like your PR is stale. Maybe after my PR ,#207, is merged. You can merge your changes.

@Tyrenn
Copy link
Author

Tyrenn commented Dec 19, 2023

@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 !

@Tyrenn
Copy link
Author

Tyrenn commented Dec 20, 2023

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

@kartikk221
Copy link
Owner

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!

@Tyrenn
Copy link
Author

Tyrenn commented Dec 21, 2023

I think I'm done, only the vocabulary "Reply" / "Response" question remains, do you have a preference ?

@Geo25rey
Copy link
Contributor

I think I'm done, only the vocabulary "Reply" / "Response" question remains, do you have a preference ?

Maybe "ResponseFormat", "ResponseBody", or "ResponseBodyFormat"

@Tyrenn
Copy link
Author

Tyrenn commented Mar 12, 2024

@kartikk221 Have you had time to test it on existing codebases ? 😄

Maybe "ResponseFormat", "ResponseBody", or "ResponseBodyFormat"

I think I'll stick with "Response" for now, I found other proposal a bit long

@ToledoSDL
Copy link

Hey! Any updates on this? I'd like to contribute, by the way...

@Tyrenn
Copy link
Author

Tyrenn commented Jul 15, 2024

The PR is ready, we just need to wait for @kartikk221 to test with existing codebases

@Tyrenn
Copy link
Author

Tyrenn commented Sep 17, 2024

Hello ! Any update on the tests ? Cheers

@kartikk221
Copy link
Owner

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?

@Tyrenn
Copy link
Author

Tyrenn commented Sep 18, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants