solidusio / solidus_auth_devise

🔑 Devise authentication for your Solidus store.
http://solidus.io
BSD 3-Clause "New" or "Revised" License
53 stars 128 forks source link

Rails 6 and Solidus AuthDevise issues? #189

Closed tmtrademarked closed 4 years ago

tmtrademarked commented 4 years ago

When loading my new Rails 6 app which uses Solidus, I get a warning about autoloading parts of the Solidus library. It seems like this is something which is not ideal, given that the warning states the autoload behavior is potentially going away in the future:

DEPRECATION WARNING: Initialization autoloaded the constants Spree::AuthConfiguration.

Being able to do this is deprecated. Autoloading during initialization is going
to be an error condition in future versions of Rails.

Reloading does not reboot the application, and therefore code executed during
initialization does not run again. So, if you reload Spree::AuthConfiguration, for example,
the expected changes won't be reflected in that stale Class object.

These autoloaded constants have been unloaded.

Please, check the "Autoloading and Reloading Constants" guide for solutions.

Any advice on how to fix this? Is this something with local setup? Or something specifically with the devise configuration? It seems likely this something related to how Zweitwork stuff is configured in this gem, but it's not immediately obvious.

Solidus Version:

[3] pry(main)> Spree.solidus_version
=> "2.10.0"

Gem versions: rails (6.0.2.2) solidus_api (2.10.0) solidus_auth_devise (2.4.0) solidus_backend (2.10.0) solidus_core (2.10.0)

aldesantis commented 4 years ago

I think this may be caused by this line, which is a common pattern in Solidus extensions. I'll try to take a look and see if we can move it somewhere else. Thanks for reporting this!

aldesantis commented 4 years ago

I've had a look at this today. It seems to me that the "right" way to fix this would be to stop using a constant to store the configuration instance, and use a method instead. In fact, using a constant to store an instance of a class is a weird pattern that we should get rid of in any case.

The new syntax would look something like the following:

# In the extension code:
module Solidus
  module Auth
    def self.config
      @config ||= Spree::AuthConfiguration.new
    end
  end
end

# In application code:
Solidus::Auth.config.option = 'value'

However, this is obviously a breaking change, and we should probably find a way to make it non-breaking, otherwise we'll have to release new majors of pretty much all the extensions. I think we could use const_missing:

module Solidus
  module Auth
    def self.config
      @config ||= Spree::AuthConfiguration.new
    end

    def self.const_missing(name)
      name == :Config ? config : super
    end
  end
end

Speaking with @kennyadsl, we also came to the conclusion that perhaps we don't need the configuration class to be reloadable at all, so we may just move into a non-reloadable path such as lib.

kennyadsl commented 4 years ago

I think this may work. If we defined this config method in a lib/ file that is required before the engine, the deprecation warning should go away and we still have the configuration loaded before: :load_config_initializers.

aldesantis commented 4 years ago

@tmtrademarked sorry, which Rails version are you using? I'm testing on an app with Rails 6, Zeitwerk and Solidus 2.10.0, and I don't get the warning you mentioned in the original issue.

tmtrademarked commented 4 years ago

Ah! I thought I was going crazy, because I couldn't reproduce this myself this morning. It turns out this is only happening for me when I run my specs. Running rails s or rails c doesn't in fact produce this warning. But running rspec does trigger this. So in the light of day, it seems like this may be more of an issue with how I've set up my testing infrastructure than a true issue in the framework. (At least, potentially. :-) ) Apologies for the noise if so!

I'm running Rails 6.0.2.2, in case that helps with further investigation. Much appreciated!

aldesantis commented 4 years ago

@tmtrademarked I think it might be something on your end, because I can't reproduce the issue in RSpec either. However, please let me know if you find a setup that allows us to reproduce it consistently!

kennyadsl commented 4 years ago

@tmtrademarked hey there, can we close this one?

tmtrademarked commented 4 years ago

Unfortunately for me, I still haven't figured out how to fix this on my end. But if this isn't reproducible in rspec with a default setup, then it probably isn't a core bug. Thanks for taking a look! I'll close for now, and if I happen to figure out how to reproduce in a clean setup, I'll reopen then.

BenMorganIO commented 3 years ago

I'm starting to experience this issue as well with Rails 6. I was able to resolve it with some other code by wrapping it in a config.to_prepare block. I'm :+1: on implementing both a constant and a method.