Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Echo v3 #665

Closed
16 tasks done
vishr opened this issue Sep 22, 2016 · 64 comments
Closed
16 tasks done

Echo v3 #665

vishr opened this issue Sep 22, 2016 · 64 comments
Assignees
Milestone

Comments

@vishr
Copy link
Member

vishr commented Sep 22, 2016

Making Echo as simple as it was in v1

  • Drop 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 middleware
  • Update docs
  • Update recipes
    • Put back WebSocket
    • Put back Graceful shutdown
  • Release notes and migrating guide
  • Use standard lib for serving files Resumeable Static Content #471
  • Wrap standard handler
  • Wrap standard middleware
  • Auto TLS via https://letsencrypt.org
  • Map type as shorthand for map[string]interface{}
  • Built-in graceful shutdown
  • Embedding golang context can cause stack overflows #669 (comment)
  • Remove static middleware
  • and more...

@ericmdantas @o1egl @CaptainCodeman @mtojek @Anon-Penguin @gavv @01walid @ipfans @matcornic

Open for discussion

@vishr vishr added this to the v2.1 milestone Sep 22, 2016
@vishr vishr self-assigned this Sep 22, 2016
@mtojek
Copy link
Contributor

mtojek commented Sep 22, 2016

/cc @abador

@ericmdantas
Copy link
Contributor

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.

@vishr
Copy link
Member Author

vishr commented Sep 22, 2016

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).

@ericmdantas
Copy link
Contributor

I agree. In my opinion, in the long run maintainability is a better path to follow.

@CaptainCodeman
Copy link
Contributor

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.

@01walid
Copy link
Contributor

01walid commented Sep 22, 2016

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

@ericmdantas
Copy link
Contributor

Agreed, 3.0 makes more sense.

@vishr vishr changed the title Echo v2.1 Echo v3 Sep 22, 2016
vishr added a commit that referenced this issue Sep 23, 2016
Signed-off-by: Vishal Rana <[email protected]>
vishr added a commit that referenced this issue Sep 23, 2016
Signed-off-by: Vishal Rana <[email protected]>
@lugorians
Copy link

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!?

@vishr
Copy link
Member Author

vishr commented Sep 23, 2016

@lugorian Is it something similar to https://echo.labstack.com/guide/context?

@o1egl
Copy link
Contributor

o1egl commented Sep 23, 2016

I think that it's the right way. My opinion is that if someone really needs fasthttp they use it natively.

@lugorians
Copy link

lugorians commented Sep 23, 2016

@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?

@vishr
Copy link
Member Author

vishr commented Sep 26, 2016

@lugorian Will look into it soon.

@aarondl
Copy link

aarondl commented Sep 27, 2016

@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 :)

@deankarn
Copy link

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:

  1. You never step outside the using Context handlers
  2. The middleware does another shallow copy to update the callers http.Request object eg.
// 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! 😃

@vishr
Copy link
Member Author

vishr commented Sep 27, 2016

@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.

@vishr
Copy link
Member Author

vishr commented Sep 27, 2016

@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

@aarondl
Copy link

aarondl commented Sep 27, 2016

@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 :)

@tikiatua
Copy link

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).

@paganotoni
Copy link
Contributor

@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 ?

@vishr
Copy link
Member Author

vishr commented Oct 24, 2016

Yes.

@paganotoni
Copy link
Contributor

@vishr i just went through the recipes/*docs examples and runt each of them, i noticed few things to fix:

  • On the middleware recipe we should update wording to say Echo3.0 on the server header
  • Could not run GAE recipe (tried building the Dockerfile and it failed).
  • Don't see graceful shutdown recipe in the codebase.

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 ?

@vishr
Copy link
Member Author

vishr commented Oct 24, 2016

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.

@paganotoni
Copy link
Contributor

@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:

  • Should i send a PR with the wording chance on the v3 middleware recipe ?
  • What should we do regarding graceful doc? same for GAE ?

I'll start looking on the /guide* and send appropriate PR. thanks for the chance to help.

@bluealert
Copy link

@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.

@vishr
Copy link
Member Author

vishr commented Oct 24, 2016

@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.

@paganotoni
Copy link
Contributor

thanks @vishr, i'll give it a shot soon.

@freWalker
Copy link

@vishr Thank you.

@paganotoni
Copy link
Contributor

@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)
}

@vishr
Copy link
Member Author

vishr commented Oct 25, 2016

Looks good. For future just send me a PR and we can discuss over there.

@paganotoni
Copy link
Contributor

@vishr sure, i just created this one #688, LMK if its ok for you, IDK if maintainers section is ok.

@paganotoni
Copy link
Contributor

paganotoni commented Oct 25, 2016

@vishr are we ok to mark "Update recipes" > "Put back Gracful shutdown" task as done or it still needs more work ?

@vishr
Copy link
Member Author

vishr commented Oct 25, 2016

Just updated.

@DevotionGeo
Copy link

When is V-3 going to release?

@vishr
Copy link
Member Author

vishr commented Oct 29, 2016

Anyone up for updating the README.md for v3?

@paganotoni
Copy link
Contributor

paganotoni commented Oct 29, 2016

I could help but im out the city now, would it be ok starting next week?

@vishr
Copy link
Member Author

vishr commented Oct 29, 2016

@apaganobeleno Sure, also look into #667 PR.

@paganotoni
Copy link
Contributor

paganotoni commented Oct 30, 2016

@vishr just sent a PR (#698) that with #667 should cover most of the documentation update for v3, however i have a question regarding the benchmark, as you know it includes fasthttp comparison, should we add a different image that shows only v3 comparison ?

@vishr
Copy link
Member Author

vishr commented Nov 1, 2016

@apaganobeleno We will just have a benchmark chart based on Echo performance for various API set.

@vishr
Copy link
Member Author

vishr commented Nov 12, 2016

A new v2 release is cut from master branch and master now has v3, still not released yet but it makes it easier to develop on master.

@vishr
Copy link
Member Author

vishr commented Nov 12, 2016

@apaganobeleno I need help in updating the migrating guide and change log, please let me know if you are in. You can send me initial PR and we can take it from there.

PS: We will drop v1 mention from the migrating guide.

@paganotoni
Copy link
Contributor

@vishr i'm in 👍

@vishr
Copy link
Member Author

vishr commented Nov 14, 2016

@apaganobeleno Update: we still need to retain v1 to v2 migration.

@vishr
Copy link
Member Author

vishr commented Nov 15, 2016

Thanks everyone!!!

@vishr vishr closed this as completed Nov 15, 2016
@gavv
Copy link
Contributor

gavv commented Nov 15, 2016

It seems that fasthttp support was finally dropped, but Feature Overview in README still notes fasthttp:

Run with standard HTTP server or FastHTTP server

@nbpalomino
Copy link

Echo v3 on go-echo/echo fork is NOT in sync with main repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests