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

Handle flash attributes on htmx redirects #137

Merged

Conversation

xhaggi
Copy link
Contributor

@xhaggi xhaggi commented Oct 25, 2024

This adds handling for flash attributes via RedirectAttributes. Now you can perform client-side redirects with htmx and the flash attributes work as with normal redirects.

Fixes #96

@xhaggi
Copy link
Contributor Author

xhaggi commented Oct 25, 2024

@wimdeblauwe there is a limitation, because the flash attribute processing does not work if HtmxResponse is set as model attribute (PR #128). I'm not sure if it was a good idea to support such a case and to be honest, I don't like it.

@xhaggi xhaggi force-pushed the feature/handle-flash-attributes branch from aac224b to 8097ea9 Compare October 25, 2024 12:09
@xhaggi xhaggi force-pushed the feature/handle-flash-attributes branch from 8097ea9 to a15d7c2 Compare October 25, 2024 12:10
@xhaggi
Copy link
Contributor Author

xhaggi commented Oct 25, 2024

BTW @wimdeblauwe there is more magic done by RedirectView

  • Treat the redirect URL as a URI template (default: true)
  • Model attributes can be exposed as HTTP query parameters (default: true)
  • Propagate the query params of the current URL. (default: false)

We could support all of them to behave as similarly as possible to Spring. Let me know what you think about it.

@checketts
Copy link
Collaborator

@wimdeblauwe there is a limitation, because the flash attribute processing does not work if HtmxResponse is set as model attribute (PR #128). I'm not sure if it was a good idea to support such a case and to be honest, I don't like it.

I couldn't find an alternative approach that was compatible with using JTE Models as the return. I'm happy to try another approach if you have any suggestions.

@xhaggi
Copy link
Contributor Author

xhaggi commented Oct 25, 2024

@checketts let's move the discussion about an alternative approach to the initial issue #127.

@wimdeblauwe
Copy link
Owner

BTW @wimdeblauwe there is more magic done by RedirectView

  • Treat the redirect URL as a URI template (default: true)
  • Model attributes can be exposed as HTTP query parameters (default: true)
  • Propagate the query params of the current URL. (default: false)

We could support all of them to behave as similarly as possible to Spring. Let me know what you think about it.

If you are willing to implement that, it would be great if we align as much as possible with RedirectView I think.

@xhaggi
Copy link
Contributor Author

xhaggi commented Oct 28, 2024

If you are willing to implement that, it would be great if we align as much as possible with RedirectView I think.

Sure, but let's merge this before and I'll create a separate PR for it. Okay?

@wimdeblauwe
Copy link
Owner

wimdeblauwe commented Oct 30, 2024

ok, but can we already merge this? Don't we need a solution to #127 first?

@xhaggi
Copy link
Contributor Author

xhaggi commented Oct 30, 2024

I'm not sure because I need more information from @checketts on how he uses it in his projects. But if you ask me, we should revert #127 in favor of a better solution to his issue. It's definitely the wrong approach to render views in HtmxHandlerInterceptor#postHandle which was introduced by #127. And as far as I know, using JteModel as a return type is not a supported feature of Jte itself.

See https://jte.gg/spring-boot-starter-3/ or https://github.com/casid/jte/tree/main/jte-spring-boot-starter-3/src/main/java/gg/jte/springframework/boot/autoconfigure for more information about integration with Spring Boot.

@wimdeblauwe
Copy link
Owner

Ok, I agree that maybe it was bad judgement to have #127. If we merge this, it means @checketts is not blocked for now and we can have flash attributes, just not the combination of flashattributes and #127, right?

@xhaggi
Copy link
Contributor Author

xhaggi commented Oct 31, 2024

That's right.

@wimdeblauwe wimdeblauwe merged commit e37b557 into wimdeblauwe:main Oct 31, 2024
2 checks passed
@wimdeblauwe wimdeblauwe added this to the 3.6.0 milestone Oct 31, 2024
@checketts
Copy link
Collaborator

I'm OK with that trade off. Sorry I've not been able to contribute much (been away working on building a physical building)

I'll re-open #127 and discuss the JTE specific needs and options there.

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.

[feature request]: RedirectAttributes with .redirect() ?
3 participants