Chi: Mux methods accept a HandlerFunc instead of Handler

[kevinconway]

I see the change that converts the mux signatures to accept an http.HandlerFunc instead of an http.Handler here: https://github.com/pressly/chi/pull/48/commits/2b1ed9c5ae2bcffa8c7da4fd4ce9e78aba8da839. There's no rationale given for the change.

In the std lib, http.Handler is _the_ handler type and http.HandlerFunc is an adapter for functions to match the http.Handler interface. With the current mux implementation, anything that is an http.Handler type must be passed in like this:

router.Get("/", myHandler.ServeHTTP)

The ServeHTTP function is converted back into a Handler by being wrapped by the HandlerFunc type. It seems redundant and, in general I believe, breaks the expectation that a mux will accept a Handler implementation. This is also inconsistent with the Handle function of the mux and the middleware type definition of func(http.Handler) http.Handler.

Most projects in the pre-1.7 space handled this case by having two signatures for each mux function:

func (m *Mux) Get(path string, h http.Handler) {}
func (m *Mux) GetF(path string, h http.HandlerFunc) {}

I'd like to get http.Handler support back and am willing to put in a patch if we can figure out the right implementation for this project.

[lwc]

Dupe of https://github.com/pressly/chi/issues/79 and was also mentioned in the original PR https://github.com/pressly/chi/pull/48#r72554065

[pkieltyka]

@lwc thanks for the references.

@kevinconway yea it's been talked about a few times and I've spent a lot of time thinking and iterating to try to get the cleanest design of the chi Router.

Here were the options..

1. For each routing method function, we have variations accepting a http.Handler or http.HandlerFunc - as described in this issue: Get(string, http.Handler) and GetF(string, http.HandlerFunc)
2. Single routing methods that accept handler argument of type http.Handler
3. Single routing methods that accept handler argument of type http.HandlerFunc

---

Option 1 isn't ideal because it makes the chi.Router interface huge, by adding another 9 functions to the interface. The simplicity and surface of the Router is important to me to stay as minimal as possible. Btw, I was initially thinking to support Get(string, http.HandlerFunc) and GetH(string, http.Handler).

Option 2 is what I had originally in chi v1 until I realized option 3 is a bit more elegant because it offers a cleaner API. Under option 2, you'd have to wrap endpoints such as..

func myHandler(w http.ResponseWriter, r *http.Request) {
  // ...
}

with http.HandlerFunc(myHandler), such as r.Get("/", http.HandlerFunc(myHandler)). This would be quite common and isn't ideal.

Option 3 (the current implementation) accepts func(http.ResponseWriter, *http.Request) to the routing methods as its the underlying type of http.HandlerFunc, which I believe is the most common case for passing to routing methods. The downside is for http.Handler's that are passed, they must call .ServeHTTP to make it a HandlerFunc signature. I believe if I'm foregoing option 1, then this becomes to best choice.

[kevinconway]

with http.HandlerFunc(myHandler), such as r.Get("/", http.HandlerFunc(myHandler)). This would be quite common and isn't ideal.

I believe this is consistent with the standard library. The net/http module provides a Handle method that accepts any http.Handler implementation and then a HandleFunc that accepts any http.HandlerFunc implementation. My personal inclination is to match the standard lib so this mux becomes a drop-in replacement if most cases.

it makes the chi.Router interface huge, by adding another 9 functions

For cases outside of the usual Handle and HandleFunc cases, I think another possible approach is to move the HTTP method to an input string rather than the function name which would only require two new funcs:

r.HandleM("GET", "/", myHandler)
r.HandlerFuncM("GET", "/", myHandlerFunc)

In any case, if you're pretty settled on Option 3 then you can close this issue. I'll keep passing .ServeHTTP around.

[pkieltyka]

@kevinconway good point about having Handle() and HandleFunc() the same in chi as in http.ServeMux - and in fact I checked and its matches up in the current version.

Interesting idea to pass the http method, perhaps we'll do that sometime. For now, yea let's stick to option 3 and we can leave this ticket open for a bit for others to offer some feedback too.

[zythosec]

@pkieltyka @kevinconway new to this discussion, and not sure what you mean by passing around ServeHTTP, but one way I've implemented it that may help (and not sure how it can be incorporated into chi or even that it needs to be) is by wrapping the chi.Mux.ServeHTTP method.

I define a custom Router type that wraps a chi.Mux and defines a ServeHTTP method that wraps the chi.Mux.ServeHTTP method:

type Router struct {
    Mux    *chi.Mux
}

// ServeHTTP wraps the ServeHTTP of our mux choice
// This allows us to pass it into http.ListenAndServe and http.ListenAndServeTLS
func (r Router) ServeHTTP(w http.ResponseWriter, r *http.Request) {
    r.Mux.ServeHTTP(w, r)
}

All my handlers get to keep the same standard signature:

func myHandler(w http.ResponseWriter, r *http.Request) {
   // ... 
}

I create a chi.Mux and add middleware, initialize routes, etc. and then initialize the router with the mux:

mux := chi.NewRouter()
mux.Get("/", myHandler) // option 3 syntax
r := &Router{Mux: mux}

So now I can pass it into http.ListenAndServe:

http.ListenAndServe(":3000", r)

Let me know if I'm completely out in left field, but this pattern has worked for me and has been extremely nice and clean to work with.

[pkieltyka]

hey @rossnanop - hmm, you can definitely wrap the chi router, and I encourage it as necessary to compose additional logic, but in the small example you already have this out of the box:

r := chi.NewRouter()
r.Get("/", myHandler)
http.ListenAndServe(":3000", r)

[zythosec]

Yeah I just realized that was what I was doing, and that I wasn't adding anything to the discussion. Sorry for the noise. I guess my point was option 3 works nicely for me. 😄

[DeepAnchor]

I know this is an old issue (and closed), but I have to say that I am in agreement with @kevinconway and the

r.HandleM("GET", "/", myHandler) r.HandlerFuncM("GET", "/", myHandlerFunc)

seems like it would be the most elegant way to solve the problem, while still keeping in line with the project's spirit of minimalism.

The use-case is when you have complex request handlers with non-standard signatures that simply implement the http.Handler interface. For example in scenarios like this where you have a request handler that returns an error -a pretty common case.

Anyway, my 2c for future versions is to forego option 1 from above and make http.Handler a first class citizen of Chi.