Suggestions

[earthboundkid]

Very cool! I like this a lot. I wrote my own router which is also a sort of stripped down take on Chi/Way/httprouter. It's interesting to see more versions of this idea.

Some thoughts:

• .Use(mw) can be changed to .Use(mws...) to save some typing in some cases.
• Integer params come up a lot, so func ParamInt[Int int|int32uint32|int64|uint64](ctx context.Context, param string, n *Int) (ok bool) would be nice to have.
• I find that mounting static files also comes up a lot, so I wrote a Router.Mount convenience method. Something to think about.
• It would be faster at route time but also better in terms of failing fast to parse the regexes up front. I suggest changing routes to

type route struct {
    method   string
    segments []string
    matchers []*regexp.Regexp 
    wildcard bool
    handler  http.Handler
}

and Handle to

    segments := strings.Split(pattern, "/")
    var matchers []*regexp.Regexp
    for _, seg := range segments {
        key, rxpattern, ok := strings.Cut(seg, "|")
        if !ok {
            continue
        }
        rx := regexp.MustCompile(rxpattern)
        matchers = append(matchers, rx)
    }
    for _, method := range methods {
        route := route{
            method:   strings.ToUpper(method),
            segments: segments,
            matchers: matchers,
            wildcard: strings.HasSuffix(pattern, "/..."),
            handler:  m.wrap(handler),
        }

        *m.routes = append(*m.routes, route)
    }

And in match you can just use the next matcher in the slice whenever you see a pipe.

[peterbourgon]

Also, maybe I'm daft, but does Mux.routes need to be *[]route or can it just be []route?

edit: and, actually, I bet you'd be better served by []*route — fewer surprises if and when you decide to add capabilities to that type, and (probably marginally) less impact to the GC.

[alexedwards]

Hey @carlmjohnson! Thanks for the positive feedback and ideas :+1:

The suggestion of .Use(mws...) makes sense, I'll make that change.

The reason for compiling the regexp patterns with each request was because I don't really use this feature often and it was the simplest way of writing it :smile: But I agree that if this package is going to be used more widely then it makes sense to change to an up-front regexp compilation. I like your suggested code, and I'll have a think myself too about how to implement it.

WRT the other two suggestions, I'd like to keep the package API small and not add features unless there is a lot of demand for them. For now, I'll add a note about handling static files to the README along with an example that people can perhaps copy-and-paste from, and keep them both in mind as potential additions for the future.

[alexedwards]

@peterbourgon The reason for Mux.routes being a *[]route rather than []route is to make the 'middleware grouping' feature work correctly. When Mux.Group() is called it creates a shallow copy of the Mux struct which you then call Handler() and HandlerFunc() on to add the routes. Making Mux.routes a pointer to a slice means that we can effectively mutate the Mux.routes slice at the original memory location when adding the route with append (rather than creating a copy at a different memory location).

There might be other ways to achieve the same end goal, I'm open to suggestions if you can think of a cleaner and clearer way to manage it.

Edit: tweaked wording to be more precise

[peterbourgon]

Ah, I see!

The Handle/HandleFunc methods are capturing the state of the outer Mux — most importantly, the middlewares that have been defined in it so far — and folding that information into the route that gets created. This may be a tiny bit surprising. Consider the following case:

mux := flow.New()
mux.Use(exampleMiddleware1)
mux.HandleFunc("/foo", ...)
mux.Use(exampleMiddleware2)
mux.HandleFunc("/bar", ...)

Which middlewares should be applied to the /foo route?

Currently, /foo gets exampleMiddleware1 only, and /bar gets both. But should that be the case? I dunno! My intuition is that both routes should get both middlewares, as I define both routes and middlewares on the same Mux value. But definitely a judgment call.

My approach to something like this would probably be to maintain routes and middlewares separately, and defer application of the latter to the former somehow. By default, maybe with each request? Or, you could maybe "freeze" the mux at some point — explicitly? on first request? — and evaluate the request stack then. This points at something else, which you might want to document: at the moment, the Mux is definitely not goroutine-safe 😉 which means once you start ServeHTTPing from it, you can't do any more Handle or Use calls.

[lucsky]

My intuition is that both routes should get both middlewares

My intuition is that only the last one should 😁

Middlewares are called middleware for a good reason, which is that they are layers of logic in a stack of handlers where order does matter. So if a handler is sitting "lower" in the stack then more middlewares are potentially traversed to reach it, and if a handler is sitting higher in the stack then middlewares sitting below will be "avoided", sort of like an early return in a chain of functions.

If that makes any sense 😊

[alexedwards]

@peterbourgon Thanks for these comments, I appreciate it.

I've updated the documentation with a warning that it's not safe to add more middleware and routes once the mux is in use by a server.

I agree with @lucsky that the current behavior WRT middleware being 'folded in' to a route is probably OK, but to avoid any element of surprise I've updated the documentation notes to explain this explicitly.

[peterbourgon]

@alexedwards Nice!

[alexedwards]

@carlmjohnson I've updated the code so that Use() is now variadic, and regexp patterns are compiled once upfront rather than on every request. Thanks again for the suggestions :+1: