sunng87 / handlebars-iron

Handlebars middleware for Iron web framework
MIT License
119 stars 20 forks source link

[Suggestion] Implementing Handler for Templates #49

Closed AtheMathmo closed 7 years ago

AtheMathmo commented 7 years ago

Hey!

This library is awesome - thank you for your continued work on it.

I'm making this issue to ask if you think it is worth implementing Handler for Template? Currently I am doing something like this in my code:

pub struct TemplateHandler {
    path: &'static str
}

impl TemplateHandler {
    pub fn new(path: &'static str) -> TemplateHandler {
        TemplateHandler { path: path }
    }
}

impl Handler for TemplateHandler {
    fn handle(&self, _: &mut Request) -> IronResult<Response> {
        let mut resp = Response::new();
        resp.set_mut(Template::new(self.path, ())).set_mut(status::Ok);
        Ok(resp)
    }
}

// Mount the templating
mount.mount("/",
            router!(
    get "/" => TemplateHandler::new("index"),
    get "/kmeans" => TemplateHandler::new("kmeans"),
    get "/dbscan" => TemplateHandler::new("dbscan"),
));

Maybe I'm missing the point but I think it would be nice to provide this behavior within this library. I know there are many use cases that will not simply be rendering a web page without providing new content. But I think this is common enough that it is valuable.

An alternative could be to implement Handler for a new wrapper type as I have - perhaps called RenderTemplate?

In any case, it is not too much extra code to add on and the library is very useful as is!

sunng87 commented 7 years ago

If I understand correctly, you were using template as static pages?

I think that's a good idea. And if we can pass some default data into the page, for example, parameters, query strings, it would be stronger:

impl Handler for TemplateHandler {
    fn handle(&self, req: &mut Request) -> IronResult<Response> {
        let mut resp = Response::new();
        resp.set_mut(Template::new(self.path, parse_data_from_request(req))).set_mut(status::Ok);
        Ok(resp)
    }
}

WDYT?

AtheMathmo commented 7 years ago

Yes you're right - I am using templates as static pages (to keep the client side views tidy).

That is a good idea! Could we have the parse_data_from_request function be defined by the TemplateHandler? I'm not sure I have the knowledge to describe how that function should look.

sunng87 commented 7 years ago

Yes, the parse_data_from_request is a built-in function that extracts some data from Request.

AtheMathmo commented 7 years ago

But this is a function that we would define within this library?

I meant to ask whether it would be sensible to let the user define this function (parse_data_from_request) within the handler. I mean something like:

(Edit: Working version in edit at end.)

pub struct TemplateHandler<F, U : ToJson>
    where F: FnOnce(Request) -> U {
    path: &'static str,
    get_data: Box<T>,
}

And we could provide something like:

impl<F: FnOnce(Request) -> ()> TemplateHandler<F, ()> {
    pub fn new_static(path: &'static str) -> TemplateHandler<F, ()> {
        TemplateHandler {
            path: path,
            get_data: Box::new(|_| { () }),
        }
    }
}

Note that this doesn't actually compile - I'm sure there should be a way though...


Working version:

pub struct TemplateHandler<U : ToJson> {
    path: &'static str,
    get_data: Box<Fn(Request) -> U>,
}

impl TemplateHandler<()> {
    pub fn new_static(path: &'static str) -> TemplateHandler<()> {
        TemplateHandler {
            path: path,
            get_data: Box::new(|_| ()),
        }
    }
}

We could also provide some other defaults while allowing the user to construct their own.

Maybe this all just too complex though, let me know what you think :)

sunng87 commented 7 years ago

I think this is a little complex and it's getting similar to a normal iron handler.

My idea is to provide a standard get_data function to extract information from request and pass them to the template. But now I'm afraid this approach will lead handlebars template to early days of JSP and PHP. Users might tend to write their business logic into the template, which is a bad idea.

AtheMathmo commented 7 years ago

I can't comment on the JSP/PHP - I'm not much of a web developer - but I agree that it does look too complex.

What would the get_data function you're proposing look like? Perhaps it would still be a worthy addition to simply add a static template handler with the functionality of new_static above?

sunng87 commented 7 years ago

My idea is to make a map of all request parameters as template context. The parameters can be extracted by params using:

fn get_data(req: &Request) -> Map {
     req.get_ref::<Params>().unwrap();
}

If this works for you, I will make it a separated module because it introduces params, and encourages logic in template (which is not welcomed by some people)

AtheMathmo commented 7 years ago

That sounds good to me!

sunng87 commented 7 years ago

The lib is here:

https://github.com/sunng87/handlebars-template-handler

AtheMathmo commented 7 years ago

Nice work! Thank you. Feel free to close.