-
-
Notifications
You must be signed in to change notification settings - Fork 209
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
Zeitwerk Loader Implementation #123
Zeitwerk Loader Implementation #123
Conversation
Logger currently has issues. Not sure why yet, will investigate later. GoodJob.logger doesn't change, due to initializer not being triggered I suppose. |
@@ -1,6 +1,5 @@ | |||
# frozen_string_literal: true | |||
require 'rails_helper' | |||
require 'good_job/cli' |
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.
Not needed
good_job.gemspec
Outdated
@@ -43,6 +43,7 @@ Gem::Specification.new do |spec| | |||
spec.add_dependency "pg", ">= 1.0.0" | |||
spec.add_dependency "railties", ">= 5.1.0" | |||
spec.add_dependency "thor", ">= 0.14.1" | |||
spec.add_dependency "zeitwerk", ">= 2.2.2" |
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.
Not sure about the version range to depend on
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.
Probably >= 2.0
is fine. Looking at the gemspec some of those versions are probably arbitrarily and unnecessarily specific.
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.
changed
require "active_job" | ||
require "active_job/queue_adapters" |
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.
lib/good_job.rb
Outdated
require 'good_job/notifier' | ||
require "active_job" | ||
require "active_job/queue_adapters" | ||
require "good_job/railtie" |
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.
When not requiring the Railtie the initializers were not triggered in railtie.rb file due to loading orders.
To verify this assumption I did two things:
- Use
loader.eager_load
afterloader.setup
which worked. - Enable eager loading in test.rb file of spec/test_app which worked
Thus moved the railtie to good_job top requires. So it should be required first regardless of Zeitwerk
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 ran across this before in Zeitwerk being troublesome and did some Github searching.
I think this should happen at the bottom of the file after Zeitwerk is set up and after the root namespace is created. Here's an example: https://github.com/dukex/aggregated_record/blob/e8a597aaee94eb6e03ba0a26654a9c7f50a9c321/lib/aggregated_record.rb#L4-L13
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.
Changed
loader.inflector.inflect( | ||
'cli' => "CLI" | ||
) | ||
loader.push_dir(File.join(__dir__, ["generators"])) |
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.
Generator is not a namespace thus adding it to main directories
Found the issue. See PR code comments |
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.
This is amazing ✨ Thank you.
The only actionable comment I made was regarding the Railtie. Let me know if you think that's a good idea.
good_job.gemspec
Outdated
@@ -43,6 +43,7 @@ Gem::Specification.new do |spec| | |||
spec.add_dependency "pg", ">= 1.0.0" | |||
spec.add_dependency "railties", ">= 5.1.0" | |||
spec.add_dependency "thor", ">= 0.14.1" | |||
spec.add_dependency "zeitwerk", ">= 2.2.2" |
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.
Probably >= 2.0
is fine. Looking at the gemspec some of those versions are probably arbitrarily and unnecessarily specific.
lib/good_job.rb
Outdated
require 'good_job/notifier' | ||
require "active_job" | ||
require "active_job/queue_adapters" | ||
require "good_job/railtie" |
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 ran across this before in Zeitwerk being troublesome and did some Github searching.
I think this should happen at the bottom of the file after Zeitwerk is set up and after the root namespace is created. Here's an example: https://github.com/dukex/aggregated_record/blob/e8a597aaee94eb6e03ba0a26654a9c7f50a9c321/lib/aggregated_record.rb#L4-L13
#87