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

Rack 3 compatibility #153

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Rack 3 compatibility #153

wants to merge 3 commits into from

Conversation

waltjones
Copy link

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

Copy link
Owner

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

rescue StandardError
end
else
raise "You must install the rackup gem when using Rack 3"
Copy link
Owner

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'
Copy link
Owner

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.

@waltjones
Copy link
Author

@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'] no longer works, for me at least, and ENV['RACKUP_HANDLER'] is needed. This can go outside the conditional, but in that case, both need to be added.

ENV['RACK_HANDLER'] ||= 'iodine'
ENV['RACKUP_HANDLER'] ||= 'iodine'

::Rack::Handler.register did still work for me, but not with string arguments.

::Rack::Handler.register(:iodine, Iodine::Rack) works, but in the Rack 3 case I used ::Rackup::Handler.register because I don't expect the ::Rack::Handler version of this to be available much longer.

The raise is present for the case where:

  • Rack 3 is used,
  • Rackup is not present
  • ::Rack::Handler.register is called with string arguments, and
  • ENV['RACKUP_HANDLER'] is not set.

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 raise will save people some time if they find themselves using Rack 3 without Rackup, and hit this code path.

I am quite certain that the iodine CLI can be called without rackup installed.

I didn't test the CLI, but if it is using Rack 3 and executes ::Rack::Handler.register, I think it will need Rackup.

@waltjones
Copy link
Author

Apologies, late follow up.

I did some additional testing, and see the case for this file loading, with rack 3, without rackup.

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.

None yet

2 participants