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

Requests in v5 are hanging, examples not working #246

Closed
Livshitz opened this issue May 14, 2024 · 7 comments
Closed

Requests in v5 are hanging, examples not working #246

Livshitz opened this issue May 14, 2024 · 7 comments

Comments

@Livshitz
Copy link
Contributor

Livshitz commented May 14, 2024

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):

  • Environment: Node
  • itty-router Version: 5.0.17
  • node: 18, 20
@kwhitley
Copy link
Owner

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
Copy link
Contributor Author

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

@kwhitley
Copy link
Owner

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 added a commit to Livshitz/itty-router that referenced this issue May 14, 2024
Should be using adapter to convert to express-compatible handler with `createServerAdapter`.
Based on this issue conversation: kwhitley#246 (comment)
Livshitz added a commit to Livshitz/itty-router that referenced this issue May 14, 2024
Should be using `createServerAdapter` adapter to convert to express-compatible handler.
Based on this issue conversation: kwhitley#246 (comment)
kwhitley pushed a commit that referenced this issue May 14, 2024
Should be using `createServerAdapter` adapter to convert to express-compatible handler.
Based on this issue conversation: #246 (comment)
@Livshitz
Copy link
Contributor Author

Livshitz commented May 14, 2024

@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: #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
Copy link
Owner

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
Copy link
Owner

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

@Livshitz
Copy link
Contributor Author

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: cloudflare/workers-sdk#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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants