andywer / srv

📡 Functional node server. Composable routing. Take a request, return a response.
MIT License
9 stars 1 forks source link

Call for feedback #1

Open andywer opened 5 years ago

andywer commented 5 years ago

Have any feedback? Is the documentation understandable?

Post your comment here 👇

andrenarchy commented 5 years ago

Some thoughts:

  1. I think the Middleware approach can be simplified drastically. In fact, I think we don't need anything like derive at all so srv could be even more lightweight. Let's say I want a middleware that logs every request with bunyan. Then I just define a function
    async function requestLogger(logger: Bunyan, handler: RequestHandler) {
      return (request: Request) => {
        const before = new Date().getTime()
        const response = await handler(request)
        logger.log({
          req: ..., 
          time: new Date().getTime() - before
        })
        return response
      }
    }

    So the route would look like

    const greet = Route.GET("/welcome", requestLogger(logger, async (request) => {
      // ...
    })

    Usually you always want to run the same middlewares for all routes (logging, authentication, error handling, ...) – you can just compose them to one function middlewares and have them explicit in the route handler instead of registering them in the Service / Server:

    const greet = Route.GET("/welcome", middlewares(async (request) => {
      // ...
    })

    This way the route is self-contained and there's no doubt about whether some middleware has been registered elsewhere. In my opinion this makes the derive function as well as the next handler superfluous, thus further simplifying the srv API. What do you think?

  2. I'd rename Service to Server – not a biggie but server.listen() sounds more natural to me.
  3. README: under features the sections Functional and Everything is a function are redundant.
andywer commented 5 years ago

Thanks, @andrenarchy! 🙂

  1. Yeah, that concept is not documented right now. The logging middleware example was rather tailored towards a use case where you want to request.log.warn("Something didn't work, but i won't fail either.") in the route handler. But you might be right, it will be a mess to statically type it if not explicitly wrapping it...

  2. Interesting point. My reason for calling it "service" was that it's the high level thing whereas await service.listen() returns the (low-level) "server". Makes sense or non-sense?

  3. LOL. True 👍

andrenarchy commented 5 years ago
  1. I'd clearly favor an explicit wrapping – it's easier to see what's going on and also easier to type w/ TypeScript I guess.
  2. That makes sense. Then I'd leave it as it is. :)
  3. trololo. :)
andrenarchy commented 5 years ago

I guess a useful approach is to just implement some very common things quickly and see how this looks like. My list would start with this: request logging, authentication, body parsing, validation of headers/body/parameters, CORS, error handling.

andywer commented 5 years ago

Check out #5 😉

A basic error handling middleware is already on board: src/middlewares/error.ts.

andywer commented 5 years ago

PS: Just noticed that at least the argument that the current middleware concept will be almost impossible to type with TypeScript will probably not be true anymore when TS 3.4 gets released.

Will think a little more about the middleware API. On a side note: In a way the current API is basically providing an implicit compose()/pipe(), so that you don't end up with code like d(c((b(a()))), but a flat [a, b, c, d] that does the same.