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

turbo_stream fix bugs for rails 7 and above apps #5467

Closed
wants to merge 4 commits into from

Conversation

Salanoid
Copy link

@Salanoid Salanoid commented Feb 9, 2022

I tried to fix this two issues: #5448, #5446.

@Salanoid Salanoid changed the title turbo_stream fix bugts for rails 7 and above apps turbo_stream fix bugs for rails 7 and above apps Feb 10, 2022
@clemensg
Copy link

@carlosantoniodasilva Would be great to get this or something similar merged

Comment on lines +299 to +304
# ==> Turbolinks configuration
# If your app is using Turbolinks, Turbolinks::Controller needs to be included to make redirection work correctly:
#
# ActiveSupport.on_load(:devise_failure_app) do
# include Turbolinks::Controller
# end

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose this part can be omitted as Turbolinks is deprecated and superseded by Turbo, where there is no such Turbolinks::Controller and such an include appears unnecessary.

Also see #5324

@tmaier
Copy link

tmaier commented Mar 27, 2022

PR #5340 looks like it aims for a deeper integration into devise than what is proposed in this pull request.

Besides this, using turbo is also valid in a rails 6.1 context. So just checking for "> Rails 7" would exclude some valid use cases

@msducheminjr
Copy link

@tmaier Would something along the lines of a class_option boolean on the generator allow for less surprising behavior than checking for a Rails version?

  class_option :turbo, type: :boolean, default: false

That way somebody who wants Turbo could run rails generate devise:install --turbo to get Turbo behavior generated irrespective of Rails version.

You could make the default true for 5.x, but keep behavior as-is unless somebody specifically requests generation with Turbo in 4.x.

@tmaier
Copy link

tmaier commented Jun 21, 2022

I am not sure what's the right approach in the context of devise.

I think I would have checked, if Turbo is defined with defined?(::Turbo)

Ref. https://github.com/hotwired/turbo-rails/blob/main/lib/turbo-rails.rb

@shaunbruno
Copy link

PR #5340 looks like it aims for a deeper integration into devise than what is proposed in this pull request.

Besides this, using turbo is also valid in a rails 6.1 context. So just checking for "> Rails 7" would exclude some valid use cases

Perhaps Devise should just be versioned with incremental support for Rails 6 dropped, in favor of a cleaner implementation for Rails 7+?

@evdevdev
Copy link

evdevdev commented Nov 7, 2022

Forgive me for chiming in on this, but I am getting confused by the different issues.

Is there a canonical (at at least blessed) way to get Devise and Rails 7 playing nicely together?

carlosantoniodasilva added a commit that referenced this pull request 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:

#5545
#5529
#5516
#5499
#5487
#5467
#5440
#5410
#5340

#5542
#5530
#5519
#5513
#5478
#5468
#5463
#5458
#5448
#5446
#5439
carlosantoniodasilva added a commit that referenced this pull request 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:

#5545
#5529
#5516
#5499
#5487
#5467
#5440
#5410
#5340

#5542
#5530
#5519
#5513
#5478
#5468
#5463
#5458
#5448
#5446
#5439
carlosantoniodasilva added a commit that referenced this pull request 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:

#5545
#5529
#5516
#5499
#5487
#5467
#5440
#5410
#5340

#5542
#5530
#5519
#5513
#5478
#5468
#5463
#5458
#5448
#5446
#5439
carlosantoniodasilva added a commit that referenced this pull request 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:

#5545
#5529
#5516
#5499
#5487
#5467
#5440
#5410
#5340

#5542
#5530
#5519
#5513
#5478
#5468
#5463
#5458
#5448
#5446
#5439
@carlosantoniodasilva
Copy link
Member

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.

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

Successfully merging this pull request may close these issues.

8 participants