-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Integrate with Hotwire/Turbo by configuring error and response statuses #5548
Conversation
1947041
to
a71c5de
Compare
Treat `:turbo_stream` request format as a navigational format, much like HTML, so Devise/responders can work properly. Allow configuring the `error_status` and `redirect_status` using the latest responders features, via a new custom Devise responder, so we can customize the both responses to match Hotwire/Turbo behavior, for example with `422 Unprocessable Entity` and `303 See Other`, respectively. The defaults aren't changing in Devise itself (yet), so it still responds on errors cases with `200 OK`, and redirects on non-GET requests with `302 Found`, but new apps are generated with the new statuses and existing apps can opt-in. Please note that these defaults might change in a future release of Devise. PRs/Issues references: #5545 #5529 #5516 #5499 #5487 #5467 #5440 #5410 #5340 #5542 #5530 #5519 #5513 #5478 #5468 #5463 #5458 #5448 #5446 #5439
a71c5de
to
f08e0ad
Compare
@carlosantoniodasilva Just tested this out on our Hotwire app and it works like a charm! 👍 Haven't tested with a fresh Rails 7 app or an old UJS app yet, but will do shortly. |
@carlosantoniodasilva Thank you for your great work! I tested this version against both Rails 6 (rails-ujs) and Rails 7 (Turbo) apps and confirmed they worked well 👍 My concern is sign-in link for OmniAuth. You might need to change it to button and disable Turbo as I discussed here. |
@excid3 that's awesome to hear, thanks! @JunichiIto awesome too! Thanks for testing it out on both apps.
Yeah, I hadn't checked on that yet, but you're right about the CORS issue, I'll update this branch to use |
This changes the OmniAuth "sign in" links to use buttons, which can be wrapped in an actual HTML form with a method POST, making them work better with and without Turbo in the app. It doesn't require rails/ujs anymore in case of a non-Turbo app, as it previously did with links + method=POST. Turbo is disabled for those OmniAuth buttons, as they simply don't work trying to follow the redirect to the OmniAuth provider via fetch, causing CORS issues/errors.
3e990a3
to
88625d4
Compare
Unfortunately we can't enforce the version in the gemspec because responders only supports Rails 5.2 now, and Devise still supports previous versions. We'll drop support for those in a future major release, so for now I'm not adding any version. This also adds a warning in case someone is using an older version of responders and tries to set the error/redirect statuses via Devise, so that they know what to do (upgrade responders) in that case.
e713db9
to
0d392fa
Compare
Just want to have something different than the currently released version to test out more easily. Plus, this is probably going to become v4.9.0 final soon anyway.
3e9b51f
to
3c76707
Compare
There's some additional information in the wiki upgrade guide for those interested, but most of it is covered in the changelog and should suffice. The post install message should help guide people upgrading to make sure they know what to do in this new version, since some may be using Turbo out there with custom responders and failure apps and those would have to be removed in order to use these new changes fully. Hopefully that's enough of a nudge for them.
3c76707
to
2df5efc
Compare
In Changelog or wiki, it might be worth mentioning that sign-out link requires <%# works with rails-ujs but does not work with Turbo %>
<%= link_to 'Sign out', destroy_user_session_path, method: :delete %>
<%# works with rails-ujs and Turbo %>
<%= link_to 'Sign out', destroy_user_session_path, method: :delete, data: { turbo_method: :delete } %>
<%= button_to 'Sign out', destroy_user_session_path, method: :delete %> |
@@ -1,5 +1,5 @@ | |||
<% if resource.errors.any? %> | |||
<div id="error_explanation"> | |||
<div id="error_explanation" data-turbo-cache="false"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I'm not sure why this change is required. I couldn't find any strange behavior even if I remove this option. Could you explain a bad scenario when we remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not strictly required for the functionality. I was noticing an issue when going to edit the user, submitting a couple of times, then using the Back
button (which is a javascript history.back() call), it was flashing the errors from the cached page before getting the response back from the server and displaying it:
CleanShot.2023-02-07.at.10.07.42.mp4
So skipping the cache of this errors section seems to have "fixed" that for me. Makes sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it was flashing the errors
Thank you for the explanation. It's hard to notice but I understand🙂
I feel "OmniAuth provider" should be "OAuth provider" in this context. Correct? |
Explain a bit more about how `data-confirm` and `data-method` need to be updated to the turbo versions `data-turbo-confirm` and `data-turbo-method`, respectively. (and depending on its usage.) [ci skip]
@JunichiIto thanks for the feedback. I believe I have covered it all with the last commit, updating the changelog/readme (and the wiki) with some information about confirm/method changes for Turbo.
Good call, I added some info about the sign out link/button.
I am removing |
Thank you for the update. It's perfect! |
Just FYI, I'm translating and updating the upgrade guide in Japanese. https://gist.github.com/JunichiIto/1398835b9825a9ce3958456041f59d27 |
Just tried this out in a project. Works fine 👍 |
Albeit it's not super recommended, it's possible and even mentioned in the changelog/wiki in case the app has some additional responder logic that needs to be applied to Devise across the board.
🎉 Thanks @carlosantoniodasilva! 🎉 |
Treat
:turbo_stream
request format as a navigational format, much like HTML, so Devise/responders can work properly.Allow configuring the
error_status
andredirect_status
using the latest responders features, via a new custom Devise responder, so we can customize the both responses to match Hotwire/Turbo behavior, for example with422 Unprocessable Entity
and303 See Other
, respectively. The defaults aren't changing in Devise itself (yet), so it still responds on errors cases with200 OK
, and redirects on non-GET requests with302 Found
, but new apps are generated with the new statuses and existing apps can opt-in. Please note that these defaults might change in a future release of Devise.PRs/Issues references:
Closes #5545
Closes #5529
Closes #5516
Closes #5499
Closes #5487
Closes #5467
Closes #5440
Closes #5410
Closes #5340
Closes #5542
Closes #5530
Closes #5519
Closes #5513
Closes #5478
Closes #5468
Closes #5463
Closes #5458
Closes #5448
Closes #5446
Closes #5439
TODO
responder
and how to override it back if needed.Testing this branch on an existing app
In order to test this branch for turbo compatibility on an existing app, update your Gemfile to point Devise to this branch, and Responders to version 3.1+:
Then, add the following config to your Devise initializer (located in
config/initializers/devise.rb
):Note: these are going to be the new defaults for newly generated apps, and will likely become the defaults for Devise in a future release as well, but for now they're configurable and default to the current values for backwards compatibility.
Check the upgrade wiki (WIP) for more info: https://github.com/heartcombo/devise/wiki/How-To:-Upgrade-to-Devise-4.9.0-(Hotwire---Turbo-integration)