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

Add "insert_fn2()", which provides inserted function with a renderer callback #69

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ayourtch
Copy link

The mustache doc about lambdas at https://mustache.github.io/mustache.5.html says:

The text passed is the literal block, unrendered. {{tags}} will not have been expanded - the lambda should do that on its own. In this way you can implement filters or caching.

It also gives the example:

{
  "name": "Willy",
  "wrapped": function() {
    return function(text, render) {
      return "<b>" + render(text) + "</b>"
    }
  }
}

However, it is impossible to implement that with the way the logic of the inserted functions works: there is no simple way to render the template within the inserted function. one arguably can use the mustache crate within the function, and perform the call there, but it is a lot of boilerplate.

So I made this "insert_fn2()" helper which allows to specify a lambda with two parameters - the first one is the string being rendered, and the second is a renderer callback. When called with the string, it will render it within the current context - so the inserted function can manipulate both the template text pre-render as well as the post-render text.

To avoid an "surprise" vulnerability whereby the function renders the text and the results contain the separators, that can be interpreted again by the current rendering code, I added a flag which disables the post-function rendering if the callback was called with a non-empty parameter. (and enables if the parameter was empty - thus allowing to enable the double-render behavior where it may be beneficial.

With this patch, one can simply do:

mapbuilder.insert_fn2("MyFunc", |s, render| format!("<b>{}</b>", render(s)))

Thus allowing to easily achieve functionality mentioned in the documentation.

@ayourtch ayourtch requested a review from erickt February 21, 2020 11:50
@JokerQyou
Copy link

So this would enable us to create custom filters for templates to consume, am I right?

With your example

mapbuilder.insert_fn2("MyFunc", |s, render| format!("<b>{}</b>", render(s)))

We would be able to do this in a template:

{{#MyFunc}}
  {{raw_value}}
{{/MyFunc}}

I'm a very newbie in Rust, and would like to ask that, if raw_value is a struct, would MyFunc get the actual struct so that we can call its methods in MyFunc?

@ayourtch
Copy link
Author

That was exactly my use case - human readable presentation
is the date/time from the default - https://github.com/ayourtch/rsp10-blog/blob/fa028e46c6c47937047473b4712cea05158f9d16/src/bin/pages/blogview.rs#L161

(Note, the code in the pointed source is using a builder layer on top, with the same interface)

I don’t think it might be feasible to pass the structure itself, since it might be arbitrary string inbetween the two delimiters that refer to the function - even if possible, the type dependencies would be quite nasty.

But I had a somewhat similar problem and I solved it by just “descending into” the structure using the same tags - then you can have a partial that you can include, which refers the fields by just names - and it can be very nice illusion of polymorphism.

The example: https://github.com/ayourtch/rsp10/blob/3a9409d690a8912c6fc43151acf11740297795ee/templates/teststate.mustache#L33

Have you experimented with that approach ?

@JokerQyou
Copy link

Sorry but I did not understand this part:

I solved it by just “descending into” the structure using the same tags - then you can have a partial that you can include, which refers the fields by just names - and it can be very nice illusion of polymorphism.

I can see the template uses a custom filter, and is including another partial:

{{#btnTest}} {{> html/submit}} {{/btnTest}}

But I don't see how it's related to accessing raw struct object. Can you elaborate a little more?

@ayourtch
Copy link
Author

Inside the partial it accesses the fields of the structure.

I am saying trying to get the raw structure in is an idea that goes agains the philosophy of mustache (in my opinion), even barring the impracticality of implementation.

What is the reasoning behind wanting to get the entire structure rather than individual elements?

@JokerQyou
Copy link

JokerQyou commented Feb 24, 2020

If the struct can be accessed within MyFunc then it would be convenient to call one of its methods to get computed value.
We can always pre-compute the value so the template only does simple rendering. However sometimes this seems not necessary.

For example a simple struct BlogPost with a method summarize(&self) -> String. This method returns the first N characters of a specific post, and would make sure the summary ends with a full sentence.

struct BlogPost {
    content: String,
    author: Author,
}

impl BlogPost {
    fn summarize(&self) -> String {
        // Some magic here
    }
}

Now the author could turn off the summary, so the post only shows a title on the list page. But for posts of other authors we would like to still render the summary.
In the case of rendering the summary, I was thinking to use this:

{{#SmartSummary}}
{{.}}
{{/SmartSummary}}
mapbuilder.insert_fn2("SmartSummary", |s, render| render(s.summarize()))

instead of this:

{{#SmartSummary}}
{{.content}}
{{/SmartSummary}}
mapbuilder.insert_fn2("SmartSummary", |s, render| render({ /** Repeat .summarize() again here **/ }))

Of course, this seem to be against mustache's design philosophy - the logic-less template. Besides, now I get a rough image of the complexity of potential implementation. So maybe we really should pre-compute the values and not bring logic into templates.

@ayourtch
Copy link
Author

Yeah, "get some misc per-element data" is something I encountered, but I dealt with it this way:

#[derive(Debug, Default)]
struct Post {
    Author: String,
    Content: String,
}

#[derive(Debug, Default)]
struct PostProps {
    ShowSummary: bool,
    // anything calculated per-post goes here
}

#[derive(Debug, Default)]
struct PostData {
    post: Post,
    props: PostProps,
}

Then in the code you can do something along these lines:

   let mut posts: Vec<Post> = vec![];
   // fill the posts.
   posts.push( Post{ Author: "Alice".into(), ..Default::default()});
   posts.push( Post{ Author: "Bob".into(), ..Default::default()});
   println!("Posts: {:#?}", &posts);
   
   let postdata: Vec<PostData> = posts.into_iter().map(|p| PostData { props: PostProps { ShowSummary: &p.Author == "Alice"}, post: p } ).collect();
   
   println!("Posts with attributes: {:#?}", &postdata);
   map.insert_vec("posts", &postdata);

And this would be the template:

{{#posts}}<li>{{post.Author}} - 
    {{#props.ShowSummary}}{{#SmartSummary}}{{post.Content}}{{/SmartSummary}}{{/props.ShowSummary}}
</li>{{/posts}}

Hm, I guess I have just convinced myself the functionality from this PR can be done the same way... :-D Just compute the summaries rather than flags... But it feels like the String->String transform is useful to have there for "light" things. Maybe. :-)

@JokerQyou
Copy link

Well, surprisingly, I ended up using the exact same way: extend the struct into another struct container and pre-compute the required conditional values, then let the template do simple rendering. 😆

@ninjabear
Copy link
Collaborator

Hi @ayourtch @JokerQyou, honestly I'm having a hard time following along with you here! I can say that this isn't an often used piece of the mustache spec afaik.

It seems _2 is code smell and that we should refactor such that it is possible to comply with the spec naturally?

@ayourtch
Copy link
Author

ayourtch commented Mar 3, 2020

@ninjabear - we did drift a little :-)

The exact problem I am attempting to solve here is visible on the rust code running at https://blog.stdio.be/ where the timestamp of the post is printed as 2020-02-22T10:55:03.666018587 - not ideal for the human to consume, since most of the time we don't care about nanoseconds :-) (and having "T" doesn't help either).

So this PR arose as a (maybe misguided) attempt to be able to have a somewhat backwards compatible function signature vs insert_fn (with the argument being a string type, the unexpanded template within the mustache template same as in the existing) - and also the code within function can render the template, and massage the ugly-looking string into something that is more palatable.

That's where @JokerQyou mentioned it would be nice to have the argument not a string, but rather the initial data structure... To which I repllied that is probably impossible, and then we arrived to the same method which we already use - that is to "extend" each element of a collection with additional record that has the formatted-for-human-consumption values, and just reference those fields.
At which point I came to conclusion that this is kinda doable for the single fields, just ugly. So this is where I started arguing with myself whether this PR is even worthy of having :)

re. Refactoring to comply better with the spec: The mustache spec, the way I read it, in the part dealing with functions, really assumes the Javascript-isms. It captures the context, and thus a pointer to render() function into the closure without any additional parameters.
This is impossible to do in Rust IMO... rendering of the template usually happens quite far from the place where the insert_* functions are called, so there is no way to implicitly capturing it.

Not sure if this clarifies or confuses things more, but hope the former! :)

@ninjabear
Copy link
Collaborator

cheers @ayourtch that does make things a bit more straightforward. I think that does seem very much like a javascript-ism of the spec!

It seems the solution you both came to is the model view presenter pattern with rust-mustache being the "View". This seems better than entangling business logic with mustache, because you can easily switch it out for erb, jinja2 etc.

@ayourtch
Copy link
Author

ayourtch commented Mar 3, 2020

In my framework (github.com/ayourtch/rsp10), there is already a fairly strict split between the "logic" and "presentation" (and I think mustache does a fabulous job at that by constraining a lot what can be done at the "view" layer);

But how many digits to print and whether to print the "T" between date/time IMHO is firmly within the realm of the "View" - the underlying value is the same, just different presentations of it. So, to that extent, filling the "Model" with the "View" related functionality is also not ideal, and why I came up with this tweak - to have only one value within the data being rendered, and then have filters that can massage it depending on the place where it is used... (Thinking of it as a server side equivalent of iterating the

-wrapped values and tweaking them with something like https://momentjs.com/)

Anyway, I have this code in my own fork which I currently use within my framework, and in the next few months I will hopefully gain some more experience in terms of both usefulness and usability of this approach, and I can update this PR with my findings...

I am very happy with mustache (as well as many other rust crates) not changing dramatically without good considerations, so there's no rush on my side to get this accepted. I just wanted to flag this as something I spotted, and start the discussion to see if this maybe something of use to the broader community. Thanks a lot! :-)

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

Successfully merging this pull request may close these issues.

3 participants