-
Notifications
You must be signed in to change notification settings - Fork 69
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
Add Rack 3 compatibility #146
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -66,7 +66,11 @@ def responsive? | |
end | ||
|
||
def run_default_server(app, port) | ||
require 'rack/handler/webrick' | ||
begin | ||
require 'rack/handler/webrick' | ||
rescue LoadError | ||
require 'rackup/handler/webrick' | ||
end | ||
Rack::Handler::WEBrick.run(app, **webrick_opts) do |server| | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hey, I assume this is causing you issues as it stands? happy to consider an additional pull request. it may be worth tracking your particular problem as a new issue for visibility There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ive just seen the issue and pr now, thanks! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at this now, it looks wrong / incomplete to me too, but we're running it on Rack 3 successfully and it worked when I tested it last year. Ah, I see on the new PR, it's only Rack 3.1 where this is an issue. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yep sorry should have followed up with the issue we've created for reference #151 |
||
@port = server[:Port] | ||
end | ||
|
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.
Given that we have rackup declared as a dependency now, do we need to use this approach, or could we go straight to requiring rackup?