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

Provide a template registration callback #500

Closed

Conversation

seanlinsley
Copy link

This makes it possible to register helper functions with Handlebars and Tera, resolving #64.

I'm using this locally to update Crates.io to use server-side rendering: rust-lang/crates.io#1173.

That branch is out of date currently; here's how I'm using this feature:

// src/lib.rs

pub fn route(app: App) {
    rocket::ignite()
        .mount("/", routes![html::index])
        .attach(Template::fairing_with(|context| {
            html::register_helpers(&mut context.engines.handlebars);
        }))
        .launch();
}
// src/html/mod.rs

#[get("/")]
pub fn index() -> Template {
    let mut json = json!({});
    json["current_user"] = json!({"id": 1, "name": "Sean Linsley"});

    Template::render("index", &json)
}

pub fn register_helpers(handlebars: &mut Handlebars) {
    handlebars.register_helper("format_num", Box::new(format_num));
    handlebars.register_helper("embed_svg", Box::new(embed_svg));
}

There have been two previous PRs to try adding this feature: #60 and #364. @SergioBenitez has wanted the ability to register the same helper functions on both rendering engines simultaneously, but I'm not sure that's possible because of the library-specific code inside of the helper functions themselves.

Instead I opted to make the underlying template objects accessible via a callback passed to Template::fairing_with. Is there a better name for this function?

Once we're happy with this change, I'll update the documentation and write a test.

this makes it possible to register helper functions with Handlebars and Tera
@seanlinsley
Copy link
Author

Because the callback returns Context instead of Engines, it would also allow application developers to register templates from strings: #234. Though it's possible that other changes are needed for that to work.

@@ -15,6 +15,7 @@ use self::glob::glob;

use std::borrow::Cow;
use std::path::{Path, PathBuf};
use std::marker::{Send, Sync};
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, this isn't alphabetical

@SergioBenitez
Copy link
Member

This looks like the original implementation in #431. Perhaps you can finish off the contribution started there?

@seanlinsley
Copy link
Author

Oh, I didn't see that PR because it wasn't linked to the previous issues & PRs.

I haven't used Template::show before, so I'm not sure I understand why it needs to accept a closure.

@SergioBenitez
Copy link
Member

SergioBenitez commented Dec 15, 2017

Template::show() is used to render a template manually, usually for testing. The current implementation for Template::show() loads the requested template and template engine each time the function is called. As a result, any helpers or additional items attached to an engine via the Template fairing won't be present unless resupplied, and templates rendered with Template::show() will fail to render if they make use of an expected helper.

In #431, I describe a solution to this issue: #431 (comment). We can't accept any PR for this unless testing works as well. We also can't accept a PR for this unless there is documentation and new tests for the feature.

@seanlinsley
Copy link
Author

Refactoring Template::show probably deserves to be a separate PR. I'd be happy to open that.

One question: how do you get a Context from a Rocket object? Perhaps via fairings?

@SergioBenitez
Copy link
Member

SergioBenitez commented Dec 19, 2017

It needs to be in one PR, otherwise, you can't possibly test this new addition. You don't need to be able to retrieve anything additional from a Rocket instance. The Template code already does what is needed.

@seanlinsley
Copy link
Author

Your previous comment said to update Template::show to accept a Rocket instance, and to retrieve the already-loaded templates from it.

Being very unfamiliar with the code, I was thinking that something like this might work:

    pub fn show<S, C>(rocket: &Rocket, name: S, context: C) -> Option<String>
        where S: Into<Cow<'static, str>>, C: Serialize
    {
        let ctxt: Context = rocket.fairings.find(|fairing| fairing.info().name == "")?;

        Template::render(name, context).finalize(&ctxt).ok().map(|v| v.0)
    }

Though this appears to handle controller renders:

https://github.com/SergioBenitez/Rocket/blob/a9c66c9426bec57ee958480ae4aa3d789f20488f/contrib/src/templates/mod.rs#L268-L277

@seanlinsley
Copy link
Author

rocket.state should be the same object behind request.guard() if I understand correctly, but for some reason state is treated as private:

error[E0616]: field `state` of struct `rocket::Rocket` is private
   --> contrib/src/templates/mod.rs:243:20
    |
243 |         let ctxt = rocket.state.get::<Context>();
    |                    ^^^^^^^^^^^^

error: aborting due to previous error

even though it should be accessible in the crate, as it's defined with pub(crate):

pub struct Rocket {
    pub(crate) config: Config,
    router: Router,
    default_catchers: HashMap<u16, Catcher>,
    catchers: HashMap<u16, Catcher>,
    pub(crate) state: Container,
    fairings: Fairings,
}

@SergioBenitez
Copy link
Member

SergioBenitez commented Dec 27, 2017

@seanlinsley The implementation details of managed state should never be exposed to any outside crate. The contrib crate is not part of the core rocket crate and should be treated (and is) exactly as if it were a third-party crate.

I think it's fair (and necessary, here) to expose a method that allows retrieving managed state from a Rocket instance. The signature (and implementation) should be:

/// Retrieve the managed state of type `T`, if it exists.
/// 
/// # Example (...)
#[inline(always)]
pub fn state<T: Send + Sync + 'static>(&self) -> Option<&T> {
    self.state.try_get()
}

You can then call rocket.state::<Content>()? to retrieve the managed state from show().

@SergioBenitez SergioBenitez added the pr: closed This pull request was not merged label Dec 29, 2017
@seanlinsley seanlinsley deleted the template-registration-callback branch December 31, 2017 18:29
@seanlinsley
Copy link
Author

Great to hear that someone managed to finish this feature 😄 Thanks @jebrosen!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: closed This pull request was not merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants