Fixup code for use with rackup gem (may be used with rack 3) #3061
Conversation
| module Rack::Handler | ||
| def self.default(options = {}) | ||
| Rack::Handler::Puma | ||
| if Object.const_defined? :Rackup |
There was a problem hiding this comment.
Although this works, I think testing for Rack version 3 is more meaningful, because this tells the reader that this was implemented in response to a change in the new Rack version.
There was a problem hiding this comment.
Sorry, I thought I responded. The rackup namespace needs to be defined. The Rack v3 gem could be loaded, but unless the rackup gem is also loaded, this will fail. I added a comment.
lib/rack/handler/puma.rb
Outdated
| else | ||
| require 'rack/handler' | ||
| ::Rack::Handler | ||
| end.module_eval <<'HEREDOC' |
There was a problem hiding this comment.
To get rid of this module_eval we could create a new lib/rackup/handler.rb file that is required here. The file could either duplicate the code, or we are putting the code inside a module and are including this new module inside the new rackup/handler and rack/handler 🤔
There was a problem hiding this comment.
The cleanest way is with module_eval, but that also 'hides' the code from RuboCop, etc. I changed the code to use include. See the commit 'Update puma.rb, use include instead of module_eval'.
RubyGems had a somewhat similar issue, they extracted the code to a Ruby file that wasn't required (but was picked up by doc systems), and loaded a string with the file contents, then eval'd the string...
|
A note for those who haven't looked at rack/rackup code, as of the current version of rackup (2.0.0), the code by default tries |
| end | ||
| end | ||
| else | ||
| raise "Rack 3 must be used with the Rackup gem" |
There was a problem hiding this comment.
Error message could be something like you must install the rackup gem., i.e. tell the user what to do exactly
There was a problem hiding this comment.
Updated, see commit 'Changes per comments'
|
|
||
| register :puma, Puma | ||
| # rackup was removed in Rack 3, it is now a separate gem | ||
| if Object.const_defined? :Rackup |
There was a problem hiding this comment.
Can we just combine this with what's in rack_default? 🤔
There was a problem hiding this comment.
No. As mentioned above, Rackup currently will look for several servers, and Puma is the first one. It determines if Puma is available by requiring rack/handler/puma.
For info on rack_default, see #194.
test/test_rack_handler.rb
Outdated
| require "rack/handler/puma" | ||
| module TestRackUp | ||
| if Rack::RELEASE < '3' | ||
| require "rack/handler/puma" |
There was a problem hiding this comment.
this can be moved out of the if statement because you use it in both branches
There was a problem hiding this comment.
Updated, see commit 'Changes per comments'
| else | ||
| require "rackup" | ||
| require "rack/handler/puma" | ||
| RACK_HANDLER_MOD = ::Rackup::Handler |
There was a problem hiding this comment.
Maybe we could just test Puma::RackHandler directly for most of the tests instead?
There was a problem hiding this comment.
Two methods are called in RACK_HANDLER_MOD, register and get. If we want to verify that things work with Rack/Rackup, it would be good to continue doing so...
MSP-Greg
force-pushed
the
00-rackup-2
branch
from
8ae6e7b
to
afb6bb5
Compare
|
Rebased |
|
Just FYI, I expect the rackup gem to evolve more rapidly than it has before. While Rack v2 and Rackup gems are currently aligned, I imagine the handler implementation will diverge in the future (or be extended I compatible ways which are not part of the Rack 2 implementation). |
The constant Rack::RELEASE was introduced only in rack version 2 (specifically: rack/rack@a2fe30a) Earlier versions provided the class method Rack.release which still exists. puma#3061 has introduced usage of Rack::RELEASE and therefore broke compatibility with rack 1.6.X.
* Use Rack.release over Rack::RELEASE The constant Rack::RELEASE was introduced only in rack version 2 (specifically: rack/rack@a2fe30a) Earlier versions provided the class method Rack.release which still exists. #3061 has introduced usage of Rack::RELEASE and therefore broke compatibility with rack 1.6.X. * test.yaml, Gemfile - allow Rack 1.6.x CI * lib/rack/handler/puma.rb - allow Rack 1.6.x * test_rack_server.rb - allow testing with Rack 1.6.x * test_puma_server_hijack.rb - allow testing with Rack 1.6.x --------- Co-authored-by: MSP-Greg <Greg.mpls@gmail.com>
Description
Rack v2 included the files needed to use
rackupfrom the command line. With Rack v3, it is now a separate gem, and the namespace has changed.Re tests, when Rack v3 was released, we (ok, I) blocked test/test_rack_handler from running with Rack v3, so tests do exist. This PR unblocks the tests.
The changes to
lib/rack/handler/puma.rblook excessive. This PR is taking the previous code, and turning it into a HEREDOC, so it can be used inmodule_eval. This is the work-around to allow it to be placed in either theRack::Handleror theRackup::Handlernamespace.Adjust code to work with both.
Note: labeled as a 'bug', not sure...
Closes #3057
Your checklist for this pull request
[ci skip]to the title of the PR.#issue" to the PR description or my commit messages.