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

Integrate with Hotwire/Turbo by configuring error and response statuses #5548

Merged
merged 7 commits into from
Feb 9, 2023

Conversation

carlosantoniodasilva
Copy link
Member

@carlosantoniodasilva carlosantoniodasilva commented Jan 31, 2023

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:

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

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+:

gem "devise", github: "heartcombo/devise", branch: "ca-turbo"
gem "responders", "~> 3.1"

Then, add the following config to your Devise initializer (located in config/initializers/devise.rb):

  # ==> Hotwire/Turbo configuration
  config.responder.error_status = :unprocessable_entity
  config.responder.redirect_status = :see_other

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)

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
CHANGELOG.md Outdated Show resolved Hide resolved
@excid3
Copy link
Contributor

excid3 commented Jan 31, 2023

@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.

JunichiIto added a commit to JunichiIto/devise-rails-6-1-sandbox that referenced this pull request Feb 1, 2023
JunichiIto added a commit to JunichiIto/devise-rails-7-sandbox that referenced this pull request Feb 1, 2023
JunichiIto added a commit to JunichiIto/fjord-books_app-2023 that referenced this pull request Feb 1, 2023
@JunichiIto
Copy link
Contributor

@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.

@carlosantoniodasilva
Copy link
Member Author

@excid3 that's awesome to hear, thanks!

@JunichiIto awesome too! Thanks for testing it out on both apps.

My concern is sign-in link for OmniAuth. You might need to change it to button and disable Turbo as I discussed #5545 (comment).

Yeah, I hadn't checked on that yet, but you're right about the CORS issue, I'll update this branch to use button_to to work around that for both cases. 👍

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.
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.
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.
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.
@JunichiIto
Copy link
Contributor

In Changelog or wiki, it might be worth mentioning that sign-out link requires data: { turbo_method: :delete } for Turbo. Developers might not notice this change because sign-out link or button isn't generated by devise install.

<%# 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">
Copy link
Contributor

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?

Copy link
Member Author

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?

Copy link
Contributor

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🙂

@JunichiIto
Copy link
Contributor

JunichiIto commented Feb 4, 2023

https://github.com/heartcombo/devise/wiki/How-To:-Upgrade-to-Devise-4.9.0-%5BHotwire-Turbo-integration%5D#omniauth-sign-in-links-in-the-shared-views-changed-to-buttons

OmniAuth "Sign in with" links where implemented using method: :post in order to send POST requests to the server that would then redirect to the OmniAuth provider

I feel "OmniAuth provider" should be "OAuth provider" in this context. Correct?

JunichiIto added a commit to JunichiIto/everydayrails-rspec-jp-2022 that referenced this pull request Feb 4, 2023
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]
@carlosantoniodasilva
Copy link
Member Author

@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.

In Changelog or wiki, it might be worth mentioning that sign-out link requires data: { turbo_method: :delete } for Turbo. Developers might not notice this change because sign-out link or button isn't generated by devise install.

Good call, I added some info about the sign out link/button.

I feel "OmniAuth provider" should be "OAuth provider" in this context. Correct?

I am removing OmniAuth and just leaving provider there for simplicity.

@JunichiIto
Copy link
Contributor

updating the changelog/readme (and the wiki) with some information about confirm/method changes for Turbo.

Thank you for the update. It's perfect!

@JunichiIto
Copy link
Contributor

Just FYI, I'm translating and updating the upgrade guide in Japanese.

https://gist.github.com/JunichiIto/1398835b9825a9ce3958456041f59d27

@BilalBudhani
Copy link

Just tried this out in a project. Works fine 👍

@carlosantoniodasilva carlosantoniodasilva marked this pull request as ready for review February 8, 2023 16:06
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.
@excid3
Copy link
Contributor

excid3 commented Feb 9, 2023

🎉 Thanks @carlosantoniodasilva! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment