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

implement context.Context #17

Open
donseba opened this issue Mar 17, 2016 · 6 comments
Open

implement context.Context #17

donseba opened this issue Mar 17, 2016 · 6 comments

Comments

@donseba
Copy link

donseba commented Mar 17, 2016

Little "question about" or "request for" context.Context
I know it is something vestigo is not, and probably will implement.

Since you have studied and written about the mainstream mux/routing tools in golang I'd like to know how to implement golang's context.Context or something similar into Vestigo..

This article is pretty clear on how to do it with the default http.Handler but i think it is not very idiomatic.

Would it be possible to do something like goji does ? And limit it to

  • func(http.ResponseWriter, *http.Request)
  • func(context.Context, http.ResponseWriter, *http.Request)

I have no idea if and how it would influence the performance.

@husobee
Copy link
Owner

husobee commented Mar 17, 2016

To be honest, the performance would be improved if we implemented a context passing function signature. I have been purposely leaving it out of vestigo for two reasons:

1.) I wanted to stay consistant with the standard lib http.Handler function signature
2.) Future golang versions will eventually support a built in net.Context within the http.Request object

When that future day happens, vestigo will be able to benefit directly, and will improve in performance.

In the mean time, I would suggest using a project like backdrop or gorilla context to allow yourself to access a context across middlewares/handlers.

@husobee
Copy link
Owner

husobee commented Apr 20, 2016

here is the code review for context in net/http/request.go for 1.7. When go 1.7 comes out, I feel like we should cut a new breaking version of vestigo that utilizes the net/context constructs in the new standard library.

@donseba
Copy link
Author

donseba commented Apr 20, 2016

Hi, thanks for the update!!! that looks really promising!! Can't wait to be able to use that!

@josephspurrier
Copy link
Contributor

Have you thought about using build constraints for 1.7 so you don't have to wait?

A good example:
https://github.com/gorilla/csrf/blob/master/context.go
https://github.com/gorilla/csrf/blob/master/context_legacy.go

@husobee
Copy link
Owner

husobee commented Aug 16, 2016

#41 <- initial attempt at using request context instead of url params

@josephspurrier
Copy link
Contributor

josephspurrier commented Aug 21, 2016

Cool! Looks good to me. I updated my project to use your Param function: blue-jay/blueprint@8e09812

My only suggestion is to move Param, ParamNames, and AddParam into their own files, away from the other code in common.go/common_legacy.go to reduce duplication. Maybe do param.go and param_legacy.go.

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

No branches or pull requests

3 participants