Closed vishr closed 8 years ago
/cc @abador
Thanks for the heads up!
Just out of curiosity: what exactly is holding back the fasthttp
integration? Is is too complex to maintain two (or more) engines? Is it because some stuff are falling behind the actual go releases/features (like http/2 not being implemented yet) and so on? Or something else?
Just asking because some people, if not most, are attracted to the piece of the graph that shows how much faster echo is when using fasthttp
- so, removing it could be pretty big (in a bad way) at least for the new people looking for new web frameworks.
But I trust @vishr's decision on what might be the best for the long run.
Is is too complex to maintain two (or more) engines?
Yes, it has been a challenge to maintain two implementation with one interface, we have to compromise on features like you mentioned.
Just asking because some people, if not most, are attracted to the piece of the graph that shows how much faster echo is when using fasthttp - so, removing it could be pretty big (in a bad way) at least for the new people looking for new web frameworks.
I think we need to find newer way to attract people ;), probably we will put old routing benchmarks :). We will figure it out :)
In short, we want to be aligned with Go http
to benefit from all the features (current and future).
I agree. In my opinion, in the long run maintainability is a better path to follow.
fasthttp is an interesting library and although it of course provides some great numbers for the Go-router-benchmark obsessed in reality it's use-cases are rather quite niche and I don't think the touted benefits are really worth the compromises that trying to support it within the one framework introduces.
I found myself using echo less and less with v2 as it just seemed to have become more complex and laborious to use when much of the complexity wasn't delivering immediate benefits (if using the standard engine).
Router benchmarking in the Go ecosystem has jumped the shark IMO - most of the frameworks around now are more than fast enough for most people and the perf differences are imperceptible to an app as a whole so focusing on features, stability and ease-of-use makes much more sense.
I'd rather see things being aligned with the Go standard framework more and fewer external dependencies. Less is more.
That is some serious changes, I'm mostly Ok with. But I'm not sure if the versioning scheme fits and reflects the amount of changes, especially the breaking ones. I'd better issue a 3.0 release. I'm afraid v2.1 will cause a lot of confusions especially for projects using gopkg.in
Agreed, 3.0 makes more sense.
Great decision! If context still will be interface? It would be nice to implement something like this https://github.com/go-playground/lars#custom-context--avoid-type-casting--custom-handlers and make echo more extensible!?
@lugorian Is it something similar to https://echo.labstack.com/guide/context?
I think that it's the right way. My opinion is that if someone really needs fasthttp they use it natively.
@vishr Yes it is similar but with some neat tricks to avoid context casting again and again. Also provides nice api for composing custom framework..
Look at this examples: https://github.com/go-playground/lars/blob/master/examples/all-in-one/main.go https://github.com/go-playground/lars#custom-context--avoid-type-casting--custom-handlers It is really nice way to register custom context, handler and use it instead of default one..
Lars router is inspired by echo(among ather frameworks), maybe echo can borrow few tricks back from lars? ;)
What do you think?
@lugorian Will look into it soon.
@vishr If we're talking about simplicity. One of the biggest warts that echov2 has is the ridiculously big Context interface.
I'd like to make a case for simple interfaces and composition. In PR #470, I show how this interface could be greatly reduced in complexity by isolating it's pieces. However in this PR I was trying to be mostly API compatible and so it stayed an interface.
https://github.com/aarondl/ultimateq/blob/master/irc/writer.go#L103
This is a good example of what I would do with this to make it simpler. io.Writer
is the only thing that provides any functionality that would ever change in this example. Context
is the same as this, it needs various "pieces" like a http.ResponseWriter
, and a net.Context
etc. Of course you can keep the interface AND implement it like this, as you can see in my example I also have an interface describing the massive implementation:
https://github.com/aarondl/ultimateq/blob/master/irc/writer.go#L39
I'm not hell-bent on one approach or the next, it just seems like a good time to clean up one of the messiest parts of the library and I wanted to provide an example of how it may be done :)
Hey @lugorian @vishr
just wanted to mention about using a Context interface for handlers; I recently struggled with this in my package lars. The recent implementation of the context
package to the native http.Request object has/will throw allot of frameworks for a loop; it almost makes chaining of std middleware mandator eg.
func(next http.HandlerFunc) http.HandlerFunc {
return func(w http.ResponseWriter, r *http.Request) {
}
}
nosurf CSRF middleware is a perfect example, it was recently updated to use context
on the http.Request object and in an application I was using the token wasn't being returned because adding the token to the context actually shallow copies a new http.Request object and so the caller did not have or know about the token added to the shallow copy's context and so if using Context as an interface wouldn't really work unless:
// because 'r' is a copy of a pointer to allow the information to get
// back to the caller, need to set the value of 'r' as below with '*r'
func(w http.ResponseWriter, r *http.Request) {
*r = *r.WithContext(context.WithValue(r.Context(), 0, "testval1"))
}
it may not be a good idea to use a Context interface, I ranted about this in releases 3.3.0 Special Notes section and 3.3.1
I even went so far as to create a new router pure to avoid all of these issues, here is an example application I whipped up to show how it would work here
not saying you shouldn't use it, just highlighting some issues I've come accross, good luck with v3! 😃
@joeybloggs Thanks for the detailed information. I will definitely keep everything in mind for v3.
Update: Context in Echo is the core of the framework, dropping it would not be a good move. We definitely need to understand this issue and address it differently.
@aarondl I am failing to understand the benefit of splitting the context into multiple interfaces. If you need to extend context, you can simply do it as mentioned in this guide https://echo.labstack.com/guide/context
@vishr It's just about cleaner code. Current implementation goes against everything interfaces should be, small and composed. If people are somehow happy with the way the Context is implemented I'll be quiet. I just wanted to note that v3 is a refactoring opportunity for something I find unpleasant about echo, we could change it to be more idiomatic and cleanly separated. I'll leave it at that :)
Would be great if you could also try to keep the functionality provided by echo to a minimum (i.e. Cookie implementation in v2 could be handled almost identically without echo).
I'm starting to use v2 right now, having a hard time understanding exactly what is being considered for v3. Is echo.Context going away? I'm currently using things like c.Bind, c.Param, c.Get/c.Set, c.JSON, and c.Attachment - will all of my handlers need to be rewritten?
@bunkat Context
mostly remains same, you will have all those methods.
@vishr As you probably already know x/net/websocket
is kinda unmaintained and has some serious shortcomings, most of which are listed in the Readme of gorilla/websocket
(especially that it can't deal with messages consisting of multiple frames). So do you think it's feasible to make Echo v3 compatible with gorilla/websocket
instead of or additional to x/net/websocket
?
@lhecker Waiting for opinion from people...
+1 to gorilla/websocket
@vishr echo.SetRenderer
seems to be gone from Echo
, still mentioned in Context
though, is it WIP?
NB: Echo.Renderer
is now public. We just need to fix the Context#Render
doc.
@leedstyh We have dropped WebSocket
API as it is much easier to to write your own handler based on the WebSocket API of your choice.
See: https://github.com/labstack/echo/tree/v3/recipe/websocket
This is a great decision in my opinion. With all the discussions about whether or not to use a "webserver framework" with go, it seems like a good idea to keep the functionality directly provided by the echo framework to a minimum in favor of allowing users to use custom - ideally standard library compliant - packages for specific functionalities.
By providing a good documented list of corresponding recipies, this should also be feasible for go newcomers.
But why dropping fasthttp if it had the performance scale as mentioned in README
?
@sarathsp06 many edge cases, no http/2, proved difficult to maintain a multi-engine framework.
@tikiatua Seconded.
@vishr When the v3 official version will be released?
@vishr is there some doc on how to help to get v3 out ? i would love to help!.
@apaganobeleno @freWalker: Code changes are almost done, however we need support from people:
If anyone is interested in updating docs (they are part of the main repo now), please let me know.
I am interested. @vishr
@vishr me too 👍
In docs:
@bluealert: guide/ @apaganobeleno: recipe/
As a first round, lets start with the above and just update code mentioned in the docs to v3 (make sure the code works!).
@vishr regarding recipe/*
should i simply check those work inside v3
branch ?
@apaganobeleno It is a hugo static site, you can run it locally. I have made sure all recipe code is compiling with v3, we just need to check if docs/recipe/* reflect the right version - all in v3 branch. Later if you are interested, you can looking into docs/middleware*.
@vishr ok, thanks for clarifying, all clear now, just one more thing, by docs/recipe/*
you mean https://github.com/labstack/echo/tree/master/website/content/recipes
right ?
Yes.
@vishr i just went through the recipes/*docs
examples and runt each of them, i noticed few things to fix:
Also, i noticed that when in a branch, hugo references files from the github repo, (master branch code) i wonder if its possible to make it reference current branch and local files.
Would it be ok to send a PR with the change to the middleware recipe ? also, what should we do for GAE and graceful ?
Once we release v3, master will be pointing to v3. Ideally, we should keep source code reference as is. With that there might not be much change in the docs. In short, we want to make sure all the code mentioned in the docs should be for v3, for instance, README.md
needs a complete revamp. I am sure there will be many changes in /docs/guide*
. May be you can take that too.
@vishr Oh, understood the v3 branch references to master, yes, i could cover the /guide/*
section.
I have a couple of questions i did not get answer from your last comment:
I'll start looking on the /guide*
and send appropriate PR. thanks for the chance to help.
@vishr sorry, I am busy for debug my app, which is written by echoV3 and gorm(https://github.com/jinzhu/gorm). @apaganobeleno If you have time now, you can help me to cover the /guide/*
section. Thank you.
@apaganobeleno
Should i send a PR with the wording chance on the v3 middleware recipe ?
Yes
What should we do regarding graceful doc? same for GAE ?
Graceful shutdown is now part of the core framework using graceful
lib. I am still not sure what to do with the recipes, probably we will keep both, however for graceful
just mention that it is part of the framework and update on how it works.
thanks @vishr, i'll give it a shot soon.
@vishr Thank you.
@vishr Regarding the graceful description would it be ok do tell something like:
Echo now ships with graceful server termination inside it, to accomplish it Echo uses
github.com/tylerb/graceful
library.By Default echo uses 15 seconds as shutdown timeout, giving 15 secs to open connections at the time the server starts to shut-down.
In order to change this default 15 seconds you could change the
ShutdownTimeout
property of your Echo instance as needed by doing something like:e := echo.New() ... e.ShutdownTimeout = 30 * time.Second ... if err := e.Start(":1323"); err != nil { e.Logger.Fatal(err) }
Looks good. For future just send me a PR and we can discuss over there.
@vishr sure, i just created this one https://github.com/labstack/echo/pull/688, LMK if its ok for you, IDK if maintainers section is ok.
@vishr are we ok to mark "Update recipes" > "Put back Gracful shutdown" task as done or it still needs more work ?
Making Echo as simple as it was in v1
fasthttp
support to keep the project simple, easy to maintain, aligned with standard library and compatible with community packages. As simple as it was in v1.Skipper
function to conditionally skip a middlewareMap
type as shorthand formap[string]interface{}
@ericmdantas @o1egl @CaptainCodeman @mtojek @Anon-Penguin @gavv @01walid @ipfans @matcornic
Open for discussion