Rethinking how we do routing

[flakey5]

As discussed in #119, I think it's worthwhile to reconsider how we route requests.

How we currently do it

Requests to the worker invoke the fetch() method in src/worker.ts. A Handler is defined for each HTTP method that the worker accepts (with the exception of HEAD, which shares a handler with GET). The Handler is in charge of routing the request and caching (if applicable).

For the POST handler it's fine imo, but the GET handler is a bit messy.

Proposal A - Provider Refactoring

As part of adding the Provider concept, I was going to refactor some bits of the routing code to make it generally cleaner. I have a untested and still very rough implementation of it for HEAD requests here: https://github.com/nodejs/release-cloudflare-worker/blob/flakey5/20240414/file-actions/src/handlers/head.ts

The general plan for it was to split up the GET method Handler so that it was only handling GETs, and to have a Handler specifically for HEAD requests. Additionally, any code specific to files or directories would be removed and placed into actions/file.ts or actions/directory.ts (see here).

This approach doesn't really change much about how we route requests in the worker - it's mostly cleanup.

Proposal B - Middleware Approach

This was proposed by @ovflowd in #119.

We can create different Middlewares that would look something like

class R2Middleware extends Middleware {
  // override
  handle(request: Request, next: (request: Request) => Promise<Response>): Promise<Response> {
    try {
      // handle request using R2
    } catch (err) {
      // some error happened - fallback
      return next(request);
    }
  }
}

Then, we can define a route like,

get('/dist/**', [r2Middlware, originMiddleware])

This is by far a lot more modular system than what we have and gives us a lot more control over how the worker is serving requests.

I was able to get something written up that should work - will update with a link when I push it. Under the hood, it uses itty-router. I was going to try out find-my-way but I don't think it's possible with its current api.

Proposal C - Just using itty-router

Something like

get('/dist/**', async req => {
  // handle requests similarly to how we would in proposal a
  // the main benefit to this proposal is we should be able to get rid of the mapUrlPathToBucketPath method
  const path = req.path.substring('/dist/'.length);
})

For both Proposal B and C we get the added benefit of being able to easily and nicely add new routes if we need to.

[ovflowd]

I'm biased towards Plan B, but also OK with plan C and A. (I'd prefer C over A)

[ovflowd]

Also, thanks for taking the time to write this up! Appreciate it!

[flakey5]

I'm biased towards Plan B, but also OK with plan C and A. (I'd prefer C over A)

I think that's about the same in regards to what I'd prefer we do as well

[ovflowd]

Awesome! Let's do it. @MoLow do you have a different opinion here?

[MoLow]

using an existing solution makes sense to me, but if a reimplementation will make code look better then I am ok with that as well

[flakey5]

Fwiw, proposal b would really just be a wrapper of itty-router.