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

Automatic Typescript route param parsing #150

Open
wants to merge 5 commits into
base: v4.x
Choose a base branch
from

Conversation

hmnd
Copy link

@hmnd hmnd commented Mar 10, 2023

This PR enables automatic type inference from provided route paths, ensuring that users can only access params defined in the route.

I've added type tests to cover these changes. I think they cover all possible eventualities, but would appreciate a second opinion.

Given that this is proposed for a major version upgrade, I did include one semi-breaking change. Instead of having to extend the Router in order to add custom methods, I've added a TMethods type parameter to Router. This allows the user to provide a union of any number of method types they would like (eg. 'puppy' | 'kitty'); all of which are mapped to the Route function type internally. This has the added benefit of ensuring the Route type remains consistent across all routes.

P.S. thanks to this gist and @supabase/postgrest-js for some of the inspiration on how to achieve this :)

@hmnd hmnd marked this pull request as ready for review March 10, 2023 10:24
@kwhitley
Copy link
Owner

kwhitley commented Mar 10, 2023

Awesome, thanks David!

Re. your proposed breaking change, mind including (here) a brief before & after snippet of what implementation would look like compared to what it is?

@kwhitley
Copy link
Owner

Briefly looking at the example.ts though, I think I def prefer your direction!

@hmnd
Copy link
Author

hmnd commented Mar 10, 2023

Re. your proposed breaking change, mind including (here) a brief before & after snippet of what implementation would look like compared to what it is?

I think the change to the example shows the difference pretty well.

The previous method is brittle in a couple of ways.

  • It requires the user to provide the Route type on their own. I think this should only be possible if they implement their own wrapper around Router.
  • It requires the user to provide their custom router type, every time they want to use their custom methods. Although custom methods should only be used sparingly, it's still overly verbose and doesn't feel right.

An alternative (possibly nicer?) option is to replace this with a simple Record<string, Route>, allowing users to use custom methods, while retaining niceties like completion for the methods in RouterHints.

Re my current implementation, say you want to support the puppy and kitty methods:

Before

interface CustomRouter extends RouterType {
  puppy: Route;
  kitty: Route;
}

const router = Router({ base: '/' })

router
  .all<CustomRouter>('*', () => {...})
  .puppy('*', () => {...})
  .kitty('*', () => {...});

After

const router = Router<'/', 'puppy' | 'kitty'>({ base: '/' })

router.puppy('*', () => {...})

router.kitty('*', () => {...});

@hmnd
Copy link
Author

hmnd commented Mar 10, 2023

If you agree, I think it would also be nice to allow providing your own extensions to the IRequest type, at the Router and Route level. itty v2 supported this, but the ts rewrite dropped it, which makes it more cumbersome to use custom properties added on to the request in previous handlers.

When provided, the extended types would replace the GenericTraps that we currently have.

Edit: working on workers project now and the current v3.x paradigm of providing an augmented IRequest within each route handler is really annoying to me.

The particular issue I have is is the properties you wish to add on to IRequest must be optional. That's because request in the RouteHandler type is explicitly typed as IRequest. So if the extended properties aren't marked optional, TS complains that the type doesn't sufficiently overlap with IRequest.

In my current use case, I'm assigning an API client to request in a .all('*') handler before any other routes, so wherever I use the client, I have to tell TS that it definitely exists with request.myClient!.

@kwhitley
Copy link
Owner

kwhitley commented Mar 20, 2023

The particular issue I have is is the properties you wish to add on to IRequest must be optional. That's because request in the RouteHandler type is explicitly typed as IRequest. So if the extended properties aren't marked optional, TS complains that the type doesn't sufficiently overlap with IRequest.

In my current use case, I'm assigning an API client to request in a .all('*') handler before any other routes, so wherever I use the client, I have to tell TS that it definitely exists with request.myClient!.

Have a suggestion for solving this one in an elegant way? I totally agree this is a problem, and one I too face all the time... usually forcing me to create a custom downstream request type and case the request as that in the downstream handlers (cumbersome/boilerplatey)

Basically, how do we allow an upstream middleware modify the downstream expected request type effectively?

@kwhitley
Copy link
Owner

Additionally, I love this:
image

Know of an elegant way to allow this to work with withParams? This is a middleware that proxies the request, checking attribute requests off the main request against the params object... meaning name and id could be requested directly off the request, as well as from request.params.

@kwhitley
Copy link
Owner

Ok, revisiting this one after some time off (and time spent tackling the greater TS issues):

First of all, guaranteed this no longer works with v4.x as it's made some heavy under-the-hood TS changes in the last several days (pending release).

I'd love to see a PR (based on the current types in v4.x) that just tackles the route params, while playing nicely with the typing methods described here:
https://itty.dev/itty-router/typescript

If we can get this in without adding too much complexity, or modifying the existing type signatures (described in the docs), that would be amazing!

@kwhitley kwhitley added discussion Let's discuss TypeScript TypeScript-related issues labels May 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Let's discuss TypeScript TypeScript-related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants