caddyhttp: Implement named routes, invoke directive
#5107
Conversation
francislavoie
added
feature ⚙️
francislavoie
force-pushed
the
named-routes
branch
from
3f7a400
to
75a1df6
Compare
There was a problem hiding this comment.
Thanks Francis -- sorry it took me so long to get to this. It's definitely not a simple change...
Is it worth it do you think? I do feel like it increases the complexity of the Caddyfile a lot, not just the code but also the comprehension (like snippets).
I wonder if we could mark this feature as experimental just in case we need to change or remove it later, but if it saves GB of RAM for large deployments at virtually no performance penalty, then maybe it's worth it.
| func (p *parser) isNamedRoute() (bool, string) { | ||
| keys := p.block.Keys | ||
| // A named route block is a single key with parens, prefixed with &. | ||
| if len(keys) == 1 && strings.HasPrefix(keys[0], "&(") && strings.HasSuffix(keys[0], ")") { |
There was a problem hiding this comment.
I wonder if the named route block should/could just be prefixed with & instead of including the parens?
There was a problem hiding this comment.
Maybe, but I like keeping the parens to keep it similar to snippets because it works similarly but more optimized (reference instead of copying)
There was a problem hiding this comment.
I just feel like &(foo) is a lot of typing for what programmers are used to seeing as &foo 🤷
There was a problem hiding this comment.
Typing two extra characters is not significantly more. Without parens it would look too much like a site block address.
modules/caddyhttp/routes.go
Outdated
|
|
||
| // Compile prepares a middleware chain from the route list. | ||
| // This should only be done once: after all the routes have | ||
| // been provisioned, and before serving requests. |
There was a problem hiding this comment.
This is actually called while serving requests though, right?
There was a problem hiding this comment.
That code comment was copied from func (routes RouteList) Compile. You're right though, it's called from invoke's ServeHTTP. I think the RouteList one's comment is also wrong though, because it gets called from Subroute.ServerHTTP and such. I'm not sure if how "compiling" is done was changed way back in early versions of v2 🤷♂️
There was a problem hiding this comment.
I adjusted the comments for these. Let me know if you agree 👍
|
I think it's useful. There's a lot of situations where it can help manage memory usage and reduce startup time by only doing work once for things that might happen in multiple sites. 👍 to marking it experimental. |
francislavoie
force-pushed
the
named-routes
branch
from
75a1df6
to
0892043
Compare
francislavoie
force-pushed
the
named-routes
branch
from
0892043
to
d7b8bf4
Compare
There was a problem hiding this comment.
I suppose it is time to give this a try 😄
Definitely mark it as experimental in the docs, subject to change. I'm not 100% on the &(named) syntax yet (looks like a "reference to a copy" to me) -- I think I still prefer &named, but there are compelling arguments both ways.
Thanks for working on this 😊
Closes #4995
Adds a new
named_routesmapping on servers, and these routes can be executed with aninvokehandler by their name.In the Caddyfile, defining named routes looks similar to snippets, but with a
&prefix which makes it read more like a reference than a copy, where a snippet is basically a copy (that's the idea anyway).The Caddyfile adapter does a bunch of funky stuff to only pull in named routes into servers where they are used with an
invokedirective. There's some juggling of options, state and piles, which... I don't super love but it works I guess. If you can think of a cleaner way to do that in code, I'm all ears.I'm also not sure where
invokeshould go in the default directive order. I put it just aboverouteandhandlefor now (because it technically makes a subroute), but it's debatable where it should go.