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

support for context.Render #21

Closed
prabirshrestha opened this issue Apr 4, 2015 · 21 comments
Closed

support for context.Render #21

prabirshrestha opened this issue Apr 4, 2015 · 21 comments

Comments

@prabirshrestha
Copy link

Would be awesome if there was a Render function in context similar to rendering JSON.

Having this RendererFunc interface would allow to use any renderer. Would also be good to have html/template as a default Render. This is similar to how http.Handler and http.HandlerFunc is implemented.

type Renderer interface {
    Render(w http.ResponseWriter, r *http.Request, status int, name string, data interface{})
}

type RendererFunc (w http.ResponseWriter, r *http.Request, status int, name string, data interface{})

func (f RendererFunc) Render(w http.ResponseWriter, r *http.Request, status int, name string, data interface{})

gin use ... interface{} for data. Also should we also pass *http.Request to Render so that it can support content negotiation? If we pass request as nil then we can tell the render to not use content negotiation. Not sure if this is too much of magic. I think content negotiation should be a separate topic.

Here is sample api that could be possible to use. Thoughts?

var e := echo.New()
e.Get("/", func (c *e.Context) {
    c.Render(http.StatusOK, "home", nil)
});
@vishr
Copy link
Member

vishr commented Apr 5, 2015

I was planning to use c.Render(code int, i interface{}) to select one of JSON, XML ... based on content negotiation and default to html. Somehow I not able to understand how interface would help here as I think these bind/render are just helper functions. If someone wants to use there own renderer or binder its just so simple to write one, although we should have most of them.

I also plan to do opposite in c.Bind, so you can directly bind from JSON, XML ... to boost speed by not looking into content-type.

@vishr
Copy link
Member

vishr commented Apr 5, 2015

9d44f49

@prabirshrestha
Copy link
Author

@vishr I'm not sure auto content negotiation would be ideal.

I was sort of expecting Render to render based on the RendererFunc (view engine). My above Render and RenderFunc could had been over-enginnered. RenderFunc would be more than enough.

c.Render(200, "person", &person{fname: "...", lname: ""})

RenderFunc can then read the template in view/person.html and pass the person model. This would be the default Render.

Then anyone can go and override the RendererFunc if they want to base on their needs.

import  "github.com/unrolled/render"

r := render.New()

var e := echo.New()
e.RenderFunc = func(rw http.ResponseWriter, req *http.Request, statusCode int, name string, data interface{}) {
     r.HTML(rw, statusCode, name, data)
}

e.Get("/", func(c *echo.Context) {
    c.Render(200, "persons", &person{fname:"..", lname:".."})
})

@vishr
Copy link
Member

vishr commented Apr 6, 2015

In the above example it looks like e.RenderFunc is global, wouldn't it be a problem if I want to mix and match different renders?

@prabirshrestha
Copy link
Author

e.RenderFunc is per echo instance - e rather than global echo.

This was sort of what I was thinking of.

e.RenderFunc = func(rw http.ResponseWriter, req *http.Request, statusCode int, name string, data interface{}) {
     if  strings.HasSuffix(name, ".md") {
         r.Markdown(....)
     } else {
         r.HTML(....)
     }   
}

Here is how one could render.

e.Get("/", func (c *echo.Context) {
    c.Render(200, "index", nil)
})

e.Get("/doc", func (e *echo.Context) {
    c.Render(200, "doc.md", nil)
})

@vishr
Copy link
Member

vishr commented Apr 6, 2015

RenderFunc will be be serving all kind of formats. Now I was thinking how do we identify JSON or XML in the RenderFunc.

@prabirshrestha
Copy link
Author

That is where the req *http.Request parameter comes in. The RenderFunc can look at the headers and do the negotiation. But I think this is a bad idea.

I was thinking that RenderFunc should only render html style contents and not json, xml, protobuf. That should be a different api. Most likely NegotiateFunc and c.Negotiate

The reason is the difference in api. Notice that render takes the name while Negotiate doesn't. (Also I need to play more with the negotiate api to make it better)

c.Render(200, "doc.md", &model{})
c.Negotiate(200, &model{})

Some references:

gin-gonic/gin#59
gohttp/response#4
https://github.com/NancyFx/Nancy/wiki/Content-Negotiation

@prabirshrestha
Copy link
Author

Also if you want to mix Render and Negotiate I think NancyFx does it in an interesting way. This allows the devs to switch models based on the content negotiation. (Need to find an idiomatic way to do this in go)

Here is an example in c#.

Get["/"] = parameters => {
    return Negotiate
        .WithModel(new RatPack {FirstName = "Nancy "})
        .WithMediaRangeModel("text/html", new RatPack {FirstName = "Nancy fancy pants"})
        .WithView("negotiatedview")
        .WithHeader("X-Custom", "SomeValue");
};

@vishr
Copy link
Member

vishr commented Apr 6, 2015

Just thinking out loud: In c.Render(200, "doc.md", &model{}) or c.Render(200, "doc.html", &model{}) since we already know that its html or md, can't we just do c.Markdown() or c.HTML() without hitting the function?

@prabirshrestha
Copy link
Author

For c.HTML I expect a html text something like <div>Prabir Shrestha</div>.

c.HTML(200, `<div>Prabir Shrestha</div>`)

But for c.Render I expect a template text something like <div>{{firstName}} {{lastName}}</div>

c.Render(200, `<div>{{firstName}} {{lastName}}</div>`, &Person{"Prabir", "Shrestha"})

And it is the responsibitliy of the RenderFunc to convert the template and the model to html text that could be used by c.HTML().

Also I don't think having c.Markdown() is good idea to put in the core fwk. Then next time someone comes up with a new spec they will want to put it in there. c.XFormat(). I think echo should provide a solid foundation to build on top off.

Let says someone decides to use handlebars instead of the default template and they already have a bunch of existing templates. That person would want to easily update the RenderFunc to use a handlerbars for new one while using default templates for the existing ones. c.Render(200, "doc.hbs", nil) or c.Render(200, "doc.html", nil). This gives the user flexibility. My above markdown example could had been a bad sample to begin with.

@vishr
Copy link
Member

vishr commented Apr 7, 2015

So primarily this render function is for templates and html. Can you think of any other types? I think c.Render implies any kind of data be it JSON, XML, HTML or any other type. Do you think we shoujld call it TemplateFunc and c.Template? Let me know your thoughts and we can discuss it further.

@prabirshrestha
Copy link
Author

Yes. It is only for templates and html.

Render is a pretty common terminology in the MVC/web world. So I think it suits better. c.Template sounds awkward for me.

Samples for c.Render

Samples for c.View

I would prefer either c.Render or c.View but not c.Template because I would consider the .html, .hbs, .md as templates but the rendered template I would considered it as view. I would still prefer c.Render over c.View as it will read as render a view called index.html.

@vishr
Copy link
Member

vishr commented Apr 7, 2015

Thanks for the detailed description and links. I will have a look at them and come up with an API to discuss further.

@vishr
Copy link
Member

vishr commented Apr 7, 2015

@prabirshrestha This is what I have thought so far. What do you think?

type RenderFunc     func(io.Writer, string, interface{}) error

// Default RenderFunc
Echo.renderFunc: func(w io.Writer, name string, data interface{}) (err error) {
    // Implement golang template/html and write it to w.
    return
}

// Or provide your own RenderFunc?

func (c *Context) Render(code int, name string, data interface{}) error {
    c.Response.Header().Set(HeaderContentType, MIMEHTML+"; charset=utf-8")
    c.Response.WriteHeader(code)
    return c.echo.renderFunc(c.Response.ResponseWriter, name, data)
}
// API call
c.Render(200, "user.tmpl", &Users{"Joe"})

@prabirshrestha
Copy link
Author

Looks awesome.

Not related to this but would be awesome to have. Most likely a different issue. Since we now have a bunch of functions like HTML, JSON and render returning error. It would be awesome to support func serveXYZ(w http.ResponseWriter, r *http.Request) error { ... } in wraphH.

Source: https://sourcegraph.com/blog/google-io-2014-building-sourcegraph-a-large-scale-code-search-engine-in-go

Handler functions: We define our handlers with an error return value, and we use a simple wrapper function to make them implement http.Handler. This means we can centralize error handling instead of having to format error messages and pass them to the http.Error for each possible error. Our handler functions look like:

func serveXYZ(w http.ResponseWriter, r *http.Request) error { ... }

@prabirshrestha
Copy link
Author

RenderFunc seems to be private in Echo.

renderFunc      RenderFunc

@vishr
Copy link
Member

vishr commented Apr 8, 2015

There is a function to set custom RenderFunc now 12bd049

@vishr
Copy link
Member

vishr commented Apr 9, 2015

@prabirshrestha I am planning to change RenderFunc to Renderer interface{} it's more idiomatic to Go.

type Renderer  interface {
        Render(io.Writer, string, interface{})
}

func (c *Context) Render(code int, name string, data interface{}) error {
    c.Response.Header().Set(HeaderContentType, MIMEHTML+"; charset=utf-8")
    c.Response.WriteHeader(code)
    return c.echo.renderer.Render(c.Response.ResponseWriter, name, data)
}

// API call
c.Render(200, "user.tmpl", &Users{"Joe"})

@prabirshrestha
Copy link
Author

LGTM

vishr added a commit that referenced this issue Apr 9, 2015
Signed-off-by: Vishal Rana <vr@labstack.com>
@prabirshrestha
Copy link
Author

@vishr Seems like you removed the status code. Can we have it back? Useful when rendering errors.

func (c *Context) Render(name string, data interface{}) error {...}

Also if I pass status code 0 I think it shouldn't write the head. Same for other methods like JSON that writes.

func (c *Context) Render(statusCode int, name string, data interface{}) error {
    if c.echo.renderer == nil {
        return ErrNoRenderer
    }
    if statusCode != 0 {
        c.Response.Header().Set(HeaderContentType, MIMEHTML+"; charset=utf-8")
        c.Response.WriteHeader(statusCode)
    }
    return c.echo.renderer.Render(c.Response, name, data)
}

@vishr
Copy link
Member

vishr commented Apr 18, 2015

@prabirshrestha I have put it back - I completely missed on rendering errors. Verifying status code is nice but I then I think we should be doing it for all invalid codes - which is a lot of work, I believe, lets assume users are adults 😉

I looked into the URL you posted about handlers returning errors and I liked the idea. Now the default HandlerFunc returns error. I also added handlers which returns error to the list. 381fbae

@vishr vishr closed this as completed Apr 18, 2015
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

2 participants