kwhitley / itty-router

A little router.
MIT License
1.75k stars 78 forks source link

✨ Enhance typescript types and add docs. #90

Closed SupremeTechnopriest closed 2 years ago

SupremeTechnopriest commented 2 years ago

This PR enhances the typescript types and adds some typescript usage documentation to the README. See the discussion in issue #89.

kwhitley commented 2 years ago

This is awesome, thanks! Left a note regarding method-hinting... wasn't sure if you still wanted to do that!

SupremeTechnopriest commented 2 years ago

Where did you leave the note? On the issue? Refresh my memory on what you mean by method hinting... With this implementation you are being specific about what you want to expose as strongly typed. So I don't think it will matter as much.

kwhitley commented 2 years ago

Left it in a PR review... and I was thinking something like a blend of what you ended up with (freeform, which is fine of course), and an earlier suggestion... something like this:

export type Router<TRequest> = {
  handle: (request: TRequest, ...extra: any) => any
  routes?: RouteEntry<TRequest>[]
  all?: Route
  get?: Route
  head?: Route
  post?: Route
  put?: Route
  delete?: Route
  connect?: Route
  options?: Route
  trace?: Route
  patch?: Route,
  [any:string]: Route,
}

...or at least maybe the more-specific .all "channel". Not sure how much/if that would help with VS code hinting... thoughts?

Maybe just:

export type Router<TRequest> = {
  handle: (request: TRequest, ...extra: any) => any
  routes?: RouteEntry<TRequest>[],
  all?: Route,
  [any:string]: Route,
}
SupremeTechnopriest commented 2 years ago

We can definitely add all that's a good idea since its always there.

But for the rest of the methods, you can set what you want strongly typed as a generic. For example:

interface IMethods {
  get: Route
  head: Route
  post: Route
  put: Route
  delete: Route
  connect: Route
  options: Route
  trace: Route
  patch: Route
}

const router = Router<Request, IMethods>()

Then everything you set in your interface will be strongly typed on the router. Since you are being explicit, we probably don't need to include the HTTP methods as optional statically. Let me add the all method statically... one sec.

SupremeTechnopriest commented 2 years ago

Oh I Have an idea.... Ill add an interface IHTTPMethods that you can extend if you want to.

import { IHTTPMethods, Router } from 'itty-router'

interface IMethods extends IHTTPMethods {
  puppy: Route
}

const router = Router<Request, IMethods>()

Then you will get all the http methods without having to define them, plus your custom puppy method.

SupremeTechnopriest commented 2 years ago

There we go. Updated the docs as well.

SupremeTechnopriest commented 2 years ago

Last one. Had some duplicated docs... Oops.

SupremeTechnopriest commented 2 years ago

Now for my use case:

import { IHTTPMethods, Router } from 'itty-router'

const router = Router<Request, IHTTPMethods>()
router.get('/', (request: Request) => {}) // Autocompletes

I'm sure this is a very common use case.

SupremeTechnopriest commented 2 years ago

Few grammar updates to the docs. Feel free to edit the copy to be in your own voice.

kwhitley commented 2 years ago

Thanks for all this btw!

Just to confirm before merging and docs polish, this is 100% backwards compatible, no breaking changes to anyone using the previous interfaces/types?

If we're a go on this, I may just smoke-test in the wild - so be prepared to field any backlash (so we aren't forced to rollback), haha :)

P.S. - I'll add credit into the docs once I merge and see what I'm looking at re the README!

SupremeTechnopriest commented 2 years ago

Hey yes! There are no breaking changes here. If anything it will relax the requirements for generic types. Now you aren't forced to add the Request type to the Router generics. Purely optional and defaults to the global Request type.

SupremeTechnopriest commented 2 years ago

Also no problem, if there are any typescript related issues just @ me on them and ill hop in.

kwhitley commented 2 years ago

Will do - side note, as I'm migrating from Travis CI (they abandoned open source authors) to GitHub actions... mind pushing a small change (any) to this branch to trigger the test action?

SupremeTechnopriest commented 2 years ago

Sure. Coming right up.

SupremeTechnopriest commented 2 years ago

Pushed. I really like github actions. You won't be disappointed.

SupremeTechnopriest commented 2 years ago

Let me know if you need another push. I have a some tweaks I can make.

SupremeTechnopriest commented 2 years ago

Just as I said that :)

It was just some nit picky things like prefixing all interfaces with I.

kwhitley commented 2 years ago

Wouldn't renaming the existing interfaces break people currently importing them (by the current name)?

kwhitley commented 2 years ago

...and I just released 2.5.0 with these changes before I saw this, lol. If we need to make any rapid changes, let's do it as a patch super fast before the world starts jumping on this.

The only one you added directly is IHTTPMethods? If so, that would be the only one safe for renaming without a version break...

SupremeTechnopriest commented 2 years ago

No worries it's okay how it is. You are right people might be using the interfaces. Just let me know when you do a major version bump and we can change it then. It's just a convention to prefix interfaces with I.

kwhitley commented 2 years ago

Good to know!!

kwhitley commented 2 years ago

Thanks guys, just catching up... if I recall, internally it awaits every handler (which allows it to work on sync/async universally), but by doing so, converts the response to a Promise naturally... confirming now.

EDIT: Confirmed. This should go in the docs and the types for sure, as we don't want anyone fooled into thinking they can return the handle directly without an await/then.

SupremeTechnopriest commented 2 years ago

I will open up another PR tomorrow. Thanks @poacher2k for pointing this oversight out!

SupremeTechnopriest commented 2 years ago

Looks like @kwhitley took care of it! 🙌

kwhitley commented 2 years ago

Released in 2.5.3 :)