Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Responders #5

Merged
merged 3 commits into from
Oct 24, 2023
Merged

Responders #5

merged 3 commits into from
Oct 24, 2023

Conversation

pfzetto
Copy link
Contributor

@pfzetto pfzetto commented Oct 22, 2023

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
Copy link
Owner

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!

@robertwayne robertwayne linked an issue Oct 22, 2023 that may be closed by this pull request
@pfzetto
Copy link
Contributor Author

pfzetto commented Oct 22, 2023

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
Copy link
Owner

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.

@pfzetto pfzetto marked this pull request as ready for review October 23, 2023 18:44
@pfzetto pfzetto changed the title WIP: Responders Responders Oct 23, 2023
@robertwayne
Copy link
Owner

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!

@robertwayne robertwayne merged commit 6eea312 into robertwayne:main Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding HtmxResponse to easily set Htmx Response Headers
2 participants