wimdeblauwe / htmx-spring-boot

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

Restrict mapping to the handler method to target and triggered element in @HxRequest #80

Closed xhaggi closed 9 months ago

xhaggi commented 9 months ago

Closes #79.

checketts commented 9 months ago

Is any work need to ensure this annotation can be composed/subclassed? For example if I wanted to create a @HxTriggeredBy("someId") does this approach allow for that?

xhaggi commented 9 months ago

@checketts thanks for pointing that out. I changed the code from AnnotationUtils.findAnnotation to AnnotatedElementUtils.findMergedAnnotation which supports @AliasFor semantics and annotation hierarchy.

xhaggi commented 9 months ago

@wimdeblauwe as already expressed as an idea, I introduced @HtmxRequestMapping besides @HxRequest and marked @HxRequest as deprecated. Let me know if we should go that way. I will adjust the README afterwards.

wimdeblauwe commented 9 months ago

I am not a fan of @HtmxRequestMapping, because then the code will look like this:

@HtmxRequestMapping("my-button")
@GetMapping("/some-url")
public String doSomething() {
    ...
}

In the case of @GetMapping (which is a @RequestMapping under the hood), the value indicates the URL path the method is available on. In the case of @HtmxRequestMapping, it is not about the URL, but a triggerId or triggerName. @odrotbohm how do you feel about this?

xhaggi commented 9 months ago

IMO it does the same as @RequestMapping but for htmx requests, it maps an htmx request to a handler method, optionally with further restrictions. But we can stick with the current naming. It was just an idea.

odrotbohm commented 9 months ago

I must have lost track on this conversation (PTO currently), but I agree with Wim, that we shouldn't make this a …Mapping annotation. I'm fine with a switch from @Hx… to @Htmx…. Conceptually, what @HxRequest currently does is, is that it qualifies the sibling @…Mapping annotation, replacing a raw headers="…" with a first-class representation of the HTMX concepts that further constrain the mapping. The annotation abstracts the implementation (using HTTP headers) that HTMX uses, and I like that. That said, I don't think we should imply that we introduce a parallel way of mapping, as that's not what we do here.

xhaggi commented 9 months ago

Ok, thanks, then I'll discard it and we'll keep HxRequest.

xhaggi commented 9 months ago

@wimdeblauwe skipped the HtmxRequestMapping idea and addressed all your comments.