-
-
Notifications
You must be signed in to change notification settings - Fork 122
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
Rails 6 and Solidus AuthDevise issues? #189
Comments
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! |
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 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 |
I think this may work. If we defined this config method in a |
@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. |
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 I'm running Rails 6.0.2.2, in case that helps with further investigation. Much appreciated! |
@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! |
@tmtrademarked hey there, can we close this one? |
Unfortunately for me, I still haven't figured out how to fix this on my end. But if this isn't reproducible in |
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 |
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:
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:
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)
The text was updated successfully, but these errors were encountered: