-
Notifications
You must be signed in to change notification settings - Fork 51
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
Rack 3 compatibility #153
base: master
Are you sure you want to change the base?
Rack 3 compatibility #153
Conversation
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.
Thank you @waltjones for the PR and for suggesting these change.
However... after reading through the changes, I wonder at the need for some of the changes.
Please correct me if I'm wrong.
We don't need rackup
at all, not as part of our test suit nor as part of our runtime. Adding this PR effectively adds rackup
as a requirement (as the gem will raise an exception when missing)... does it not?
Also, the main code (::Rack::Handler.register
...) seems to remain unchanged. So I wonder if simply adding the extra line wouldn't be better...?
::Rackup::Handler.register(:iodine, Iodine::Rack) if defined?(::Rackup::Handler)
lib/rack/handler/iodine.rb
Outdated
rescue StandardError | ||
end | ||
else | ||
raise "You must install the rackup gem when using Rack 3" |
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.
The rackup
gem is optional, no? why raise an error?
I am quite certain that the iodine
CLI can be called without rackup
installed.
@@ -24,10 +24,19 @@ def self.shutdown | |||
end | |||
end | |||
|
|||
ENV['RACK_HANDLER'] ||= 'iodine' |
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.
It is possible that iodine
will be included before the rackup
gem. In these cases having the ENV
set could be beneficial for the later inclusion of rackup
...
Let's leave this line outside the if
statement.
@boazsegev thank you for looking, and thank you for iodine. There are two relevant changes in Rack 3 that I'm aware of.
ENV['RACK_HANDLER'] ||= 'iodine'
ENV['RACKUP_HANDLER'] ||= 'iodine'
The
For me, that fails in an unexpected way (I don't remember now quite what the exception was), and it took the effort that led to this PR to get to the bottom of it. Providing the
I didn't test the CLI, but if it is using Rack 3 and executes |
Apologies, late follow up. I did some additional testing, and see the case for this file loading, with rack 3, without rackup. |
Rack 3 now has Rackup as a separate gem: rack/rack#1937
This PR detects whether the Rackup namespace is present, and supports both Rack 2 and Rack 3.
The conditional logic for this PR follows the pattern and rationale used here. puma/puma#3061