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 #682

Merged
merged 7 commits into from
Apr 23, 2024
Merged

Rack 3 compatibility #682

merged 7 commits into from
Apr 23, 2024

Conversation

tomasc
Copy link
Contributor

@tomasc tomasc commented Mar 31, 2024

Add compatibility with Rack 3 (while maintaining Rack 2 support).

  • down-case headers
  • call .bytesize method support on Rack::Files::Iterator (if available) to determine content-length
  • update test suite accordingly
  • patch Rack::TestApp to avoid multiple calls to body
  • add Rack::SERVER_PROTOCOL env to Rack::TestApp to pass Rack::Lint

supersedes #660 and #661

@jrochkind
Copy link
Contributor

(I re-discovered I have privs to authorize you to run CI, so I have done so! I would not merge without hearing from @janko , and do not have rubygems release privs!)

@jrochkind
Copy link
Contributor

Looks like failing on jruby for some reason, I haven't looked into it.

@jrochkind
Copy link
Contributor

As a result of this PR, CI would run only on rack 3.

How important does anyone think it is to run on both rack 2 and rack 3? @tomasc ? I suppose to do that we might use appraisal to have alternate gemfiles, and github CI's matrix feature to run both. I have done this before in other projects.

@tomasc
Copy link
Contributor Author

tomasc commented Mar 31, 2024

@jrochkind yes, I was thinking about running tests on both Rack 2 & 3 would be great. Feel free to update the PR accordingly. Thanks!

@jrochkind
Copy link
Contributor

I am not sure what time I have, but thank you, you also feel free of course!

@tomasc
Copy link
Contributor Author

tomasc commented Apr 1, 2024

@jrochkind ok this was not too difficult.

– added Appraisals for Rack 2 & 3
– removed tests for Ruby 2.3 (for simplicity's sake, since Rack 3 requires Ruby >= 2.4)
– added testing on Ruby 3.3

Can you please trigger the CI run?

@tomasc
Copy link
Contributor Author

tomasc commented Apr 1, 2024

@jrochkind can you please run it again, we were missing bundle exec appraisal install

@jrochkind
Copy link
Contributor

I thought once I approved it once, you'd be able to keep running it, but apparently not!

@tomasc
Copy link
Contributor Author

tomasc commented Apr 1, 2024

@jrochkind yeah :( sorry. Once more please, I forgot to ignore the appraisal lockfiles …

@tomasc
Copy link
Contributor Author

tomasc commented Apr 1, 2024

@jrochkind ok, close, only the jruby tests fail now. I have no idea how to fix those errors though …

@jrochkind
Copy link
Contributor

Feels likely unrelated to this PR, a pre-existing jruby error? What do you think?

We'll need SOME kind of communication or go-ahead from @janko to proceed regardless. While I have privs to "merge" from years ago, I've never used it before, so won't without hearing from janko. (Plus I don't actually have rubygems release rights anyway).

@tomasc
Copy link
Contributor Author

tomasc commented Apr 1, 2024

Well, let's try to get the jruby fixed -- we can google around a bit.

And indeed, we need @janko or a new maintainer.

This gem is so great and I believe theres quite a few people invested in it with their apps (as much as I am). Even if we end up just maintaining it in its current scope.

@janko
Copy link
Member

janko commented Apr 4, 2024

Apologies for my lack of activity, it's been harder to find time ever since I got a kid 👶🏻

This looks pretty good. One thing I would like remove is the duplication in how header values are resolved. Maybe we can extract the conditional downcasing of headers somewhere and apply it in different places. Or do something like:

headers = headers.transform_keys(&:downcase) if Rack.release > "3"

Also, Appraisal is probably not necessary here, we could just run the test suite with an environment variable, and read it in the Gemfile to specify the Rack version. Or just not run tests on Rack 2 in CI, if we minimize the duplication, then there is minimal chance of breakage.

@janko
Copy link
Member

janko commented Apr 4, 2024

BTW, after this PR I should really get rid of rack-test_app in favor of rack-test, now that Jeremy Evans took over maintenance it got a lot better.

@jrochkind
Copy link
Contributor

Thanks @janko, nice to see you! (I still offer my help as co-maintainer, to help support minimal maintenance and compatibility PR's! I am happy to have a phone call or chat to discuss some time)

It was me that recommended appraisal -- From maintaining other gems that I test with multiple versions of dependencies, in which I started out trying to just use conditionals in the Gemfile, I have found that there can be a variety of weird edge case problems that occur when doing this the "brute" way that using Appraisal resolve, and I've been very happy with appraisal.

BUT your call, if you prefer doing it the other way, sorry for misdirecting you @tomasc !

@adam12
Copy link

adam12 commented Apr 9, 2024

Also, Appraisal is probably not necessary here, we could just run the test suite with an environment variable, and read it in the Gemfile to specify the Rack version. Or just not run tests on Rack 2 in CI, if we minimize the duplication, then there is minimal chance of breakage.

Another approach is a folder of gemfiles and then using the Github Actions matrix feature to set BUNDLER_GEMFILE env var.

@jrochkind
Copy link
Contributor

Another approach is a folder of gemfiles and then using the Github Actions matrix feature to set BUNDLER_GEMFILE env var.

Appraisal is fairly light implementation of API on top of that to do it in a standard and transparent way (while also combining your main Gemfile in). But whatever @janko prefers is fine with me!

@hms
Copy link
Contributor

hms commented Apr 20, 2024

@tomasc

I'm confirming that I've migrated off of my mutant branch on to this one and everything seems to be working as expected.

Thank you for the time and effort to resolve these issues.

refactor to
`headers = headers.transform_keys(&:downcase) if Rack.release >= "3"`
@tomasc
Copy link
Contributor Author

tomasc commented Apr 23, 2024

@jrochkind @janko @hms I have:

  • updated the header downcase syntax per @janko's recommendation
  • updated the activerecord-jdbcsqlite3-adapter in hope it might resolve the jruby tests that error out

@hms thanks for testing! I run this branch on several of my apps for the 3 or so weeks, and all seems good as well.

@jrochkind can you please trigger the tests?

@janko if you prefer something else than the appraisal gem please let me know which option exactly, I can try to update the setup. But I have to say the appraisal was super simple to install, clear and lightweight. I'd vote for keeping it.

In any case, it would be great to get this merged and released soon. Thanks everyone!

@janko
Copy link
Member

janko commented Apr 23, 2024

@tomasc I appreciate the update 🙏🏻 It seems that JRuby is still failing, because of mismatch between Active Record versions. I will merge this, and correct that afterwards. I will also probably replace Appraisal with environment variables.

@janko janko merged commit 5231c36 into shrinerb:master Apr 23, 2024
16 of 18 checks passed
@tomasc
Copy link
Contributor Author

tomasc commented Apr 23, 2024

Thanks @janko – fantastic!

I am also very happy to see you around – Shrine is such a fantastic library (and the documentation!!), it would be such a pity to lose it.

@jrochkind
Copy link
Contributor

Thank you @janko , we really appreciate it! A release soon-ish including rack 3 support would be awesome too!

@janko
Copy link
Member

janko commented Apr 29, 2024

Just released Shrine 3.6.0 with Rack 3 compatibility.

@jrochkind
Copy link
Contributor

Thank you!!

@davidalejandroaguilar
Copy link

Seems like this gem just got a breath of fresh air, glad we got this issue fixed.

Hopefully my PRs provided some help in any way, thanks for the release!

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.

6 participants