kwhitley / itty-router

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

Support `withContent` without `content-type` header #186

Closed nathanclevenger closed 4 months ago

nathanclevenger commented 9 months ago

Describe the Issue

I keep running into hard-to-debug issues with APIs on itty-router ... and each of the last several times, the issue ended up being that the clients we're properly sending the content-type header set to application/json even though there was a body in json format. I've even found commercially available webhooks that don't properly set this header, and as a result I've had to replace the withContent middleware simply with a version that doesn't check the header

Example Router Code

I'm currently replacing this code:

if (request.headers.get('content-type')?.includes('json'))
  request.content = await request.json()

with:

if (request.body) {
  try {
    request.content = await request.json()
  } catch (err) { }
}

or

if (request.body) {
  request.content = await request.text()
  try {
    request.content = JSON.parse()
  } catch (err) { }
}

Expected Behavior

I would expect the content property to be set if there was a body that was valid json

Actual Behavior

This currently requires the client to properly set the content-type header, which right or wrong, many APIs don't actually enforce

Additional Context

If you're open to this, I would be happy to make a pull request @kwhitley

kwhitley commented 9 months ago

Hey Nathan, I’m gonna be spending the next few days I traveling back home from my honeymoon, but I’m down to explore options for sure. My one major concern is performance, which we'd need to do some tests on. The idea of running text+json parse passes automatically sounds taxing, but perhaps it's a lighter impact than I realize.

kwhitley commented 4 months ago

This cascade has been added in 4.1.1 :)