Apply changes for rails-ujs and Turbo compatibility #5545

JunichiIto · 2023-01-11T13:49:18Z

Purpose

I want to use Devise easily for both Rails 6.1(rails-ujs) and Rails 7.0(Turbo). Typical steps are like this:

rails new
echo 'gem "devise"' >> Gemfile
bundle install
rails g devise:install
rails g devise User
rails db:migrate
edit application controller
add edit account link and sign out button
rails c

My PR enables it.

Notable changes

Status codes

Returns 422 Unprocessable Entity status instead of 200 when validation error occurs for the following actions:

POST /resource/confirmation
POST /resource/password
PUT  /resource/password
POST /resource
PUT  /resource
POST /resource/unlock

Returns 303 See Other status instead of 302 for the following actions:

DELETE /resource
DELETE /resource/sign_out

Returns 302 status instead of 200 when sign-in fails for turbo_stream requests.

Views

Recommends to use button_to to sign-out because it works for both rails-ujs and Turbo:

<%# This works for rails-ujs and Turbo %>
<%= button_to 'Sign out', destroy_user_session_path, method: :delete %>

If you want to use link_to to sign-out, please choose one or the other:

<%# Turbo %>
<%= link_to 'Sign out', destroy_user_session_path, data: { turbo_method: :delete } %>

<%# rails-ujs %>
<%= link_to 'Sign out', destroy_user_session_path, method: :delete %>

Example apps

I created example apps for Turbo and rails-ujs with my version of Devise. They also have system tests.

Please feel free ask me if you have any questions or requests.

JunichiIto · 2023-01-18T04:23:40Z

@carlosantoniodasilva @rafaelfranca Hello, I'd appreciate if you would review my PR.

carlosantoniodasilva · 2023-01-18T12:21:26Z

@JunichiIto really appreciate your work here, but please be a bit patient. If anything there's breaking changes to consider.

I'm looking into some current build failures before anything else, and we'll get to it.

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

carlosantoniodasilva

Hey @JunichiIto, appreciate our work here.

I left a few thoughts below as I looked through the changes, but no need to change anything yourself here. I've been working on related changes #5548 on top of what you did here and a few other PRs/issues, to try to accommodate for turbo without making it a full breaking change, and integrating with responders. Your updates here were extremely helpful to get a better understanding of what was needed, and get that up and running, so thank you.

Please feel free to give that one a shot, and leave any feedback or issue you may encounter there. Let's also keep this open for now, it can be closed if / when that one gets merged. Thanks again.

carlosantoniodasilva · 2023-01-31T13:44:27Z

app/controllers/devise/confirmations_controller.rb

@@ -14,7 +14,7 @@ def create
    if successfully_sent?(resource)
      respond_with({}, location: after_resending_confirmation_instructions_path_for(resource_name))
    else
-      respond_with(resource)
+      respond_with resource, status: :unprocessable_entity


Note: We will be able to use responders to handle this for us across the board going forward.

Please see https://github.com/heartcombo/responders/blob/fb9f787055a7a842584ce351793b249676290090/CHANGELOG.md#unreleased (still unreleased at the time of this writing)

carlosantoniodasilva · 2023-01-31T13:45:05Z

app/views/devise/registrations/edit.html.erb

@@ -38,6 +38,6 @@

 <h3>Cancel my account</h3>

-<p>Unhappy? <%= button_to "Cancel my account", registration_path(resource_name), data: { confirm: "Are you sure?" }, method: :delete %></p>
+<p>Unhappy? <%= button_to "Cancel my account", registration_path(resource_name), data: { confirm: "Are you sure?", turbo_confirm: "Are you sure?" }, method: :delete %></p>


This looks like a good option to support both confirm options.

carlosantoniodasilva · 2023-01-31T13:46:20Z

app/views/devise/shared/_links.html.erb

@@ -20,6 +20,6 @@

 <%- if devise_mapping.omniauthable? %>
  <%- resource_class.omniauth_providers.each do |provider| %>
-    <%= link_to "Sign in with #{OmniAuth::Utils.camelize(provider)}", omniauth_authorize_path(resource_name, provider), method: :post %><br />
+    <%= button_to "Sign in with #{OmniAuth::Utils.camelize(provider)}", omniauth_authorize_path(resource_name, provider), data: { turbo: false } %><br />


Yeah, I think the best option for this one might be a button since this is not something that should go through turbo I guess?

The only other option that could work would be two methods. (method: :post and data: { turbo_method: :post }), but again I'm not sure about the Turbo behavior for something like this so may be better to disable it entirely.

As far as I tested, Turbo didn't work with OAuth because it raises CORS error.

Here is the result when I click the link generated via <%= link_to "Sign in with #{OmniAuth::Utils.camelize(provider)}", omniauth_authorize_path(resource_name, provider), method: :post, data: { turbo_method: :post } %>:

So I had to change this line from link to button and add data: { turbo: false } option.

Good point, I hadn't thought about CORS but yeah since it's all happening via JS it will trigger a CORS error when trying to follow the redirect with fetch under the hood.

button_to it is then, to support this correctly on all cases, since there's no way to do POST with turbo like rails-ujs does. Thanks!

carlosantoniodasilva · 2023-01-31T13:47:23Z

lib/devise/failure_app.rb

@@ -38,7 +38,7 @@ def respond
      if http_auth?
        http_auth
      elsif warden_options[:recall]
-        recall
+        request_format == :turbo_stream ? redirect : recall


I am afraid that while redirecting works in the sense that it will re-display the form as expected, it will lose some context about the flash / error message and show the more generic "you need to be signed in" instead of the "invalid email or password".

Ideally we'd still recall, but change the status, so it has that same behavior.

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

JunichiIto · 2023-02-01T10:37:18Z

@carlosantoniodasilva Thank you for your feedback. I'm glad to hear you've started working on #5548 and looking forward to the day when it gets released.

carlosantoniodasilva · 2023-02-09T21:18:41Z

The main branch should contain all that's necessary for fully working with Turbo now, based on the changes here and a few others. A new version will be released soon, but feel free to test it out from the main branch in the meantime, and report back on any issues. Thanks.

JunichiIto force-pushed the for-turbo-stream branch 2 times, most recently from 0fc795d to 2d1ae92 Compare January 11, 2023 22:01

Apply changes for turbo_stream compatibility

683117d

JunichiIto force-pushed the for-turbo-stream branch from 2d1ae92 to 683117d Compare January 11, 2023 22:18

JunichiIto changed the title ~~[WIP] Apply changes for turbo_stream compatibility~~ Apply changes for rails-ujs and Turbo compatibility Jan 12, 2023

JunichiIto marked this pull request as ready for review January 12, 2023 01:12

carlosantoniodasilva self-assigned this Jan 18, 2023

carlosantoniodasilva mentioned this pull request Jan 31, 2023

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

Merged

4 tasks

carlosantoniodasilva reviewed Jan 31, 2023

View reviewed changes

carlosantoniodasilva closed this in #5548 Feb 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply changes for rails-ujs and Turbo compatibility #5545

Apply changes for rails-ujs and Turbo compatibility #5545

JunichiIto commented Jan 11, 2023 •

edited

Loading

JunichiIto commented Jan 18, 2023

carlosantoniodasilva commented Jan 18, 2023

carlosantoniodasilva left a comment

carlosantoniodasilva Jan 31, 2023

carlosantoniodasilva Jan 31, 2023

carlosantoniodasilva Jan 31, 2023

JunichiIto Feb 1, 2023

carlosantoniodasilva Feb 1, 2023

carlosantoniodasilva Jan 31, 2023

JunichiIto commented Feb 1, 2023

carlosantoniodasilva commented Feb 9, 2023

Apply changes for rails-ujs and Turbo compatibility #5545

Apply changes for rails-ujs and Turbo compatibility #5545

Conversation

JunichiIto commented Jan 11, 2023 • edited Loading

Purpose

Notable changes

Status codes

Views

Example apps

JunichiIto commented Jan 18, 2023

carlosantoniodasilva commented Jan 18, 2023

carlosantoniodasilva left a comment

Choose a reason for hiding this comment

carlosantoniodasilva Jan 31, 2023

Choose a reason for hiding this comment

carlosantoniodasilva Jan 31, 2023

Choose a reason for hiding this comment

carlosantoniodasilva Jan 31, 2023

Choose a reason for hiding this comment

JunichiIto Feb 1, 2023

Choose a reason for hiding this comment

carlosantoniodasilva Feb 1, 2023

Choose a reason for hiding this comment

carlosantoniodasilva Jan 31, 2023

Choose a reason for hiding this comment

JunichiIto commented Feb 1, 2023

carlosantoniodasilva commented Feb 9, 2023

JunichiIto commented Jan 11, 2023 •

edited

Loading