hotwired / turbo-rails

Use Turbo in your Ruby on Rails app
https://turbo.hotwired.dev
MIT License
2.07k stars 320 forks source link

Devise FailureApp can't include Turbo::Native::Navigation anymore #507

Open arusa opened 10 months ago

arusa commented 10 months ago

As learned from @joemasilotti, I was including Turbo::Native::Navigation in my Devise FailureApp, so I could use the method turbo_native_app? there.

Since v1.5.0 that doesn't work anymore, because Turbo::Native::Navigation uses helper_method, which is not available in Devise::FailureApp:

https://github.com/hotwired/turbo-rails/blob/097d8f90cf0c5ed24ac6b1a49cead73d49fa8ab5/app/controllers/turbo/native/navigation.rb#L10

This is not a big issue, as I can easily duplicate the turbo_native_app? method.

But I wonder if this is intended, or if classes, that don't know about helpers, should still be able to include Turbo::Native::Navigation.

seanpdoyle commented 10 months ago

@arusa The concern is intended to extend ActionController::Base descendants (since it's in defined in app/controllers/).

If the maintainers are interested in that kind of broader support, a PR that guarded https://github.com/hotwired/turbo-rails/blob/main/app/controllers/turbo/native/navigation.rb#L10 with a respond_to?(:helper_method) block might be enough to implement it.

arusa commented 10 months ago

Sounds like an easy solution.

If it's intended to extend ActionController::Base there could be more "features" added later that break the apps again, that include it somewhere else.

Maybe the method turbo_native_app? should live in a more generic place, where it is accessible from everywhere and Turbo::Native::Navigation could also use that.

excid3 commented 10 months ago

A temporary solution is to define a helper_method that does nothing:

class TurboFailureApp < Devise::FailureApp
  # Compatibility for Turbo::Native::Navigation
  class << self
    def helper_method(name)
    end
  end

  include Turbo::Native::Navigation

  # Turbo Native requests that require authentication should return 401s to trigger the login modal
  def http_auth?
    turbo_native_app? || super
  end
end