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

Don't force eager load for Rails 5 #480

Merged
merged 1 commit into from
Mar 25, 2018

Conversation

tahirpoduska
Copy link

Addresses #467

@phstc
Copy link
Collaborator

phstc commented Mar 13, 2018

Hi @tap87

Thanks for the PR!

I will give it some testing this week.

Have you tested with/without a daemon -d?

@@ -66,8 +66,10 @@ def load_rails
else
# Painful contortions, see 1791 for discussion
require File.expand_path('config/application.rb')
::Rails::Application.initializer 'shoryuken.eager_load' do
::Rails.application.config.eager_load = true
if ::Rails::VERSION::MAJOR == 4
Copy link
Collaborator

@phstc phstc Mar 13, 2018

Choose a reason for hiding this comment

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

@tap87 for keeping compatibility, I'm wondering if instead of checking for Rails 4, we should check for Rails greater than or equals to 5, so all the previous versions, would continue to work in the same they are now, given the issue seems to be only with Rails 5 and greater.

BTW I'm using shoryuken with Rails 5.0.2 and it's working fine.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, we could take that approach. I tried to follow sidekiq here since that is were the existing code was adapted from. https://github.com/mperham/sidekiq/blob/master/lib/sidekiq/cli.rb#L261

@tahirpoduska
Copy link
Author

I have tested it both with and without the daemon option, and it works fine

@tahirpoduska
Copy link
Author

@phstc Any updates on this PR. This is how sidekiq does it. Resque also checks for app config instead of forcing eager load. I also checked que and they also don't force eager_load in Rails 5

@phstc
Copy link
Collaborator

phstc commented Mar 20, 2018

Hi @tap87

Sorry, I've been very busy lately, but I added this to my list to check this week. If everything works well, I will merge and release this week.

@phstc phstc merged commit bbfa813 into ruby-shoryuken:master Mar 25, 2018
@phstc
Copy link
Collaborator

phstc commented Mar 25, 2018

@tap87 3.2.3 is out with your fix 🎉 Great job, thanks 🍻

@tahirpoduska
Copy link
Author

Thanks @phstc

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.

2 participants