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

Add missing annotations for response headers #118

Conversation

xhaggi
Copy link
Contributor

@xhaggi xhaggi commented May 1, 2024

This will add the missing annotations for all available response headers.

Superseded #117.

@xhaggi xhaggi force-pushed the missing-response-header-annotation-support branch 2 times, most recently from b03bf62 to a981c71 Compare May 1, 2024 10:30
@xhaggi
Copy link
Contributor Author

xhaggi commented May 1, 2024

@tschuehly @wimdeblauwe one thing I have changed now is that @HxPushUrl has true as default, which pushes the fetched URL to the history. The documentation of the header Hx-Push-Url value is incomplete and this is only mentioned in the documentation of the hx-push-url attribute.

@xhaggi xhaggi force-pushed the missing-response-header-annotation-support branch from 6ca6c3b to 9e8f235 Compare May 1, 2024 18:43
@xhaggi
Copy link
Contributor Author

xhaggi commented May 1, 2024

I have made a few improvements to the Javadoc of the annotations.

@xhaggi
Copy link
Contributor Author

xhaggi commented May 1, 2024

@wimdeblauwe I added another commit which introduces dedicated annotations @HxTriggerAfterSettle and @HxTriggerAfterSwap with support for multiple events. Also @HxTrigger now supports multiple events. This replaces the HxTriggerLifecycle in @HxTrigger which is now deprecated and can be removed at a future version.

It also makes it possible to configure triggers for all three lifecycles (directly after response was processed, after settle and after swap) in the same response.

@xhaggi xhaggi force-pushed the missing-response-header-annotation-support branch 3 times, most recently from a65f505 to 0f9ab75 Compare May 1, 2024 19:40
@checketts
Copy link
Collaborator

@xhaggi What is the reasoning to deprecate @HxTriggerLifeCycle? Was there a discussion I need to catch up on?

@xhaggi
Copy link
Contributor Author

xhaggi commented May 2, 2024

The reason for this is that all the places where it is used are now code paths that should not be used in the future. Or do you think there is a reason to leave it as it is?

@wimdeblauwe
Copy link
Owner

Maybe we should deprecate the lifecycle attribute on @HxTrigger itself as well?

@checketts There was no prior discussion about this as far as I know. For me, both the current and the new proposal are fine. But this:

It also makes it possible to configure triggers for all three lifecycles (directly after response was processed, after settle and after swap) in the same response.

Seems like a nice advantage of the separate annotations, so I think it is a good improvement.

@xhaggi xhaggi force-pushed the missing-response-header-annotation-support branch 2 times, most recently from 4cfecd3 to 512d937 Compare May 2, 2024 07:55
@xhaggi
Copy link
Contributor Author

xhaggi commented May 2, 2024

Maybe we should deprecate the lifecycle attribute on @HxTrigger itself as well?

Sure and done. I also updated the README where I missed to add the annotations.

@xhaggi xhaggi force-pushed the missing-response-header-annotation-support branch from 512d937 to eb12958 Compare May 2, 2024 08:14
@xhaggi
Copy link
Contributor Author

xhaggi commented May 2, 2024

I've added another commit with small adjustments to reduce dupe code and make setting an htmx response header type-safe in HtmxHandlerInterceptor.

@xhaggi xhaggi requested a review from wimdeblauwe May 2, 2024 12:02
@checketts
Copy link
Collaborator

I'm fine with the deprecation. We had created it to clarify the usage, but I haven't used it myself so making it more flexible is fine by me.

@wimdeblauwe wimdeblauwe added this to the 3.4.0 milestone May 5, 2024
@wimdeblauwe wimdeblauwe merged commit a8b65ab into wimdeblauwe:main May 5, 2024
2 checks passed
@wimdeblauwe wimdeblauwe mentioned this pull request May 5, 2024
@xhaggi xhaggi deleted the missing-response-header-annotation-support branch May 31, 2024 08:49
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.

3 participants