search

robertwayne / axum-htmx

A set of htmx extractors, responders, and request guards for axum.
Apache License 2.0
186 stars 10 forks source link

Responders #5

Closed pfzetto closed 1 year ago

pfzetto commented 1 year ago

Hello, sorry for the delay. This is the first WIP implementation of responders as mentioned in #4.

I`m going to improve the current implementation but thought it might be good if you can see what I'm up to. I've implemented the responders similar to the existing extractors to provide a consistent usage of this lib. The HX-Location header needs more work, the others I would consider relatively finished.

There are still a few other things I'm not completely happy with, but I currently don't see a better way (Error handling, usage of serde + serde_json). And of course the documentation in Readme.md is still missing.

Greetings, Paul

robertwayne commented 1 year ago

Awesome, thanks!

Taking on a serde dependency for the feature is unfortunate, but looks like it will be necessary with the arbitrary data htmx accepts in some of the headers. I'm almost thinking we allow HX-Location and HX-Trigger (plus its variants) to work without serde. Then, if the user wants to use data in their events / location, they can include the serde feature.

This would allow responders to be included in the core library, which seems preferable to me. Thoughts?

Something else to note is that HxTrigger is both a request extractor and a response now, which means we have an ambiguous glob export from the top-level.

My initial thought is to maybe change it so feature-gated items have to be explicitly called (eg. axum_htmx::responders::HxTrigger, while leaving the extractors as glob exports. This becomes moot if we ditch responders as a feature for serde, though.

An alternative would be to remove the top-level exports and require explicit imports by default (eg. axum_htmx::extractors::HxTrigger). Though I do find the latter less dev-friendly, it is the more consistent option.

Renaming is also an option, but I think maintaining 1:1 naming with the htmx header names is ideal. Special casing HX-Trigger might be confusing. I'm not completely opposed to it, though.

I have to think about the error handling some more, but this seems to be the best way at present.

Looks good so far, though!

pfzetto commented 1 year ago

I like the approach of implementing basic versions of Hx-Trigger* and Hx-Location that use the non-json method described in the spec with an optional serde-feature.

As for the ambiguous naming, I think that a HxTrigger and Hx-Triggerare more confusing than helping. I would think a rename to something likeHxResTrigger` would be better. Not using the glob exports would certainly fix the problem on the library side, but not on the implementation-side as extractors and responders most likely will be used in the same context and would require a import renaming each time they are used.

robertwayne commented 1 year ago

As for the ambiguous naming, I think that a HxTrigger and Hx-Triggerare more confusing than helping. I would think a rename to something likeHxResTrigger` would be better. Not using the glob exports would certainly fix the problem on the library side, but not on the implementation-side as extractors and responders most likely will be used in the same context and would require a import renaming each time they are used.

I agree here. HxResponseTrigger* would be suitable then.

robertwayne commented 1 year ago

This all looks pretty good to me, everything seems to work as expected from the testing I've done. Thanks for your work on this!