-
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
Fix Rack 3 support #152
Fix Rack 3 support #152
Conversation
Now Rack::Handler has been moved to a seperate gem, we need to specify it specifically. I’ve monkeypatched `Rack::Handler::WEBrick` to support both use cases everywhere in the gem
As of Rack `3.1.0`, `rack.input` is now optional, so cannot always be guaranteed to be present (https://github.com/rack/rack/blob/b4c92944e0dd04874bed36281fc8e1a44023677f/CHANGELOG.md?plain=1#L40)
This ensures we’re backwardsly compatible with both versions of Rack.
thanks for the pr @pezholio i’m travelling at the minute and just about to jump on a plane but will look at this probably Monday unless i get some time at the weekend |
Cool, thanks! 👍 |
Thanks for adding tests to cover both rack 2.x and 3.x. I can confirm the issue only occurs in rack 3.1.x which is why it didn't crop up before as part of #146 |
yarggg I've gotta double revert, because I don't have a changelog message for release and I don't want to force push on the main branch. |
Hey, So I've got a couple of minor issues I think
I wonder if we need to relax the restriction on the latest gem to 2.x up to 4.x of Rack, without the use on an env var? That would mean there is a dep on rackup 2.x though |
Fixes #151
As of Rack > 3.1.0, the previous fixes for
Rack::Handler::WEBrick
not being found have been removed. Additionally,rack.input
is now optional, so we can't guarantee thebody
variable is there when reading the request in the mock service.I've also updated the CI to run with both Rack
2.x.x
and3.x.x
, so we can guarantee backwards compatibility. I imagine if we stopped support for Rack2.x.x
completely this would cause issues with dependencies for some users.