kwhitley / itty-router

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

Middleware for responses #85

Closed markusahlstrand closed 2 years ago

markusahlstrand commented 2 years ago

Similar to how express works there doesn't seem to be an easy way for an itty middleware to trigger after the response. For express there's the https://www.npmjs.com/package/express-mung library to mitigate this, but it's not the most pretty solution. It would be nice if a middleware could be passed a next function that you could await that could return the response object, so something like this:

async function middleware(request, next) {
  const response = await next();

  console.log(response.status);
}

This would make it possible to do things like error handler middlewares, logging response and keeping track of response times.

markusahlstrand commented 2 years ago

After testing a few different approaches I have a slightly different proposal. Adding the next parameter would be a breaking change which might not be ideal even though it's in my opinion would be the cleanest solution.

Another solution would be to allow the middleware to return a function that is executed on the returned response. This way a middleware can modify the response before it's sent to the client:

async function middleware(request) {
  const startAt = new Date();
  return async (err, response) => {
    // Log the duration of the call
    console.log(`Duration: ${new Date() - startAt}`)

    // Not sure if the headers are immutable? 
    response.headers.foo = 'bar';
    return response;
  } 
}

If any errors are return you could use this pattern to write error handlers as well.

Not sure if this could achieved with the Proxy functionality? Guess it would need to be part of the actual router?

kwhitley commented 2 years ago

Hey @markusahlstrand - currently you can do one of the following:

  1. Modify the response after router.handle returns one:
    router
    .handle(request)
    .then(response => {
    // do something to the response, like inject CORS headers, etc
    return response
    })
    .catch(err => console.log(err))
  2. Build a response incrementally, using the request (or any additional arg) as the carrier agent between middleware/handlers.

The current (request, ...args) signature, while different than (request, response, next) of Express, is actually one of the reasons itty works out of the box with Cloudflare's module syntax, as additional params (specifically the env) can easily be passed along through the handler chain.

kwhitley commented 2 years ago

That said, itty is also incredibly flexible thanks to this freeform signature. If you wanted to emulate something more like Express, you could simply do something like this:

const router = Router()

router
  .get('*', (request, response) => {
    response.status = 204 // modify response, but exit without returning
  })
  .all('*', (request, response) => response) // return response downstream once done building

// and where you call the handle method elsewhere, just pass in a fresh response... 
// middleware/handlers all receive any params sent to the handle function.
router.handle(request, new Response())
markusahlstrand commented 2 years ago

Thanks for the reply!

The use case I have right now is that I would like to log the responses to a S3 firehose, which I think should work fine with doing a then/catch on the handler like you showed above. It would be nifty to be able to use middlewares like in koa-router, but for the time being it's not something I need.

jamesarosen commented 2 years ago

Here's a use-case that is easily modeled with next:

export default async function attachSessionToCachedResponse(request, next) {
  // get the edge-cached HTML and the session info simultaneously:
  const sessionRequest = new Request('https://api.example.com', { headers: request.headers })
  const [cachedResponse, sessionResponse] = Promsie.all([ next(), fetch(sessionRequest) ])

  // attach the session cookies to a mutable copy of the HTML response:
  const responseWithSession = new Response(cachedResponse.body, cachedResponse)
  responseWithSession.headers.set('Set-Cookie', sessionResponse.headers.get('Set-Cookie'))
  return responseWithSession
}

That's certainly possible with the current framework, but it's a little fragile:

router.get('*', (request, response) => {
  // get the session asynchronously:
  const sessionRequest = new Request('https://api.example.com', { headers: request.headers })
  const sessionResponsePromise = fetch(sessionRequest)

  // teach the response how to attach the session once it's resolved:
  response.attachSession = async () => {
    const sessionResponse = await sessionResponsePromise
    const responseWithSession = new Response(this.body, this)
    responseWithSession.headers.set('Set-Cookie', this.headers.get('Set-Cookie'))
    return responseWithSession
  }

  // don't return so the request flows upstream
}

router
  .handle(request, new Response())
  .then(response => return response.attachSession()) // attach the session
danbars commented 2 years ago

Here is what I do. Not ideal, but works. Having real middlewares would be awesome.

addEventListener('fetch', event => {
  const mainRouter = Router()
  mainRouter
    .all('/*', logger({
      logDnaKey: LOG_DNA_KEY,
      host: LOG_DNA_HOSTNAME,
      appName: LOG_DNA_APPLICATION_NAME}))
    .all('/*', requestTimer)
    .all('/v1/*', someOtherHandler)
    .all('*', missingHandler) 

  event.respondWith(
    mainRouter
      .handle(event.request, event)
      .then(async response => {
        await finalize(event, response)
        return response
      })
      .catch(async error => {
        return await errorHandler(error, event)
      })
  )
})

// in this method I do all the response middleware.
//not as nice as real middlewares because all concerns are in one place, but it does the work
//note that your error handler also has to call this method
async function finalize(event, response) {
  event.request.timer.end() //this one measures request handling time
  response.headers.set('X-Timer', event.request.timer.total())
  response.headers.set('X-RequestId', event.request.requestId)
  const origin = event.request.headers.get('origin') // handle CORS
  if (origin && ['https://allowed.origin.com', 'http://localhost:9292'].includes(origin)) {
    response.headers.set('Access-Control-Allow-Origin', origin)
    response.headers.set('Access-Control-Allow-Methods', 'POST, GET, OPTIONS, DELETE, PUT, PATCH')
    response.headers.set('Access-Control-Allow-Headers', 'Content-Type, Authorization')
    response.headers.set('Access-Control-Allow-Credentials', 'true')
  }
  event.waitUntil( //write to logger. I use logDNA
    flushLog(event.request)
  )
}
Moser-ss commented 2 years ago

Hey @kwhitley I saw in several comments about the topic of handling responses the following code snippet

router
  .handle(request)
  .then(response => {
    // do something to the response, like inject CORS headers, etc
    return response
  })
  .catch(err => console.log(err))

But I am struggling to adapt that to my implementation

router.all('/api/*', endpoints.handle)

because I am not sure how to chain the handle request in this case

danbars commented 2 years ago

@Moser-ss your line is just the configuration of the router, it doesn't actually handle the request. After that you have to do router.handle(request) To this one you can chain .then(response) just as in the snippet that you showed

Moser-ss commented 2 years ago

Thanks for the tip, so assuming this setup

const router = Router();

router.all('/api/*', endpoints.handle);
router.all('/webhooks/*', webhooks.handle);
router.all('/websocket/*', websocket.handle);
router.all('*', () =>
  jsonResponse(
    {
      error: "Resource doesn't exist.",
    },
    404
  )
);

export default {
  fetch: router.handle,
  scheduled: async (
    event: Event,
    env: EnvInterface,
    ctx: ExecutionContext
  ): Promise<void> => {
    ctx.waitUntil(cronTrigger(env));
  },
};

I will need to create a new function where I call the router.handle(request, env,ctx).then(response) but I was thinking, technically I can still use async-await example in pseudo-code

async function main(request, env,ctx) {
 const response = await router.handle(request, env,ctx)
// manipulate response object
return response
}

the only downside of this approach is the fact the response handler will be apply to all routes

danbars commented 2 years ago

Right. This downside is the reason that this ticket exists. It is less modular to write code this way.

בתאריך יום ד׳, 20 באפר׳ 2022, 12:24, מאת Stephane Moser ‏< @.***>:

Thanks for the tip, so assuming this setup

const router = Router();

router.all('/api/', endpoints.handle); router.all('/webhooks/', webhooks.handle); router.all('/websocket/', websocket.handle); router.all('', () => jsonResponse( { error: "Resource doesn't exist.", }, 404 ) );

export default { fetch: router.handle, scheduled: async ( event: Event, env: EnvInterface, ctx: ExecutionContext ): Promise => { ctx.waitUntil(cronTrigger(env)); }, };

I will need to create a new function where I call the router.handle(request, env,ctx).then(response) but I was thinking, technically I can still use async-await example in pseudo-code

async function main(request, env,ctx) { const response = await router.handle(request, env,ctx) // manipulate response object return response }

the only downside of this approach is the fact the response handler will be apply to all routes

— Reply to this email directly, view it on GitHub https://github.com/kwhitley/itty-router/issues/85#issuecomment-1103689650, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABOVDOZERACLGSA6WLCUZSDVF7ELXANCNFSM5MSH6BFA . You are receiving this because you commented.Message ID: @.***>

kwhitley commented 2 years ago

Here is what I do. Not ideal, but works. Having real middlewares would be awesome.

I mean, by its definition, middleware is simply anything that operates in the middle (in this case of a request and the eventual response/return). Any handler is technically middleware, the same as in Express. The only things differentiating itty's middleware from Express middleware is:

import { ResponseMaker } from './somewhere'

// all handlers take the arguments you pass to the handle, in that order... so it's easy to change things up
const middleware = (req, res) => {
  res.status(400) // let's pretend this is how we modify the response-like object
}

const router = Router()

router
  .all('*', middleware)
  .get('/foo', (req, res) => {
    // res.status === 400 from the middleware

    return res.json({ error: 'Bad Request' }) // let's pretend this returns a JSON Response
  })
  .get('*', (req, res) => res.status(404).end())

// we change the default signature of handlers this easily...
export default {
  fetch: (req, ...other) => router.handle(req, new ResponseMaker(), ...other)
}
danbars commented 2 years ago

The feature that I'm missing from express style is the ability to do something at the beginning before all other middleware, and then at the end after all middleware ran and returned their response. Examples:

With your method of using ResponseMaker once a middleware returns you will not reach the "after" middlewares.

It is a nice trick, but doesn't solve all cases

danbars commented 2 years ago

Something that I just thought about, but didn't try, is to create another instance of Itty Router that will handle the response middlewares.

event.respondWith(
    mainRouter
      .handle(event.request, event)
      .then(response => {
        return responseRouter(event.request, response)
      })
      .catch(async error => {
        return await errorHandler(error, event)
      })
  )

This way you can configure responseRouter with multiple handlers per method/path.

markusahlstrand commented 2 years ago

I ended up with a solution where the handler can return a function that in turn returns a promise, which works well for me but it is unfortunately a pretty large change to the router and not really in line with the simplicity philosophy of this project. If you're interested I can share this solution @danbars