wimdeblauwe / htmx-spring-boot

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

Rethink annotation support for htmx response headers #81

Closed wimdeblauwe closed 8 months ago

wimdeblauwe commented 9 months ago

This issue is triggered by the discussion at https://github.com/wimdeblauwe/htmx-spring-boot/issues/79

I guess for the server-side implementation, we have to make up our minds at some point how to deal with the fact that HTMX has request and response triggers. The current @HxTrigger seems to cater to the latter, which, I think, is a bit odd as controller method annotations are primarily metadata to map the incoming request.

Trying to summarize the proposals:

Have single request and response related annotation

Positive:

Negative:

Drop the annotations for response headers

We could just support returning a HtmxResponse object where the response headers are set and no longer offer the annotations. This will also make it clear that annotations are only for mapping the incoming request.

Negative:

Have individual response headers

If we do this, we should all start them with @ResponseHx... to make it clear that they are response headers.

I think we can do this kind of breaking change in a new 4.0 release no problem. We just have to think hard what is the best way forward. There is no clear winner for me, so all feedback is welcome.

wimdeblauwe commented 9 months ago

I had a look to some other libraries in other programming languages, but none of them seem to use annotations as far as I can tell.

xhaggi commented 9 months ago

If we stick to supporting annotations for htmx's response headers, I would prefer the @HxResponse annotation approach, since most header values are string-based and we don't need separate annotations for them. This makes things much easier, IMO. However, if we are going to do this for 4.0, I would prefer to release 3.1 after merging #80 and start with the new major version.

wimdeblauwe commented 9 months ago

My preference also goes to either @HxResponse or just drop the annotation all together. But we can keep the annotation for people that want it I guess. I'd love to hear from the other contributors (@checketts @odrotbohm @dsyer) before we take a final decision.

checketts commented 9 months ago

I prefer the @HxResponse approach as well. I don't use annotations in responses though.

dsyer commented 9 months ago

I like an annotation. It would be good if there was one that could automatically push the current URL (including query params). For that reason if no other I slightly prefer an annotation specially for that and therefore a kitchen sink @HxResponse wouldn’t be my choice.

FWIW I don’t like the annotation names that start with @Response. What are the request-specific use cases? Maybe if we enumerate them we could make a better decision?

wimdeblauwe commented 9 months ago

I tried to summarize the things here if I understood your question correctly @dsyer

Request use cases

So currently for htmx request headers, there is the@HxRequest annotation and the HtmxRequest class.

Response use cases

There are currently the following annotations defined corresponding to response headers that htmx will use:

wimdeblauwe commented 9 months ago

FWIW I don’t like the annotation names that start with @Response

There is @ResponseBody and @ResponseStatus of course already in the Spring framework.

We could also just place the annotations in a response and request package, but it would probably still be a bit confusing.

xhaggi commented 9 months ago

We could also just place the annotations in a response and request package, but it would probably still be a bit confusing.

-1 from my side.

xhaggi commented 9 months ago

One thing that just crossed my mind, what do you think if we rename the annotation @HxRequest to @HtmxRequestMapping, similar to Spring's @RequestMapping. And @HxResponse could then be called @HtmxResponseHeaders or if we don't decide for that just @HtmxResponseTrigger, @HtmxResponseRefresh, @HtmxResponseLocation etc.

xhaggi commented 8 months ago

I would like to make a decision here. Should we go with one annotation @HxResponse, one per response header option, or should we drop the annotation support completely?

However, we should keep in mind that most headers support complex values, e.g. HX-Trigger, HX-Reswap, HX-Location etc., and if we want to fully support all of them, it would lead to something like this.

@HxResponse(
  triggers = { @HxTrigger("event1"), @HxTrigger(name = "event2", detail = "{json string}") },
  triggersAfterSwap = @HxTrigger("event_after_swap"),
  reswap = @HxReswap(type = INNER_HTML, swapMs = 100, ...),
  location = @HxLocation(path = "/xyz", source = "my-source", ...)
  pushUrl = "https://www.example.com",
  replaceUrl = "https://www.example.com",
  reselect = "#target",
  refresh = true)
public String htmxView() {
  return "view";
}

vs.

@HxTrigger("event1")
@HxTrigger(name = "event2", detail = "{json string}")
@HxTriggerAfterSwap("event_after_swap")
@HxReswap(type = HxSwapType.INNER_HTML, swapMs = 100, ...)
@HxLocation(path = "/xyz", source = "my-source", ...)
@HxPushUrl("https://www.example.com")
@HxReplaceUrl("https://www.example.com")
@HxReselect("#target")
@HxRefresh
public String htmxView() {
  return "view";
}

BTW I have already done most of the work in #67 and actually only need a decision.

odrotbohm commented 8 months ago

The Spring approach (in contrast to the JakartaEE world) has been to work more with annotation attributes than individual annotations. That said, I wonder if – assuming that there are so many things to set for the response – that configuration wouldn't be better off expressed in a dedicated return type rather than a gazillion annotations or annotation attributes. Furthermore, the values contributed could then be calculated and thus be much more dynamic than sole static values. We could also make sure that values like pushUrl etc. are actual URLs etc.

If we stick to the annotation attributes approach, I'd recommend reviewing the individual ones to rather take simple Strings that a dedicated, nested annotation as the declaration is much more succinct then. triggers looks like it could be a simple String array instead of needing nested @HxTrigger.

xhaggi commented 8 months ago

That said, I wonder if – assuming that there are so many things to set for the response – that configuration wouldn't be better off expressed in a dedicated return type rather than a gazillion annotations or annotation attributes.

This is already part of yesterday's release 3.1. All response headers are now fully supported by using the return type HtmxResponse.

If we stick to the annotation attributes approach, I'd recommend reviewing the individual ones to rather take simple Strings that a dedicated, nested annotation as the declaration is much more succinct then. triggers looks like it could be a simple String array instead of needing nested @HxTrigger.

That could be done if we don't want to support trigger details. But sure, we can review and decide. Too bad that annotations are not as flexible here.

dsyer commented 8 months ago

I like the second example better (and agree that he-trigger cannot be expressed as an array of string in general).

I would like to reiterate my request for a @HxPushUrl variant that pushes the current url (including query params). That’s so common you would want it even if you didn’t need the other annotations.

xhaggi commented 8 months ago

I would like to reiterate my request for a @HxPushUrl variant that pushes the current url (including query params). That’s so common you would want it even if you didn’t need the other annotations.

This would definitely be handy, so that @HxPushUrl without a value defaults to the current url (ServletUriComponentsBuilder.fromRequest(request).toUriString()). I'll make a note of that 👍

odrotbohm commented 8 months ago

Wouldn't it make sense to make this the default when returning an HtmxResponse (thanks, @xhaggi, for pointing me to the refined version of that. Love it.) with an option to override the URL or opt out of this if it is so common?

wimdeblauwe commented 8 months ago

I would like to reiterate my request for a @HxPushUrl variant that pushes the current url (including query params). That’s so common you would want it even if you didn’t need the other annotations.

What would be the reason for using that annotation and not the hx-push-url attribute on the HTML itself?

dsyer commented 8 months ago

Because you don’t know the query params in the html.

wimdeblauwe commented 8 months ago

I don't get it sorry.

The way I have used it is like this:

<form th:action="@{/contacts}"
                    th:method="get">
          <label for="search">Search</label>
          <input type="search" name="q" id="search"
                         th:value="${query}"
                         hx:get="@{/contacts}"
                         hx-trigger="search, keyup delay:200ms changed"
                         hx-target="tbody"
                         hx-swap="outerHTML"
                         hx-push-url="true"
                         >
    <button type="submit">Search</button>
</form>

When submitting the form, a request to /contacts?q=... is done and when the response returns the URL in the browser is updated with that same URL. Or do you want to use a different query parameter in the URL in the browser compared to the query parameter you use for the htmx request?

dsyer commented 8 months ago

Oh hx-push-url="true" must be a special value I didn’t know about. That’s really useful, thanks. I will try it out now. You should take a look at the spring-petclinic-htmx and make sure there aren’t any other features if missed out on.

checketts commented 8 months ago

Loosely related, @wimdeblauwe using a common combination of attributes like hx-get and hx-push-url in conjunction happens frequently. I would love patterns that could simply that. (one attribute that does 2 or more in the resulting html).

Unrelated from annotations though... I haven't opened a ticket around it yet since I can't think of a good solution.

xhaggi commented 8 months ago

Thanks @wimdeblauwe for clarifying it. I hadn't thought of that, but I wonder why 😆

wimdeblauwe commented 8 months ago

Back on topic, i was thinking about ResponseEntity<T> which is a type used as a return type in controller methods. It allows so set the response status, but there is also @ResponseStatus as a "shortcut" to just set certain aspects of the response entity.

Thinking of that, i feel that separate annotations are closer aligned to this. The HtmxResponse class would be similar to ResponseEntity<T> and the new annotations would be the "shortcut" way.

The main issue I see is how we make it clear that our annotations are about the response without prefixing each of them with HxResponse ? Or would that be clear enough if we only have a single HxRequest annotation for request related checks?

xhaggi commented 8 months ago

I would have no problem with prefixed annotations.

@HxRequest
@HxResponseTrigger("event1")
@GetMapping("/path")

This is understandable and to be honest, you don't combine that many response header options.

dsyer commented 8 months ago

I still hate @HxResponseTrigger. What's wrong with @HxTrigger? It matches the HTML attribute name, and what else is it going to be other than an event to send in the response?

xhaggi commented 8 months ago

I don't care if it is HxResponseTrigger or HxTrigger. Since we have decided to go for one annotation per header, all that is missing now is the prefix 😄. @wimdeblauwe now it's your choice?

wimdeblauwe commented 8 months ago

Ok for me without prefix, given we use only @HxRequest for request headers (With triggerId and triggerName attributes like we added in 3.1).

@dsyer About:

I still hate @HxResponseTrigger. What's wrong with @HxTrigger? It matches the HTML attribute name, and what else is it going to be other than an event to send in the response?

The reason for this whole discussion was:

I guess for the server-side implementation, we have to make up our minds at some point how to deal with the fact that HTMX has request and response triggers. The current @HxTrigger seems to cater to the latter, which, I think, is a bit odd as controller method annotations are primarily metadata to map the incoming request.

So I don't know if @odrotbohm has now changed his mind? :)

odrotbohm commented 8 months ago

I haven't. I don't use annotations to control what happens with(in) the response. Guess that's coming from me being socialized with building REST APIs. Dealing with those involves working with concepts of HTTP: headers, status codes, bodies etc. It always felt more natural to me to handle all aspects of that in the very controller method and return a ResponseEntity.

Server-side rendered Spring MVC has always worked a bit different, as you can get away with populating the model and returning a view name for entire applications. I know that ModelAndView exists, but it doesn't seem ubiquitously used. I feel like HTMX interactions seem to breathe even more of the HTTP API spirit, as it heavily leans on HTTP concepts to make its magic work. That why I am totally fine with HtmxResponse alone, as that allows me to trigger the HTTP concepts, but speaks HTMX lingo at the same time.

wimdeblauwe commented 8 months ago

But if we add stuff like @HxTrigger, it is still a choice of the user to use either @HxTrigger or HtmxResponse, just like with @ResponseStatus and ReponseEntity I think. I don't think we should restrict the choice really.

odrotbohm commented 8 months ago

The reason I find @HxTrigger particularly odd is that looking at it on a method declaration, it's not clear whether that's an annotation that constraints the request mapping (what the attributes in @HxRequest now do) or whether it issues a trigger in the client.

xhaggi commented 8 months ago

Thanks all for expressing your point of views. @wimdeblauwe I like @HxResponse... more because it is aligned with the Spring @Response annotations and, as already said, it is more clear for the user what it does.

dsyer commented 8 months ago

I disagree (quite strongly). The only method-level annotations in Spring starting with @Response* are @ResponseBody and @ResponseStatus. They both refer to the return value from the method. @HxTrigger does not, and in my mind there is no confusion, and no need for alignment because there is nothing really to align with.

xhaggi commented 8 months ago

That is the point at which we go round in circles. @odrotbohm do you have any objections if we do it the way @dsyer says?

odrotbohm commented 8 months ago

… in my mind there is no confusion

So you're saying that seeing an @HxTrigger(…) on a method, not knowing whether it's constraining the request mapping ("a request that's been caused by HTML element …") or adding a trigger header to the response is not confusing? Only considering my HTMX knowledge, it could be both. Which is it?

Even taking my Spring knowledge into account: all Spring MVC annotations that do anything to the response contain Response in their name. @HxTrigger does not, which makes me even lean more into the direction of assuming it's a request constraining annotation. I don't like @HxResponseTrigger esthetically either, but it would clarify what's going on by both aligning with the Spring MVC convention and disambiguating the HTMX-level question.

dsyer commented 8 months ago

But, as I already said, a) there's nothing to align with, and b) there's no ambiguity. There's nothing else it could do in HTMX than send an event back with the response. I prefer to align with HTMX naming (the HTML attribute name -s "hx-trigger").

odrotbohm commented 8 months ago

There's nothing else it could do in HTMX than send an event back with the response.

It could. It could constrain the request mapping to only match requests that have a request header of HX-Trigger-(Name|ID). Basically, what @HxRequest(…) now does. So unless you know that there is another annotation to do that HxTrigger suspiciously looks like a mapping-constraining annotation, not a response related one.

dsyer commented 8 months ago

Sigh. So I learn that "Hx-Trigger" is both a request and a response header. It's the only one that can be both, so bad play htmx.org IMO, especially considering that the meaning is different (for a request it's an element ID, for a response it's an event name). That would seem to suggest that we might need both, and for them to be distinguishable. Since this is the only case of a header that belongs in both request and response it could be a special case though - there's no need to spoil the alignment with HTML attribute names for all the other headers.

So maybe we don't need to spoil it...

We could use @HxTrigger as a response header and add an attribute to @HxRequest (which currently doesn't need any because the request header only has the value "true").

Or we could make @HxTrigger a request header and not support it for responses at all. I'm in favour of that actually because an annotation probably doesn't give you any more flexibility than the HTML (an event name and a static payload). If you want a dynamic response header you have to use the HtmxResponse API anyway.

wimdeblauwe commented 8 months ago

Currently, HxRequest already supports triggerId and triggerName as attributes to indicate that the controller method should only be selected if an htmx request originated from an element with a given id or a given name (So corresponding to the Hx-Trigger request header).

HxTrigger is currently an annotation that adds a response header when used.

So in the current version, if you want a controller method that is matched for a HTML button with the id deleteButton and what to trigger a client-side event itemDeleted when something was deleted, you would do it like this:

@HxRequest(triggerId="deleteButton")
@HxTrigger("itemDeleted")
public String deleteItem() {
  ...
}

I have another idea: what if we use @HxTriggerEvent for the response header? For all other response headers, we align with the header names of htmx, and for this one, we slightly deviate, but we also make it a clearer (Maybe we could even ask them to use use Hx-Trigger-Event in htmx 2.0 for the response header?)?

odrotbohm commented 8 months ago

Hm, I don't think the extra Event really helps in distinguishing between request and response triggers. You have a point that @HxTrigger is likely to be used with @HxRequest so in the sample you showed one could get to the conclusion that @HxTrigger is related to the response. That said, I'd still prefer:

@HxRequest("deleteButton")
@HxResponseTrigger("itemDeleted")
public String deleteItem() { … }

simply because of the symmetry in the prefixes.

That said, we don't have to beat this to death. If in doubt, we could just stay with the current arrangement. I am likely to programmatically use HtmxResponse primarily anyway.

wimdeblauwe commented 8 months ago

In that case, I would propose to leave things as is.