wimdeblauwe / htmx-spring-boot

Spring Boot and Thymeleaf helpers for working with htmx
Apache License 2.0
521 stars 49 forks source link

Add static convenience methods to simplify creation of HtmxResponse.Builder #136

Closed xhaggi closed 3 weeks ago

xhaggi commented 4 weeks ago

I added some convenience methods to HtmxResponse to simplify the creation of the HtmxResponse.Builder in certain cases. Now you can write HtmxResponse.redirect("/redirect").build() instead of HtmxResponse.builder().redirect("/redirect").build().

xhaggi commented 3 weeks ago

@wimdeblauwe after using the builder in several projects, I find it a bit cumbersome. I had introduced the builder back then, only for the reason that HtmxRequest also had a builder. Now I think this was not such a good idea and would rather get rid of it and work directly with HtmxRequest / HtmxResponse, which makes things much easier. We could deprecate the builders and copy all builder methods directly to HtmxRequest / HtmxResponse. I don't see any breaking changes at the moment and we can safely remove the builder in 4.0. What do you think about this?

checketts commented 3 weeks ago

I second that approach and it aligns with the JTE usage I've been using where the builder is injected to the method and we make calls on it instead of the HtmxResponse (since it is immutable)

When deprecating the builder I would like to help make the JTE use case work with the changes.

wimdeblauwe commented 3 weeks ago

ok for me.

xhaggi commented 3 weeks ago

@wimdeblauwe should we support method chaining like the builder does? IMO this is not really necessary for HtmxRequest as we create it in the resolver. But what about HtmxResponse?

wimdeblauwe commented 3 weeks ago

Would it not be weird to have method chaining if it is not a builder? Wouldn't that just make it a builder without calling it a builder?

Thinking about this a bit more. If we remove the builder, then the code of my book will no longer work.

These are the ways I currently use it:

Return an empty response:

return HtmxResponse.builder().build(); 

Return a RedirectView:

            RedirectView redirectView = new RedirectView("/contacts");
            redirectView.setStatusCode(HttpStatus.SEE_OTHER);
            return HtmxResponse.builder()
                    .view(redirectView)
                    .build();

Return multiple views for OOB swaps:

        return HtmxResponse.builder()
                .view("index :: #overall-total")
                .view("index :: day-total")
                .build();

Maybe we can just leave in the builder, but undeprecate the non-builder methods? That way people can choose how they want to work?

xhaggi commented 3 weeks ago

Would it not be weird to have method chaining if it is not a builder? Wouldn't that just make it a builder without calling it a builder?

That was exactly my thought too. Then I would say we keep the builder and we can merge this MR to make it easier to use.

Let me also add HtmxResponse.empty() to cover your first use case.

xhaggi commented 3 weeks ago

After our discussion about dropping support for HtmxResponse as a return type, I think we should also drop the builder and make HtmxResponse mutable. Injecting the builder would then not be a viable option.

String user(HtmxResponse res) {
  res.trigger("mytrigger");
  res.refresh();

  return "myview"
}