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 support #1857

Merged
merged 29 commits into from Jan 5, 2024
Merged

Rack 3 support #1857

merged 29 commits into from Jan 5, 2024

Conversation

dentarg
Copy link
Member

@dentarg dentarg commented Dec 30, 2022

Adds support for Rack 3, drops support for Rack 2.

Close #1797

See the individual commits for various details.

Old comment from Dec, 2022

There's one failing test left to address (besides async tests that I have disabled for now, that area is being discussed a bit in #1853): EDIT: behaviour changed in Rack 3: rack/rack#1989

it "handles params without a value" do
mock_app {
get '/' do
assert_nil params.fetch('foo')
"Given: #{params.keys.sort.join(',')}"
end
}
get '/?foo'
assert_equal 'Given: foo', body
end

Opening as draft until we have addressed that – and also until I've taken this for a spin with a real app – something for next year! Happy New Year Sinatra users 🥳


This work was sponsored by 84codes (CloudAMQP, CloudKarafka, ElephantSQL, CloudMQTT)

@dentarg
Copy link
Member Author

dentarg commented Dec 30, 2022

There's one failing test left to address

EDIT: behaviour changed in Rack 3: rack/rack#1989


Some links about it:

Re: async/streaming, https://github.com/dentarg/sinatra/actions/runs/3807969308/jobs/6478153396 is a test run before I reverted 4210184 if anyone are curious

.github/workflows/test.yml Outdated Show resolved Hide resolved
@jkowens
Copy link
Member

jkowens commented Dec 31, 2022

Thank you @dentarg and 84codes 🤗

@ioquatix
Copy link
Contributor

It would be great to have a 3.x release that supports Rack 3+

@dentarg
Copy link
Member Author

dentarg commented Jan 21, 2023

@ioquatix you mean a release that supports both Rack 2 and Rack 3?

@ioquatix
Copy link
Contributor

Yeah,

You can use rack-session and rackup gems if those are dependencies, the v1 release of those gems is for Rack v2 and the v2 release of those gems is for Rack 3+.

@dentarg
Copy link
Member Author

dentarg commented Jan 21, 2023

We'll see. Sounds like (a lot?) more effort.

@ioquatix
Copy link
Contributor

Maybe check this for ideas about how to be compatible with both Rack 2 and Rack 3. The biggest one is dealing with headers. If you don't want to force people to use lower case headers, you can use Rack::Headers.

rails/rails#46594 (comment)

@ioquatix
Copy link
Contributor

Looking good.

@dentarg
Copy link
Member Author

dentarg commented Feb 25, 2023

@ioquatix I wanted to make the tests pass on "rack head" (main branch) too, but rack/rack@1bd0f15 makes a few tests fail (for sinatra and sinatra-contrib). Not sure how I should handle that. I suspect that change in Rack will be considered a breaking change? Will it be included in Rack 3.1?

@ioquatix
Copy link
Contributor

It will be included in Rack 3.1 - it's not breaking in the sense that it's recommended to use that mime type. The previous mime type is now considered legacy/obsolete, but browsers would handle them both as meaning the same thing. Your test should accept both.

dentarg added a commit to dentarg/sinatra that referenced this pull request Feb 25, 2023
Next Rack release (probably 3.1) will include this change:

rack/rack@1bd0f15
sinatra#1857 (comment)
dentarg added a commit to dentarg/sinatra that referenced this pull request Mar 4, 2023
Next Rack release (probably 3.1) will include this change:

rack/rack@1bd0f15
sinatra#1857 (comment)
@dentarg dentarg marked this pull request as ready for review March 4, 2023 22:52
dentarg added a commit to dentarg/sinatra that referenced this pull request Mar 5, 2023
Next Rack release (probably 3.1) will include this change:

rack/rack@1bd0f15
sinatra#1857 (comment)
@dentarg
Copy link
Member Author

dentarg commented Mar 5, 2023

Not sure this fail is related to the changes here, maybe more tweaks are needed? Re: #1887 (comment)

1) Failure:
IntegrationAsyncTest#test_with_puma_streams_async_0 [/home/runner/work/sinatra/sinatra/test/integration_async_test.rb:[21](https://github.com/sinatra/sinatra/actions/runs/4336782825/jobs/7572318485#step:5:22)]:
--- expected
+++ actual
@@ -1 +1 @@
-["hi!", "hello"]
+["hi!", "hello", "hi!", "hello"]

Locally, it works for me to run the sinatra tests on truffle ruby:

docker run --rm -it -v $(pwd):/app -w /app ghcr.io/graalvm/truffleruby:debian-22.3.1 bash
apt-get update && apt-get install --yes \
          git \
          pandoc \
          nodejs \
          pkg-config \
          libxml2-dev \
          libxslt-dev \
          libyaml-dev
bundle
bundle exec rake

@dentarg
Copy link
Member Author

dentarg commented Mar 5, 2023

Not sure this fail is related to the changes here

The re-run passed: https://github.com/sinatra/sinatra/actions/runs/4336782825/jobs/7572670654

Copy link
Member

@zzak zzak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!! Thank you so much 🍡

@zerbinidamata
Copy link

Hey all, is this close to being released?

@dentarg
Copy link
Member Author

dentarg commented Mar 14, 2023

Depends on what's "close"

I think there's a discussion that needs to happen about dropping Rack 2 support

Also, I will be busy the coming the weeks

@dentarg
Copy link
Member Author

dentarg commented Mar 14, 2023

@zerbinidamata have you tested this in a real world application? if not, can you?

@dentarg
Copy link
Member Author

dentarg commented Mar 16, 2023

Re: my comment from December:

There's one failing test left to address
EDIT: behaviour changed in Rack 3: rack/rack#1989

it "handles params without a value" do
mock_app {
get '/' do
assert_nil params.fetch('foo')
"Given: #{params.keys.sort.join(',')}"
end
}
get '/?foo'
assert_equal 'Given: foo', body
end

Just saw rails/rails#47652 and rack/rack#2052 (apparently released with Rack v3.0.6)

dentarg and others added 23 commits January 5, 2024 10:32
Multiple response header values are encoded using an Array instead of
newlines: https://github.com/rack/rack/blob/v3.0.3/UPGRADE-GUIDE.md#multiple-response-header-values-are-encoded-using-an-array

Rack 3 does not remove cookies from the internal storage (because it
doesn't make much sense), see rack/rack#1844, rack/rack#1840
Co-authored-by: Samuel Williams <samuel.williams@oriontransfer.co.nz>
Revert "Run tests for sinatra-contrib and rack-protection"

This reverts commit 13b5b0f.
The 2.0 series of these games are the same as the 0.x series, as these
gems were created for Rack 3.
The 1.x series of these games are for Rack 2.
Rebase gone wrong.

Remove the old comment from 1dfae3d.
Next Rack release (probably 3.1) will include this change:

rack/rack@1bd0f15
sinatra#1857 (comment)
Co-authored-by: Eloy Pérez <ej.perezgomez@gmail.com>
No point in using the beta version anymore.
The tests shows this after dcdebe9

Sinatra does not (yet?)
Tried adding the same code the regular Gemfile has for rack head, but
than I ran into

    /usr/local/bundle/gems/bundler-2.4.22/lib/bundler/source/git/git_proxy.rb:354:in
    `allowed_with_path': The git source https://github.com/rack/rack.git
    is not yet checked out. Please run `bundle install` before trying to
    start your application (Bundler::GitError)

So it seems we need to bundle before trying to start the app, and that
just feels too complicated.

The thought of using bundler/inline has crossed my mind, maybe I will
try it at some point.
truffleruby and jruby uses the childprocess with
CHILDPROCESS_POSIX_SPAWN, and when doing so, the environment variables
aren't cocered to strings until very late:
https://github.com/enkessler/childprocess/blob/v4.1.0/lib/childprocess/unix/posix_spawn_process.rb#L109-L130

On CRuby it seems symbol environment variables will overwrite any
existing (string) ones but that does not happen on JVM rubies (I've only
tested with TruffleRuby though, but JRuby builds also failed in CI)
@dentarg dentarg merged commit 8a17d4b into sinatra:main Jan 5, 2024
24 checks passed
@dentarg dentarg deleted the rack-3 branch January 5, 2024 10:15
@dentarg dentarg mentioned this pull request Jan 5, 2024
@ioquatix
Copy link
Contributor

ioquatix commented Jan 6, 2024

Wow, nice work!

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.

Support rack 3
10 participants