-
Notifications
You must be signed in to change notification settings - Fork 0
Autoload issue with ImpressionistController #2
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
Conversation
Instead of depending on autoload to bring in the `ImpressionistController` constant, we manually bring it with a call to `require` and just move that file to `lib`. Prior to this, Rails would emit a warning on startup about needing to use autoload durning an initializer like this, letting us know that a future version of Rails will error from this. This should hopefully remove that error and still allow these controller mixins to be available. Co-authored-by: stefannibrasil <[email protected]>
@@ -20,7 +20,7 @@ Gem::Specification.new do |s| | |||
|
|||
s.add_dependency "friendly_id" | |||
s.add_dependency 'nokogiri', RUBY_VERSION < '2.1.0' ? '~> 1.6.0' : '~> 1' | |||
s.add_dependency 'rails', '>= 3.2.15' | |||
s.add_dependency 'rails', '>= 3.2.15', '< 7.1' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this constraint, we were getting an ArgumentError
when creating a Rails 7.1 app.
@@ -53,14 +53,14 @@ Layout/BlockAlignment: | |||
# Cop supports --auto-correct. | |||
Layout/ClosingParenthesisIndentation: | |||
Exclude: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of the changes in this file are because we needed to rename the previous file to the new file inside of /lib.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not actually sure why we have this gem--but the changes make sense. If it's going to be a problem w Rails 7.1, maybe we should spend some time researching if we can drop it completely? (definitely thinking outloud/out-of-scope for this change!)
I did a quick search and based on this comment from @mercedesb, we only use it for tracking the Landing pages visitors: https://convertkit.slack.com/archives/CHZGVGC2W/p1686947789442399 but it looks like this is an old question: https://convertkit.slack.com/archives/C02G2JWUX/p1578402933118100 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sneaky sneaky
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's correct, I believe Impressionist
is used for legacy landing pages, so usage of this should be dropping, but still in use. :|
Instead of depending on autoload to bring in the
ImpressionistController
constant, we manually bring it with a call torequire
and just move that file tolib
.Prior to this, Rails would emit a warning on startup about needing to use autoload durning an initializer like this, letting us know that a future version of Rails will error.
This should hopefully remove that error and still allow these controller mixins to be available.
PS. This is the recommended approach by Xavier here charlotte-ruby#305