kwhitley / itty-router

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

Typescript Examples #89

Closed SupremeTechnopriest closed 2 years ago

SupremeTechnopriest commented 2 years ago

Are there any typescript examples laying around? I looked at the .d.ts and its not very informative. The Router type seems to be incomplete. It doesn't expose the methods as functions (GET, POST, DELETE, ect). Could you please explain how the generic TRequest is supposed to be used?

Thanks!

SupremeTechnopriest commented 2 years ago

Ah it looks like the route types are handled by this any clause.

{
  [any:string]: Route
}

Not really ideal. Maybe we can change the type to:

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

This way all the methods can be autocompleted. Let me know what you think. Happy to open a pull request. Just let me know!

SupremeTechnopriest commented 2 years ago

Looking again, we should probably set the handle function's request argument to the generic Request that's passed in? Maybe something like this?

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

Hey @SupremeTechnopriest!

Looks like you already caught what's going on under the hood... definitely happy for you to PR and help out on the TS side, but with the following caveats:

  1. As you've seen, itty doesn't actually have any "real" methods, other than the handle function... they are "executed" by simply accessing the property on the Router proxy itself. If there's a TS way to hint that to users (so they don't feel constrained to the methods defined in the type), that would be fantastic... in the meantime, I'm sure it would certainly help to hint out at least all obvious paths, as you've shown.
  2. The "Request" is definitely not a requirement on a web standards Request object, but rather just any object that contains a "url" and "method" property. This was intentional to allow itty to be used in non-standard applications - just want to make sure this rather loose requirement is preserved.
  3. For consistency, I would suggest you modify the RouteHandler type (https://github.com/kwhitley/itty-router/blob/v2.x/src/itty-router.d.ts#L6) to read "extra" (rather than "args"), as you've shown in your example - for the sake of consistency. It might make it a bit more obvious to users that handlers will receive the same "extra" args passed to the handle.
  4. Since I don't usually live in TS land well enough to know, do you know of an easy way for me to verify/test these from the unit test (or perhaps simply a typescript compile pass during build/test) side of things? We have the actual execution/behavior side itty super well covered in tests, but the types side is a complete blind spot currently... could def use some help/advice on that front!
SupremeTechnopriest commented 2 years ago

Thanks for the guidance. I will start working on a PR.

  1. There's a few ways we can do this. We can denote the method functions as optional with a ?
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
}

unknown

And we can add some comments.

unknown-1

The comment text is just an example. If you have an ideas of what you would want it to say lmk and ill work it in.

  1. Yeah this makes sense now. I can pass in the cloudflare Request type and get all the cloudflare specific properties in my type checkin request.cf.

  2. Can do.

  3. I used to do the same thing as you. Write my projects in js and then maintain a .d.ts by hand. I hated how slow typescript is to compile. It really made it unusable for me. Since esbuild has come out my compile times are now under 100ms. Im actually using estrella which is built on top of esbuild and adds a lot of nice features. My project react-idle-timer was written in js (see the v4 branch). I just recently finished rewriting it in typescript (master branch) using estrella for building. It's very nice having the typedefs automatically generated. Im now in the process of rewriting all my projects in typescript. Something you might want to consider. Here is my build script. That said, the only real way to test the types is to write some tests in typescript and let the type checker fail the test. I don't know of any way to test a .d.ts standalone.

I have actually found Proxies to be extremely slow performance wise. I stopped using them all together in my code bases. I did a simple benchmark where the methods are defined statically and the result is pretty eye opening. You can see the results here.

Here is the code with the methods defined statically.

const createHandler = (method, base, routes) => (route, ...handlers) => {
  routes.push([
    method,
    RegExp(`^${(base + route)
      .replace(/(\/?)\*/g, '($1.*)?')
      .replace(/\/$/, '')
      .replace(/:(\w+)(\?)?(\.)?/g, '$2(?<$1>[^/]+)$2$3')
      .replace(/\.(?=[\w(])/, '\\.')
      .replace(/\)\.\?\(([^[]+)\[\^/g, '?)\\.?($1(?<=\\.)[^\\.') // RIP all the bytes lost :'(
      }/*$`),
    handlers
  ])
}

export const Router = ({ base = '', routes = [] } = {}) => ({
  routes,
  async handle (request, ...args) {
    let response, match
    const url = new URL(request.url)
    request.query = Object.fromEntries(url.searchParams)
    for (const [method, route, handlers] of routes) {
      if ((method === request.method || method === 'ALL') && (match = url.pathname.match(route))) {
        request.params = match.groups
        for (const handler of handlers) {
          if ((response = await handler(request.proxy || request, ...args)) !== undefined) return response
        }
      }
    }
  },
  all: createHandler('ALL', base, routes),
  get: createHandler('GET', base, routes),
  head: createHandler('HEAD', base, routes),
  post: createHandler('POST', base, routes),
  put: createHandler('PUT', base, routes),
  delete: createHandler('DELETE', base, routes),
  connect: createHandler('CONNECT', base, routes),
  options: createHandler('OPTIONS', base, routes),
  trace: createHandler('TRACE', base, routes),
  patch: createHandler('PATCH', base, routes)
})

I know it makes the source slightly larger, but I think the performance benefit is worth it. Its about 30%-40% faster depending on the system.

What do you think about me rewriting the entire library in typescript so we don't have to manage the typedefs by hand. It will have fast build times and typescript tests. I will define all the available methods statically so all the typedefs will be there by default. Plus we will gain some significant performance 😬.

Let me know what you think and I will get started!

SupremeTechnopriest commented 2 years ago

Just noticed you support the .all method as well. Just adding a note so its not overlooked.

kwhitley commented 2 years ago

So regarding the performance, that basically matters [far more] in a bottlenecked system like a Node server... 40% gains sounds like a lot if the one process is doing thousands/millions of operations (itty isn't in a serverless environment), for example if you're performance-tuning an array-mapping function, data transformer, or DOM manipulation library. In the case of a worker function, that's typically a single request... so 0.001ms vs 0.002ms is far less of a concern. Even in a client-side service worker, those requests are asynchronous, usually a few (to a hundred), and are unlikely to collide with each other in any meaningful way.

In the case of itty, the choice to not have a rigid structure was an intentional one (it actually originated much like you've outlined above, then shifted to free-form before the first release). A change to a rigid structure like that would prevent the fundamental free-form/future-proof nature of the module. With a Proxy and a few hundred bytes, we have a microrouter that can be used nearly for any method imaginable, with no update to the library required, and this also allows it to be used in non-traditional application. To back that out would add bytes (it's called itty for a very code-golfed reason) and introduce a breaking change to the community, for a performance gain that no one would ever be able to measure/feel in a real-world application.

kwhitley commented 2 years ago

And regarding defining the library itself in TS, totally fine with that (for example, itty-router-extras should absolutely just be written in TS since those function signatures are much more defined than itty), assuming it outputs to no more bytes than the current JS version, and breaks nothing in the process. The whole fun challenge with itty, and the reason it's called itty, is that code-golfed nature... smallest router with tons of features, etc, etc. We're not trying to make the world's best router, most-feature-rich, one that handles 100% of use-cases, or is the most type-constrained library out there (actually I prefer the opposite to allow downstream flexibility) ;)

kwhitley commented 2 years ago

Def appreciate the enthusiasm though! Just want to keep changes to the library in line with the vision :)

SupremeTechnopriest commented 2 years ago

Yeah that's why I checked with you first before diving into a PR. Makes sense to me. Since you aren't really trying to adhere to the HTTP specification the suggested changes to the types wouldn't really make sense. You want to leave it more generic so you are able to create their own methods:


const router = Router()

router.otter((request) => {
  return new Response(`${request.action} the otter`)
})

const request = { method: 'OTTER', url: 'https://iloveotters.com', action: 'pet' }
router.handle(request)

For my needs I will only ever be using the HTTP spec methods. At my day job I am the CTO of a performance company and every little bit counts. I think I will just maintain a fork with my suggested changes. You can close this issue if you have no objections!

Thanks for the discussion!

SupremeTechnopriest commented 2 years ago

Actually on second thought. We could add another generic to expose typed methods. That would give you stronger types and maintain the library's vision.

export type Router<TRequest, TMethods = {}> = {
  handle: (request: Request, ...extra: any) => any
  routes: RouteEntry<TRequest>[]
} & TMethods & {
  [any:string]: Route
}

export function Router<TRequest, TMethods>(options?:RouterOptions<TRequest>): Router<TRequest, TMethods>

and some sample code:

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

interface IMethods {
  get: Route
  post: Route
  /**
   * A special method for getting otters.
   */
  otter: Route
}

const router = Router<Request, IMethods>()

router.get('/', (request: Request) => {})
router.post('/', (request: Request) => {})
router.otter('/', (request: Request) => {
  // Return a raft of otters
})

addEventListener('fetch', (event: FetchEvent) => {
  event.respondWith(router.handle(event.request))
})

image

I think that is the best of both worlds. What do you think? Shall I whip up a PR?

SupremeTechnopriest commented 2 years ago

The second generic is totally optional by the way. It will work fine with:

const router = Router<Request>()

and still exposes the string key any.

SupremeTechnopriest commented 2 years ago

I went ahead and created a PR. I think this is a good addition in line with your vision. It also makes the first generic optional for less friction. Added some docs as well!

kwhitley commented 2 years ago

Addressed with your #90 - thanks again!! 🙏