Discussion: Top-Level Router API

[elithrar]

Might as well start this to pick up where gorilla/mux#130 leaves off.

Some high-level comments:

• func (r *Route) Handler(handler http.HandlerFunc, methods ...string) *Route should accept a http.Handler and have a sister method HandleFunc that accepts a http.HandleFunc.
• Not sure where this puts the Get, Head, etc. convenience methods—additional methods for these would clutter the API.
• Should router.Sub return a *Router instead, allowing you to use the same methods as you would on a *Router? prefix could be a field of Router that the call to router.Sub sets.

[moraes]

It's ok to have both Handle(h http.Handler, m ...string) (notice that it's not Handler, to be consistent with http.ServeMux) and HandleFunc(h http.HandlerFunc, m ...string) methods. The convenience methods Get(), Post() keep accepting a HandlerFunc, and there's no need for equivalents that accept a Handler.

A Subrouter is a very lightweight route factory, while Router carries data and logic that go well beyond this. Making Subrouter() return *Router would mix this distinction and require Router to have a prefix and a router field itself (empty and nil for the main Router) and check if it is the main router in every method, and call the main router if not, and so on, while a Subrouter would not use any of the fields used by the main router. It would be a type trying to act as two types, and this sounds like a messy design.

So Subrouter should only have methods to create a Route and another Subrouter. I don't see it having more than 2 methods, ever.

Maybe Subrouter just needs a better name, because it's not really a router.

[garyburd]

The http.HandlerFunc type should not be used as the handler function argument type. The documentation for http.HandlerFunc does not apply to the use of the type in this package. The HandlerFunc.ServeHTTP method is not needed here. Both of these issues can confuse newbies.

I suggest using the anonymous function type func(w http.ResponseWriter, r *http.Request) or declaring a named type in this package. I prefer the anonymous type because there's one less named type for the programmer to understand.

[moraes]

Interesting. I never even considered not using HandlerFunc, but thinking about it, the anonymous function way would suffice.

[elithrar]

RE: the sub-router having only two methods - this might confuse users since you don't get access to any of the convenience methods on *Router - e.g. r.Route and makes a sub-router a much more limited router.

e.g.

s := r.Sub("/users")
s.Handle("/new", myHandler)      // matches "/users/new"
s2 := s.Sub("/{id}")
s2.Handle("/", myHandler)        // matches "/users/{id}/"
s2.Handle("/tickets", myHandler) // matches "/users/{id}/tickets"

... could instead be:

r := router.New()
r.Get("/", indexHandler)
s := router.New()
r.Handle("/users", s)  // tell the top-level router to pass any requests prefixed with "/users" to s.
s.Handle("/new", userHandler)      // matches "/users/new"
s.Get("/list", userList)    // matches "/users/list"

This is functionally similar to the way Goji (https://godoc.org/github.com/zenazn/goji/web#Mux) tackles sub-routers.

You can have packages initialize their own routers (e.g. JSON API, web, static) and then import them in your package main and plug them into your top-level router via r.Handle("/prefix", package.ExportedRouter).

[moraes]

In this case:

r := router.New()
r.Get("/", indexHandler)
s := router.New()
r.Handle("/users", s)
s.Handle("/new", userHandler)
s.Get("/list", userList) 

...when you call s.URL(name) would you get a URL with or without the /users prefix?

[kisielk]

This may be just my opinion, but I think we could do without the convenience functions. It makes declarations less readable because there's more different kinds of functions, and s.Handle("/foo", h) and s.Get("/foo", h) are already basically redundant (unless I'm missing something...). It saves a very small amount of typing in exchange for a loss in readability and a bigger API.

[kisielk]

Another thing to consider is to use option types like the CSRF package instead of strings, and then have the function be Handle(path string, options ...Option). The route declarations would then be r.Handle("/foo", muxy.Post) etc. However you could also pass other option constructors in to Handle.

[moraes]

@elithrar: The name Subrouter in this initial proposal is a bit misleading, because it is not doing any nested dispatch, nor it's merging routes from one router into another. It is just a convenience to register URL's that share a common prefix (a domain and/or path). It's dumb but kinda lovely.

To achieve what you want, we would need something different. For example, package1 defines a router:

r := mux.New()
r.Route("/bar").Handler(barHandler)

And package2 imports it and submounts it under a given prefix:

r := mux.New()
r.Mount("/foo", package1.r)

In the end, only package2's router would know how to build the URLs that are being served, though.

[garyburd]

we could do without the convenience functions

Here's an example without the convenience functions:

r.Route("/foo").Handle(h, "GET")
r.Route("/bar").Handler(getBar, "GET").Handler(updateBar, "POST")

and the same example with the convenience functions:

r.Route("/foo").Get(h)
r.Route("/bar").Get(getBar).Post(updateBar)

[moraes]

The convenience functions are a delicate dilemma.

Sure, the API would be tidier without them, and they could be defined elsewhere through proxy types, but they make usage easier and prettier for common enough scenarios, and I think it has been proved that people like them.

[elithrar]

I'm definitely +1 for the convenience functions. They make sense when you
have separate handlers for each method (e.g. GET vs. POST). There's
obviously a small API surface cost but I think enough routers in Go-land
provide similar methods to make the cost reasonable.

On Tue, Oct 27, 2015 at 7:40 AM rodrigo moraes notifications@github.com
wrote:

The convenience functions are a delicate dilemma.

Sure, the API would be tidier without them, and they could be defined
elsewhere through proxy types, but they make theusage easier and prettier
for common enough scenarios, and I think it has been proved that people
like them.

—
Reply to this email directly or view it on GitHub
#1 (comment).

[AaronO]

@moraes I definitely think Router.Mount() would be really nice to have, I've written my thoughts down here: gorilla/mux#136

[moraes]

@AaronO, soon:

// Mount imports all routes from the given router into this one.
//
// Combined with Sub() and Name(), it is possible to submount routes using
// pattern and name prefixes:
//
//     r := New()
//     s := r.Sub("/admin").Name("admin:").Mount(admin.Router)

[muir]

I have a use case where I want to combine routers. Some method to let me do that would simplify my code by 200+ lines.

With a way to combine routers, I could drop half of my documented features: http://godoc.org/github.com/BlueOwlOpenSource/endpoint