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

Publicly explain rationale behind response to #1119 #1128

Open
couchand opened this issue Feb 19, 2025 · 8 comments
Open

Publicly explain rationale behind response to #1119 #1128

couchand opened this issue Feb 19, 2025 · 8 comments

Comments

@couchand
Copy link
Contributor

Greetings! Longtime Askama user, occasional contributor, confused by all of the recent changes, just trying to get my code working.

I'm trying to sort out what updates need to be made to get various Askama-using projects running on Axum 0.8. I was surprised to find that the askama_axum crate has been deprecated. So I followed the trail and discovered #1119, where @joshka generously put a fair amount of effort into clearly describing the value that such a feature provides.

But @Kijewski can't even be bothered to reply more than two words before closing the issue? I understand you're busy and maintaining open-source projects is a nonstop burden, but seriously:

Respectfully, no.

is all you can muster? Please do your users the benefit of replying in full, or don't bother replying at all. Maybe yet another fork is in order, if this is the approach that current leadership is taking.

@Kijewski
Copy link
Collaborator

Kijewski commented Feb 19, 2025

Immediately after opening the question, Joshka posted the link into five threads:

In this moment I saw this behavior as harassment. I tend to not interact with people I cannot stand, so I closed the issue and hid the following comments. Was this the best way to react? Possibly not, but I won't stand any accusations like "is [that] all you can muster". Be kind.

I ask you the same question I asked Joshka, to which they never answered btw: Is anything in frameworks.html / upgrading.html unclear, and how can it be made better? I'd be happy if you could read the documentation and maybe open PRs to enhance it.

Please see #1035 (comment) for the reasoning for the upcoming changes.

@couchand
Copy link
Contributor Author

Thanks for sharing links to provide more information, more information is often helpful when trying to coordinate groups of people. Just curious if you have a reply to this part of @joshka's most recent comment on that issue:

For each of these projects every single usage of the template will now have to have extra code to convert from the template into a response for the specific framework. This code is non-differentiated and has to be added to every handler in the project if it is not built in to Askama.

It's not that the amount of code is difficult to add that is the problem here, it's that that code has to be added everywhere.

That seems like a pretty compelling reason to try to figure out a way to make it work, right? I mean, @joshka even volunteered to do the legwork here.

I ask you the same question I asked Joshka, to which they never answered btw: Is anything in frameworks.html / upgrading.html unclear, and how can it be made better?

It wasn't until about twenty minutes ago that I found out that Askama is deprecated and Rinja is the new Askama. So perhaps you can forgive me for not looking over there?

Is this the section you're referring to?

The integration crates were removed. Instead of depending on e.g. askama_axum / rinja_axum, please use template.render() to render to a Result<String, askama::Error>.

Use e.g. .map_err(|err| err.into_io_error())? if your web-framework expects std::io::Errors, or err.into_box() if it expects Box<dyn std::error::Error + Send + Sync>.

Please read the documentation of your web-framework how to turn a String into a web-response.

How can this documentation be made better? By improving the the process. The only thing I think of when reading these docs is "can't this user experience be made better so the docs don't have to be?"

@joshka
Copy link

joshka commented Feb 20, 2025

In this moment I saw this behavior as harassment.

The intention was not to harass. I can see how this might have come across that way however and for that I apologize.

My intent in putting comments on each issue / PR was to close loops to ensure that people watching those issues would be notified that there's some related discussion to parts that were meaningful. It was also to point out the problems concretely in the places where the problems that removing the integration occurred in practice. The purpose of #1119 was to foster some discussion of this which was noticeably absent / not well communicated. Closing it with "Respectfully, no" made me feel incredibly disrespected.

There's more discussion on this in the axum discord channel:
https://discord.com/channels/500028886025895936/1328130432927006783

The need for this feature comes from wanting to be able to test the data that a handler returns rather than the output serialized into a response of the return value. Axum handlers return IntoResponse, not Response. Testing the actual return value rather than the serialized value makes testing simple. Removing the integration part makes it more difficult.

Documentation is not a good fix for this.

The best I can come up with that keeps the things which were possible in Askama 0.12 is to add a wrapper struct and use that everywhere where you would previously just return the template.

pub struct HtmlTemplate<T> {
    template: T,
}

impl<T: Template> IntoResponse for HtmlTemplate<T> {
    fn into_response(self) -> Response {
        match self.template.render() {
            Ok(html) => Html(html).into_response(),
            Err(err) => {
                error!("Failed to render template: {err:?}");
                StatusCode::INTERNAL_SERVER_ERROR.into_response()
            }
        }
    }
}

impl<T: Template> From<T> for HtmlTemplate<T> {
    fn from(template: T) -> Self {
        HtmlTemplate { template }
    }
}

pub async fn index<T: Todos>(todos: State<T>) -> Result<HtmlTemplate<Index>> {
    let todos = todos.all().await?;
    Ok(Index::new(todos, Filter::All).into())
}

#[tokio::test]
async fn test_index() {
    let todo = db::Todo {
        id: 1,
        text: "hello".to_string(),
        completed: false,
    };
    let todos = vec![todo.clone()];
    let mut mock = MockTodos::new();
    mock.expect_all().once().return_once(|| Ok(todos));

    let response = super::index(State(mock)).await.unwrap();

    assert_eq!(response.template.todos, vec![todo]);
}

I suppose it could also be possible to create another derive macro that implements IntoResponse for each template and derive that too. But effectively that was askama_axum.

@joshka
Copy link

joshka commented Feb 20, 2025

I just published askama-derive-axum, which implements an IntoResponse derive macro which replaces the askama_axum crate.

use askama::Template;
use askama_derive_axum::IntoResponse;

#[derive(Template, IntoResponse)]
#[template(path = "index.html")]
struct IndexTemplate {
    title: String,
    body: String,
}

async fn index() -> IndexTemplate {
    IndexTemplate {
        title: "My Blog".to_string(),
        body: "Hello, world!".to_string(),
    }
}

I ask you the same question I asked Joshka, to which they never answered btw: Is anything in frameworks.html / upgrading.html unclear, and how can it be made better? I'd be happy if you could read the documentation and maybe open PRs to enhance it.

The current behavior allowed template rendering to be performed as a crosscutting concern without extra code, and made it possible to unit tests handlers against concrete template types instead of against responses. The documentation links above only show how to workaround the changes for a simplistic use case which doesn't align to how people use Askama currently. The documents could be improved by

  1. Explaining the reason for removing the behavior
  2. Explaining how to properly work around the no longer available approaches (i.e. any of the above mentioned workarounds)

@Kijewski
Copy link
Collaborator

Kijewski commented Feb 20, 2025

The intention was not to harass. I can see how this might have come across that way however and for that I apologize. […] Closing it with "Respectfully, no" made me feel incredibly disrespected.

I guess if I said more clearly (in more than 2 words) why I wasn't happy with the interaction, things would have gone better. Sorry for that!

Rather than maintaining n crates, 1 for every framework, I would rather like something similar to https://github.com/Kijewski/rinja_frameworks. I stole your idea to make "IntoResponse" a new derive item. :) That makes it much simpler to not work in lock-step with future askama point releases.

Right now, the repo does not have any documentation or ci tests, and only axum and actix-web are implemented. I would invite you, @joshka, to co-maintain the crate if you like to. (Of course, if you would rather keep maintaining your crate, then that's perfectly fine with me, too!) And if @GuillaumeGomez likes the idea to implement the framework integration this way too, I would move it into this org.

In any case, I would keep the frameworks section of the book, only mentioning the new crate a possible alternative. Everyone has a different workflow, and I myself would most likely not use the integration crate. In most cases my templates are more likely to contain an &str than a String, commit() is only called if the the template could be rendered correctly, etc.

@joshka
Copy link

joshka commented Feb 21, 2025

I guess if I said more clearly (in more than 2 words) why I wasn't happy with the interaction, things would have gone better. Sorry for that!

Thanks, I appreciate your apology. Water under the bridge.

Rather than maintaining n crates, 1 for every framework, I would rather like something similar to https://github.com/Kijewski/rinja_frameworks. I stole your idea to make "IntoResponse" a new derive item. :) That makes it much simpler to not work in lock-step with future askama point releases.

I encourage the stealing here. I'm ambivalent to whether the end result of this ends up as a single or multiple crates (will happily drop the above crate if there's a good alternative that comes out of it). IntoResponse is an axum concept though, so it may not make sense to use this as the cross framework name. Each framework uses something similar (actix => Reply, warp => Responder, rocket => Responder), so this sort of suggests idiomatically that these would be different names. I think askama-web / rinja-web might make a good name for that if you're looking for an alternative that captures the intent a bit better than -frameworks. Something where you end up with an import something like use askama_web::axum::IntoResponse or use askama_web::actix::Reply seems like it could be an ergonomic approach to me.

Right now, the repo does not have any documentation or ci tests, and only axum and actix-web are implemented. I would invite you, @joshka, to co-maintain the crate if you like to. (Of course, if you would rather keep maintaining your crate, then that's perfectly fine with me, too!) And if @GuillaumeGomez likes the idea to implement the framework integration this way too, I would move it into this org.

There's quite a bit more complexity in the implementation than I'd imagined would be necessary (double crate, proc macros wrapping declarative macros), and I haven't fully jumped in to to understand what problems the implementation solves - perhaps you can share some insight into the design? It's highly likely my naive implementation has missed those cases. Happy to work with you on something and deprecate mine when there's a good replacement. I suspect there's some probably some issues with the way I'm importing from the proc macro.

In any case, I would keep the frameworks section of the book, only mentioning the new crate a possible alternative. Everyone has a different workflow, and I myself would most likely not use the integration crate. In most cases my templates are more likely to contain an &str than a String, commit() is only called if the the template could be rendered correctly, etc.

I think it makes sense to provide a few canonical examples here to provide for users who learn top down from examples rather than bottom up from the implementation details. A little documentation now saves a bunch of issues later.

@Kijewski
Copy link
Collaborator

Kijewski commented Feb 21, 2025

I released a first version (mostly to secure the name) as askama_web v0.0.1-pre.0. I implemented some of your suggestions (thank you!), and will try to answer your questions early next week.

@joshka
Copy link

joshka commented Feb 21, 2025

Cool - I'm going to move continue discussion over in an issue in https://github.com/Kijewski/askama_web/ for better context. Kijewski/askama_web#1

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

No branches or pull requests

3 participants