CrowdHailer / raxx

Interface for HTTP webservers, frameworks and clients
https://hexdocs.pm/raxx
Apache License 2.0
401 stars 29 forks source link

Add middleware interfaces and integration #139

Closed nietaki closed 5 years ago

nietaki commented 5 years ago

Tested (and hooked up to the router). I think I got it to a polished and stable enough state to discuss it and finish it up.

Should close https://github.com/CrowdHailer/raxx/issues/136.

Remaining tasks

Vital

Optional

Cleanup/housekeeping

CrowdHailer commented 5 years ago
nietaki commented 5 years ago
  1. There might be a better name we could use, that doesn't have a strong connotation in the Elixir world yet. Chain might be a good one, especially if you think about it as Raxx.Middleware.Chain :)
  2. I don't think middlewares only make sense in the context of router and controller, the router is just what they come pre-integrated with. In principle you should be able to augment any Raxx.Server with middlewares, exposing the same interface outside: [Raxx.Middleware] -> Raxx.Server -> Raxx.Server
    • I don't the right way of attaching some middlewares to a Server is not by baking them into it, as in the MyServer example. In that case, whatever logic you implement in MyServer is impossible to use without the pipelines anymore. What could be done instead would be something like the following:
      defmodule VanillaServerWithMiddleware do
      use Raxx.Pipeline(list_of_middleware, VanillaServer)
      # (nothing else required)
      end

      That way VanillaServer can be re-used in other situations, but you don't have to write the plumbing yourself.

That's sort of what I meant by putting middleware before router matches: If in the example above you replace list_of_middleware with [{Raxx.Middleware.Head, nil}] and VanillaServer with SomeRouter, you won't have to put the :HEAD type request matches in the router itself. I think it's relatively elegant and generic.

  1. I did notice the similarity between the middleware interface and the server interface, but in the end I convinced myself that it makes sense that they're very similar, but not the same. Still, we can think of a server as a middleware that never calls down to the rest of the pipeline and we could generate valid middleware modules from all valid server modules, but not the other way around. I'd be curious to see a unifying interface, but it would be important to me that if you implement the Raxx.Server interface, you don't have to think about middlewares and that the pipeline type still can remain opaque.
CrowdHailer commented 5 years ago

What of the original checkpoints are still relevant? Curious about this point

document putting middlewares before router marches (it's possible and not too difficult!)

nietaki commented 5 years ago

I knew extracting Raxx.Stack.State was slightly overkill, but I thought it would be cool to clearly separate the state and all functions it has to provide, so changing its structure in the future is very self contained and pleasant. But I wouldn't be surprised if at some point in time it made sense to bring it back to Raxx.Stack so that, for example, we can inline all the private helper functions for performance.

I think that as long as the state type is opaque and everything related to it is undocumented, there's no really wrong way of doing it in this iteration.

I looked through the original checkpoints yesterday and I think all of them are still valid. I was thinking of adding another one: make Router middleware-aware, but it's probably best tackled as a separate task - it's a good amount of careful API design.

WRT this:

document putting middlewares before router marches (it's possible and not too difficult!)

it was meant to say "matches". What I meant to say was that having middlewares that change the request before it gets to the router routing stage would be easy to do as well - just need to have the router as the Server in the Stack and the middlewares before it.

This note was written with the assumption the Router will already have been modified to accommodate middlewares, which it hasn't yet.

CrowdHailer commented 5 years ago

Yep I think tackling router as another PR is a good idea. I had some other thoughts about changing from a general purpose match to just matching on path and method, but that's best for another discussion

CrowdHailer commented 5 years ago

Diagrams are a good addition

nietaki commented 5 years ago

@CrowdHailer I think it's ready 🎉 If you find anything that's undocumented, let me know.

Some of the moduledocs refer to "controllers" which doesn't use Raxx terminology and might not feel right to you, feel free to rewrite or discuss it. I found it hard to "write around" the double meaning of server - HTTP server and Raxx.Server.

I run out of the day before unit testing Raxx.Stack getters and setters, but they are tested by the more end-to-end Stack tests.

CrowdHailer commented 5 years ago

@nietaki I've hit approve but assume that you can't actually do the merge