wimdeblauwe / htmx-spring-boot

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

Polish partials to include 'main' fragment #21

Closed checketts closed 2 years ago

checketts commented 2 years ago

Side ideas, but not required for this change:

checketts commented 2 years ago

This is now feature complete, tested, and documented. (fyi @odrotbohm )

I want to ensure we consider #18 (dynamic HX-Triggers being returned). But I'm fine merging this and then changing the API if it makes sense. (Alternatively, I'm also OK if we want to hold off on merging this if you prefer @wimdeblauwe .)

wimdeblauwe commented 2 years ago

@checketts Thanks for advancing this! I just did a quick test to get a feel for the API and I noticed 2 things:

Pass fragment info as lambda

This is how my controller method ends up:

    @PostMapping
    @HxRequest
    public HtmxPartials htmxAddTodoItem(TodoItemFormData formData,
                                        Model model) {
        TodoItem item = repository.save(new TodoItem(formData.getTitle(), false));
        model.addAttribute("item", toDto(item));
        model.addAttribute("numberOfActiveItems", getNumberOfActiveItems());
        return new HtmxPartials()
                .main("fragments :: todoItem")
                .replace("active-items-count")
                .with("fragments :: active-items-count");
    }

Notice how it is quite difficult to see what attributes are there for the main fragments, and which ones are there for the OOB fragment.

How would you feel if the API made something like this possible:

    @PostMapping
    @HxRequest
    public HtmxPartials htmxAddTodoItem(TodoItemFormData formData,
                                        Model model) {
        TodoItem item = repository.save(new TodoItem(formData.getTitle(), false));
        model.addAttribute("item", toDto(item));

        return new HtmxPartials(model) // Add the model here so the `with` method can give it to the lambda
                .main("fragments :: todoItem")
                .replace("active-items-count")
                .with("fragments :: active-items-count", model -> model.addAttribute("numberOfActiveItems", getNumberOfActiveItems()));
    }

Another alternative: Consider this helper private method:

private String activeItemsCountFragment(Model model) {
  model.addAttribute("numberOfActiveItems", getNumberOfActiveItems());
  return "fragments :: active-items-count";
}

Then the controller method could be:

    @PostMapping
    @HxRequest
    public HtmxPartials htmxAddTodoItem(TodoItemFormData formData,
                                        Model model) {
        TodoItem item = repository.save(new TodoItem(formData.getTitle(), false));
        model.addAttribute("item", toDto(item));

        return new HtmxPartials(model) // Add the model here so the `with` method can give it to the lambda
                .main("fragments :: todoItem")
                .replace("active-items-count")
                .with(this::activeItemsCountFragment);
    }

Avoid nesting

My Thymeleaf fragment is defined like this:

<span th:fragment="active-items-count"
      id="active-items-count"
      class="todo-count">
        <th:block th:unless="${numberOfActiveItems == 1}">
            <span class="todo-count"><strong th:text="${numberOfActiveItems}">0</strong> items left</span>
        </th:block>
        <th:block th:if="${numberOfActiveItems == 1}">
            <span class="todo-count"><strong>1</strong> item left</span>
        </th:block>
</span>

The result after the OOB swap is this:

<div id="active-items-count" hx-swap-oob="true">
<span id="active-items-count" class="todo-count">
            <span class="todo-count"><strong>2</strong> items left</span>
</span>
</div>

Notice how my fragment is wrapped with that <div> and the id is duplicated. I wonder if we could place the hx-swap-oob="true" directly on the top level element of the fragment instead of wrapping it?

@odrotbohm Would love to hear your input on this as well.

odrotbohm commented 2 years ago

Regarding the model issue. I deliberately avoided getting the Model into play when setting up the partials for two reasons: first, there is no partial-specific model. Model attributes always apply to all partials. I didn't want to investigate additional partial specific model attributes as SpringMVC does a lot of stuff to it (@ModelAttribute on methods, potentially in @ControllerAdvice classes) and it felt like I'd introduce additional complexity for no good reason, because… second. The API was designed around the idea that the primary use case is a single template that would just declare individual parts to be exchangeable / amendable as fragments. That means, for the initial render, we would need the entire model to render the full page anyway. The HTMX partials part would basically optimize the calculation and rendering because the controller knows which parts of the screen need to be updated. Thus, it's very likely that the model serving an HTMX request is basically a subset of what's needed for the entire page to render, key-wise I mean.

Does it make sense to start with a rather constrained first draft of this and then wait for community feedback?

Regarding the nesting. It's been a while, but I was thinking HTMX would drop the wrapping element, but I just checked the docs, and it's not obvious to me how we could set that. A mixture of using hx-target and the different swap modes likely, although the latter seem to define where the content goes in respect of the target, not the source. Trying to alter the output of the fragment rendering output seemed to need additional interaction with the Thymeleaf API or even end users tweaking their template in some regard, which is why I started with the wrapping element in the first place (and because it's the way Hotwire does it, as I basically copied that support and adapted it 😬).

odrotbohm commented 2 years ago

Looking at this:

<span th:fragment="active-items-count"
      id="active-items-count" … >

Could we introduce an hx:fragment="…" that configures both a fragment and an id? The latter only if there's none defined already?

checketts commented 2 years ago

I wonder if we could place the hx-swap-oob="true" directly on the top level element of the fragment instead of wrapping it?

That would be ideal, but much more complicated. I have no idea where to start on that. (Perhaps wrapping the partial, the processing it a second time with a custom Thymeleaf processor that inspects its children?)

fragment info as lambda

I think treating the model as shared between fragments is a good first approach. One idea I want to play with is flagging partials as conditional based on the htmx trigger/fullpage headers. That way we could render the full page load on direct navigation, and partials when needed. However I suspect that code will be just as clear using HtmxRequest directly (and is an experiment for the future)

Today I'll be playing with my form submission/validation (since it returns multiple partials based on validation success) to see how this behaves in an application as well.

checketts commented 2 years ago

@wimdeblauwe I played with it and the duplicate ids aren't acceptable. However in discussing on the htmx discord, unwrapping doesn't seem to be supported natively by htmx. So I remove the wrapping logic.

I added a test the follows your TodoMvc example and will add a few more cases to walk through the other options.

By removing the wrapping, then it means the user will need to add hx-swap-oob to their templates directly, but that seems OK since templates are closely tied to the response.

    @PostMapping("/partials/add-todo")
    public HtmxPartials htmxAddTodoItem(TodoItem item, Model model) {
        model.addAttribute("item", item);
        model.addAttribute("itemCountSwap", "true"); //Manually need to control the hx-swap-oob
        model.addAttribute("numberOfActiveItems", todoRepository.getNumberOfActiveItems());
        return new HtmxPartials()
                .addTemplate("fragments :: todoItem")
                .addTemplate("fragments :: active-items-count");
    }

Template (note the hx:swap-oob="${itemCountSwap} ?: ''")

<span th:fragment="active-items-count"
      id="active-items-count"
      hx:swap-oob="${itemCountSwap} ?: ''" 
      class="todo-count">
        <th:block th:unless="${numberOfActiveItems == 1}">
            <span class="todo-count"><strong th:text="${numberOfActiveItems}">0</strong> items left</span>
        </th:block>
        <th:block th:if="${numberOfActiveItems == 1}">
            <span class="todo-count"><strong>1</strong> item left</span>
        </th:block>
</span>

This approach also addresses how the user can select other option for the hx-swap-oob since it supports quite a few choices.

checketts commented 2 years ago

This PR is ready for merging if you are OK with the changes @wimdeblauwe

wimdeblauwe commented 2 years ago

@odrotbohm I never intended my proposal to be that there would be a "separate" Model instance just for the fragment. I just wanted a simple way to be able to combine the attributes with the name of the partial template in a single method so it is clear in the code what attributes are intended for the partial template. But knowing anything you add to that model is shared with all templates added.

odrotbohm commented 2 years ago

Looks great so far! I am not quite sure I really like the need to put the swaps into the template manually. For one, they don't serve any purpose in a whole page rendering. Second, you now have to touch both the controller and the template to define how a fragment is supposed to be handled. So far, the template did not need to know anything about this except the fragment boundaries.

I guess HtmxResponse makes quite some sense, as it puts HtmxRequest into symmetry. That said, I guess we need quite a bit of Javadoc for the user-facing API elements as otherwise it's hard to understand what especially the annotations do and activate or cause, both on the server and in the client.

wimdeblauwe commented 2 years ago

https://github.com/wimdeblauwe/htmx-spring-boot-thymeleaf/pull/21#discussion_r875939383 -> true that there is only 1 response, but it does consist out of different parts. Thinking a bit more, I wonder if we won't run into strange things when combining responses that declare the same template or trigger?

wimdeblauwe commented 2 years ago

I guess HtmxResponse makes quite some sense, as it puts HtmxRequest into symmetry. That said, I guess we need quite a bit of Javadoc for the user-facing API elements as otherwise it's hard to understand what especially the annotations do and activate or cause, both on the server and in the client.

Yes, we should definitely start a manual. I could add something like I have done at https://wimdeblauwe.github.io/error-handling-spring-boot-starter/ (but that would be for after next week, kind of swamped right now)

checketts commented 2 years ago

Since the triggers are presented as a map, later trigger would clobber earlier ones, though the end result is that the trigger will still fire, so no harm there (unless the details were important). I can add a warning check to let the user know when they have overwritten a trigger.

I could do the same with templates being added. Instead of adding them to a List<String> I could make it a Set<String> so duplicates are merged.

In the end the user needs to do the right thing, but we could be defensive to avoid blowing up (with helpful error messages).

wimdeblauwe commented 2 years ago

Since the triggers are presented as a map, later trigger would clobber earlier ones, though the end result is that the trigger will still fire, so no harm there (unless the details were important). I can add a warning check to let the user know when they have overwritten a trigger.

I could do the same with templates being added. Instead of adding them to a List<String> I could make it a Set<String> so duplicates are merged.

In the end the user needs to do the right thing, but we could be defensive to avoid blowing up (with helpful error messages).

Seems like a good idea!

checketts commented 2 years ago

@wimdeblauwe I pushed the template and trigger merging and logging (and added some tests for that). Anything else?

wimdeblauwe commented 2 years ago

Looks good to me!

wimdeblauwe commented 2 years ago

FYI: I created an alternative version of todo-mvc using this OOB support. See https://github.com/wimdeblauwe/blog-example-code/tree/master/todomvc-htmx-oob

The main changes in TodoItemController:

1) I no longer have to inject the HttpServletResponse into the methods to set the trigger response header, which I think is great. 2) The re-use with the and(HtmxResponse) came in handy already. As I needed only the OOB fragment for the delete operation, but the combined main fragment of the todoitem + count update for the add and toggle operations. 3) The delete operation is a bit nicer now. In the version that uses client-side events, I needed to respond with an empty string there to get htmx to remove the html of the deleted item. Now that I just send the OOB fragment, this is no longer needed. It makes the code a bit cleaner for me.

With OOB, I also like the fact that the browser receives all updates in 1 request. Otherwise, it needs to first do a request, trigger the client event, trigger a new request to the server and finally update the item count html. We'll have to see with more usage I guess in what situations OOB is better and maybe in other situations event triggers are better.

checketts commented 2 years ago

Nice! I realize I had missed some of the other response headers. I'll add those in a separate PR.

Also I'll look into adding the main JavaDocs, at least for HtmxRequest and HtmxResponse, since those are the main user-facing APIs