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

Autoload issue with ImpressionistController #2

Merged
merged 3 commits into from
Oct 16, 2023

Conversation

keegnotrub
Copy link

@keegnotrub keegnotrub commented Oct 10, 2023

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.

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

keegnotrub and others added 3 commits October 10, 2023 13:30
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'

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:

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.

@stefannibrasil stefannibrasil marked this pull request as ready for review October 13, 2023 19:54
Copy link

@christinalcole christinalcole left a 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!)

@stefannibrasil
Copy link

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

Copy link

@mercedesb mercedesb left a comment

Choose a reason for hiding this comment

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

Sneaky sneaky

Copy link

@jonlunsford jonlunsford left a 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. :|

@keegnotrub keegnotrub merged commit 59c8f00 into master Oct 16, 2023
2 checks passed
@stefannibrasil stefannibrasil deleted the sb-rk-autoload-controller-fix branch October 16, 2023 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants