sailfish icon indicating copy to clipboard operation
sailfish copied to clipboard

Option to disable destructuring of `self`

Open LordMZTE opened this issue 2 years ago • 8 comments

Sailfish currently starts the generated code off with let Foo { bar, baz } = self. While this is useful for simple templates, it can get in the way sometimes, because self has been moved.

This is annoying when, for example, I want to call a method on self, for example self.make_some_string().

We could fix this by adding a paremeter to the derive macro helper attribute, which either replaces the destructuring with let Foo { bar, baz } = &self (using a reference) or one that removes it entirely, so that fields need to be accessed using self.field in the template.

LordMZTE avatar Jan 16 '22 20:01 LordMZTE

I made a proof-of-concept implementation of the latter where fields need to be accessed with self in templates and you can call methods on self over at https://github.com/Svenskunganka/sailfish/tree/self_in_templates

I was worried that HTML escaping would be impacted, but it looks to be working fine. There is a drawback in the implementation though, the derived TemplateOnce has a mut self in render_once and render_once_to, see diff: https://github.com/Svenskunganka/sailfish/commit/03c3e425fccdc17363c1f0d7658ccd4ffd8438a3#diff-7b61aa77cead8aa5149bc31e98caeb632d11b27f0dd3a5ac556c7255f01bab09L262-R273

This is to allow calling mut methods in templates, for example:

#[derive(TemplateOnce)]
#[template(path = "template.stpl")]
struct MyTemplate {
    mutable: String,
}

impl MyTemplate {
    fn mutate(&mut self) {
        self.mutable = "Mutated".to_owned();
    }
}

fn main() {
    let template = MyTemplate { mutable: "Mutate me" };
    println!(template.render_once());
}

template.stpl:

<span><%= self.mutable %></span>
<% self.mutate(); %>
<span><%= self.mutable %></span>
<span>Mutate me</span>
<span>Mutated</span>

I don't know if this is a good idea. Directly mutating fields in templates might not be something users would do, but I can see users wanting to mutate fields in their implemented methods if those methods were allowed to be called from templates.
Since render_once consumes self, there can be no other references to the template anyway so the mut shouldn't be an issue and so far all tests pass and the examples work.

Being able to call self.render_once() inside a template will lead to fun times...

There's also the argument that template structs is not the place where functionality should be implemented and they should mostly be "ready to render" as soon as they're initialized, barring minor things such as trim or capitalize but there's filters for that.

@vthg2themax @jdrouet Thoughts?

Svenskunganka avatar Mar 11 '22 09:03 Svenskunganka

I agree that logic shouldn't be done inside of template structs, but the main use case for this I thought of would be a function that converts some data to a string and is too long to be nicely included in a template. Sometimes, storing the finished string in the struct might also work, but it's inefficient if the string is only included conditionally, or if the data the string is composed of is also needed separately. It could also allow for some deduplication in case there's a parameter involved.

LordMZTE avatar Mar 11 '22 13:03 LordMZTE

@Kogia-sima @jdrouet What do you think?

As for me, I think if it could potentially speed things up for the developer, while also not slowing down the rest of the project, that it sounds like a possible addition. I'm all for making developer productivity better, however, I also don't want to slow down this excellent framework in cases where people are just using it without that new feature. Let me know if you have any other thoughts on this @LordMZTE , or anyone else.

vthg2themax avatar Mar 21 '22 19:03 vthg2themax

We are currently looking for a new templating library for one of our projects, and sailfish seems a pretty good fit. We are however also generating one component of the resulting HTML ourself, since the rendering of that component is a bit more complex. Having access to self would allow us to just call the method generating that component in the template directly, which would simplify the code.

notuxic avatar Oct 31 '22 17:10 notuxic

We are currently looking for a new templating library for one of our projects, and sailfish seems a pretty good fit. We are however also generating one component of the resulting HTML ourself, since the rendering of that component is a bit more complex. Having access to self would allow us to just call the method generating that component in the template directly, which would simplify the code.

Can you give me an example of what it would look like in your project without this change, and with this change, just so I can get a better feel for how much it would help? I'm leaning towards inclusion of this though.

vthg2themax avatar Oct 31 '22 18:10 vthg2themax

Can you give me an example of what it would look like in your project without this change, and with this change, just so I can get a better feel for how much it would help? I'm leaning towards inclusion of this though.

Note that I have just started trying out the library. But as far as I can tell, you currently can't call a function that takes a self-parameter, since struct fields have been moved. A simple example of our case would be:

struct Foo {
 bar: Vec<SomeOtherType>,
 baz: String,
 // [...] plus some other fields
}

impl Foo {
fn some_complex_custom_html_generation(&self) -> String {
 // [...] do some custom html rendering
  }
}
<div>
<h2><%= baz %></h2>
<%- self.some_complex_custom_html_generation() %>
</div>

As far as I can tell, currently I'd have to remove the &self and pass all struct fields I want to use as parameters instead:

fn some_complex_custom_html_generation(bar: Vec<SomeOtherType>, /* some more struct fields */) -> String
<%- some_complex_custom_html_generation(bar, /* some other struct fields */) %>

This also means, that in our case a destructuring on a &self instead of a self should be sufficient.

notuxic avatar Oct 31 '22 20:10 notuxic

Can you give me an example of what it would look like in your project without this change, and with this change, just so I can get a better feel for how much it would help? I'm leaning towards inclusion of this though.

Note that I have just started trying out the library. But as far as I can tell, you currently can't call a function that takes a self-parameter, since struct fields have been moved. A simple example of our case would be:

struct Foo {
 bar: Vec<SomeOtherType>,
 baz: String,
 // [...] plus some other fields
}

impl Foo {
fn some_complex_custom_html_generation(&self) -> String {
 // [...] do some custom html rendering
  }
}
<div>
<h2><%= baz %></h2>
<%- self.some_complex_custom_html_generation() %>
</div>

As far as I can tell, currently I'd have to remove the &self and pass all struct fields I want to use as parameters instead:

fn some_complex_custom_html_generation(bar: Vec<SomeOtherType>, /* some more struct fields */) -> String
<%- some_complex_custom_html_generation(bar, /* some other struct fields */) %>

This also means, that in our case a destructuring on a &self instead of a self should be sufficient.

Okay, I will get this merge done in the next day. Thank you for your response!

vthg2themax avatar Oct 31 '22 20:10 vthg2themax

@notuxic @LordMZTE It looks like this change will require code to be written, and will take some time yet. Sorry for the misunderstanding.

vthg2themax avatar Nov 04 '22 20:11 vthg2themax