kwhitley / itty-router

A little router.
MIT License
1.69k stars 77 forks source link

Requests in v5 are hanging, examples not working #246

Closed Livshitz closed 1 month ago

Livshitz commented 1 month ago

Describe the Issue

Using the bare example of Express and running it, the request will hang.

image

This is also happening for all other examples. Downgrading to 5.0.4 didn't help as well, only downgrading to 4.x resolves this issue.

Example Router Code

Excerpt from the examples:

import express from 'express'
import { Router, error, json } from 'itty-router'
import 'isomorphic-fetch'

const app = express()

const router = Router()

router.get('/', () => 'Success!').all('*', () => error(404))

const handle = (request) => router.handle(request).then(json).catch(error)

app.use(handle)

app.listen(3001)
console.log('listening at https://localhost:3001')

Request Details

Environment (please complete the following information):

kwhitley commented 1 month ago

The good news is, I think your issue is simply a router.fetch (instead of router.handle) naming issue. We supported both in v4, but finally deprecated router.handle in v5, so when your code uses that, it translates to registering a route on the "HANDLE" http verb, instead of processing a request.

We made this migration to make itty naturally mate up to the expected export signature of environments like Workers or Bun, that look for a "fetch" method on the export, even though "handle" is arguably a better name (since it "handles" requests).

v5 Migration Notes: https://itty.dev/itty-router/migrations/v4-v5

:)

const handle = (request) => router.fetch(request).then(json).catch(error) // <-- use fetch, not handle

We need to make that distinction bolder/more in-your-face I think... or try to chase down some of the old articles and get them to update...

Livshitz commented 1 month ago

I tried changing to 'fetch' and didn't change anything. Were you able to reproduce? Did this fix it to you?

kwhitley commented 1 month ago

oooohhh, sorry I also see that you're trying to use this like middleware in express... which def won't work.

Express/Node has an entirely different signature pattern (res, req, next) than itty (req, ...args), so more translation is needed. This is why we have to have the extra steps in this guide:

https://itty.dev/itty-router/guides/node

You might try using createServerAdapter in @whatwg-node/server (from the Node guide above) and see if using that directly in the use (middleware) path like you've used works.

If it does, please let me know and we can document that for others!

Out of curiosity though, why are you using both express and itty together? I would assume if you're already using express (which has its own router), there'd be no real gain from including itty, unless you just wanted deeper route code to look cleaner than with express?

Livshitz commented 1 month ago

@kwhitley tested with createServerAdapter and worked properly. I guess that the changes from handle to fetch made some mismatch as this was working in v4. here's a PR with the change to the example: https://github.com/kwhitley/itty-router/pull/247

why are you using both express and itty together?

I'm using itty as a runtime/framework-agnostic routing, so I could interchange between cloudflare for prod, express for local dev (for debugging) or Bun and other environments but keep the same routing as plug-and-play.

kwhitley commented 1 month ago

Ahhh I see! A couple points then:

If your end goal is mostly just CF Workers, I'd recommend just using wrangler dev for local development... with the caveat that there's currently a bug requiring an odd workaround (https://itty.dev/itty-router/guides/cloudflare-workers). They have a PR staged to fix that, but it's taking awhile.

As a runner up to wrangler dev, I'd just do Bun, as it requires zero translation/adapter layers from CF (given that it too is based around web standards). Itty passes through unknown config to the final object, which also happens to work great with Bun! For instance, here's all it takes to set the port of the Bun server:

import { AutoRouter } from 'itty-router'

const router = AutoRouter({ port: 3001 }) // port becomes part of the final object signature

router.get('/', () => 'Hey there!')

export default router
kwhitley commented 1 month ago

That all said, thanks for figuring out what it takes for express integration (and thanks for the PR)!! :)

Livshitz commented 1 month ago

Sure thing, that's the least I could do. Thank you for this useful library.

I remembered wrangler dev didn't supporting proper debugging in vscode, so I had to abstract the routes and load them via express for local dev. But now found this one to make it work properly: https://github.com/cloudflare/workers-sdk/discussions/4174 Yet, the ability to abstract routes from runtime, even run it on Bun or elsewhere, is a good practice IMO.

Just for the record, here's a simple Bun-Itty-router snippet:

import { AutoRouter } from 'itty-router';
const router = AutoRouter({ port: 3001 });

router.get('/', () => 'success' );

Bun.serve(router);

// bun add @types/bun
// bun src/index.ts