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.

Sign up for GitHub

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

Jump to bottom

Allow rack >= 3 in Rails. #46594

Closed
wants to merge 1 commit into from
Closed

Allow rack >= 3 in Rails. #46594

wants to merge 1 commit into from

Conversation

ioquatix
Copy link
Contributor

@ioquatix ioquatix commented Nov 27, 2022 •
edited

Allow Rack 3 to be used in Rails.

Blocked By:

@rails-bot rails-bot bot added the actionpack label Nov 27, 2022
@casperisfine
Copy link
Contributor

You'll have to bundle update rack to see if the test suite passes.

As for the failure you see, it was fixed on main, so you can rebase.

@yahonda
Copy link
Member

yahonda commented Nov 28, 2022

There is a similar pull request #45741 Please consider closing one of them.

@p8
Copy link
Member

p8 commented Nov 28, 2022

Wouldn't this also allow rack 4?

@ioquatix ioquatix mentioned this pull request Dec 5, 2022
Closed
@rafaelfranca rafaelfranca added this to the 7.1.0 milestone Dec 5, 2022
@ioquatix ioquatix mentioned this pull request Dec 17, 2022
Merged
@ioquatix ioquatix force-pushed the rack-3 branch 6 times, most recently from f5896de to f87c7c5 Compare January 10, 2023 11:03
simi
simi reviewed Jan 10, 2023
View reviewed changes
Gemfile.lock Outdated Show resolved Hide resolved
@ioquatix ioquatix force-pushed the rack-3 branch 2 times, most recently from d440fbb to 0c66e84 Compare January 12, 2023 08:54
@ioquatix
Copy link
Contributor Author

On my local dev machine, just considering actionpack, I've got the test suite down to 27 failures and 2 errors.

@ioquatix
Copy link
Contributor Author

One thing I could use some help with is adding Rack 2 and Rack 3 to the test matrix. It is probably only strictly required for actionpack but maybe others wouldn't hurt.

@ioquatix ioquatix force-pushed the rack-3 branch 4 times, most recently from 1d6c828 to 9be1e57 Compare January 14, 2023 09:25
@ioquatix ioquatix force-pushed the rack-3 branch 2 times, most recently from 854256d to 7cc2592 Compare February 15, 2023 01:29
@drnic
Copy link
Contributor

drnic commented Feb 15, 2023

Great talk at RubyConfAU today @ioquatix! Looking forward to my rails 7.1 app being more awesome. Thanks for all the work!

@ioquatix
Copy link
Contributor Author

A related issue: #47456

@ioquatix

This comment was marked as outdated.

Sign in to view
@ioquatix ioquatix force-pushed the rack-3 branch 2 times, most recently from 6eb7f48 to 92afa7a Compare March 13, 2023 00:37
@ioquatix
Copy link
Contributor Author

ioquatix commented Mar 13, 2023 •
edited

Outstanding PRs/Issues

@domharrington domharrington mentioned this pull request Mar 13, 2023
Merged
@ioquatix ioquatix force-pushed the rack-3 branch 2 times, most recently from 92afa7a to fdf34d0 Compare March 16, 2023 03:06
@JoeDupuis JoeDupuis mentioned this pull request May 7, 2023
Merged
4 tasks
JoeDupuis
JoeDupuis reviewed May 7, 2023
View reviewed changes
actionpack/test/controller/test_case_test.rb

post :render_body, params: params.dup
post :render_body, body: body
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think setting the body directly is the right fix here.

This would bypass the body_stream branch in ActionDispatch::Request#body and there is already have a test for it.

This is currently failing on CI, so I tried a different fix and submitted a PR targeting main to bring back CI to green.

actionpack/test/controller/test_case_test.rb

assert_equal params.to_query, @response.body
assert_equal body, @response.body
Copy link
Contributor

Choose a reason for hiding this comment

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

This test pass for me on main with Rack 3.0
Unless I am missing something, I don't think we need to change it anymore.

This was referenced May 9, 2023
Closed
Closed
@ioquatix
Don't assume post with params will preserve input body.
9b1f88a 
Rack 3 drops the requirement for input bodies to be rewindable, and
params, once read, may not be available for "raw_post". Update the tests
to prefer to use raw "body:" rather than "params:" which can be consumed
by Rack::Request in Rack 3.
@ioquatix
Copy link
Contributor Author

ioquatix commented Jun 13, 2023 •
edited

Okay, as far as I'm concerned, this is now all working the best it can be for a 7.1 release. Thanks everyone for your huge effort and patience. It was truely a multi-year project.

@zzak I'd also recommend we make the rack-3 parts of the CI non-optional if they aren't already.

@ioquatix ioquatix closed this Jun 13, 2023
@ioquatix ioquatix deleted the rack-3 branch June 13, 2023 03:41
@ioquatix
Copy link
Contributor Author

ioquatix commented Jun 13, 2023 •
edited

Summary of all work done in rails:

@piradata
Copy link

piradata commented Jul 5, 2023

The last version of actionpack (7.0.6) still wants rack to be ~> 2.0, >= 2.2.4.
Do you guys know if there is any open PR on actionpack about changing this?

@skipkayhil
Copy link
Member

The last version of actionpack (7.0.6) still wants rack to be ~> 2.0, >= 2.2.4. Do you guys know if there is any open PR on actionpack about changing this?

The work to support Rack 3 has been done on the main branch, so it will be released as Rails 7.1

@adrianna-chang-shopify adrianna-chang-shopify mentioned this pull request Jul 27, 2023
Merged
4 tasks
@jrreed jrreed mentioned this pull request Sep 18, 2023
Open
@cynthia-anya cynthia-anya mentioned this pull request Feb 1, 2024
Closed
atosbucket added a commit to atosbucket/turbo-rails that referenced this pull request Apr 11, 2024
@atosbucket
Use Rack version < 3 on edge Rails (#431)
31eabdc 
There is some test failure when running against edge Rails, due to the
dependencies installing Rack 3 which is [not yet supported][1]. Pinning
our Rack version prevents the failures.

Once the linked Rails PR has landed, we can remove this constraint.

[1]: rails/rails#46594
newheaven918 added a commit to newheaven918/turbo that referenced this pull request Apr 26, 2024
@newheaven918
Use Rack version < 3 on edge Rails (#431)
9d22aef 
There is some test failure when running against edge Rails, due to the
dependencies installing Rack 3 which is [not yet supported][1]. Pinning
our Rack version prevents the failures.

Once the linked Rails PR has landed, we can remove this constraint.

[1]: rails/rails#46594
luisjose1996 added a commit to luisjose1996/Turbo-Rails that referenced this pull request May 10, 2024
@luisjose1996
Use Rack version < 3 on edge Rails (#431)
18a674d 
There is some test failure when running against edge Rails, due to the
dependencies installing Rack 3 which is [not yet supported][1]. Pinning
our Rack version prevents the failures.

Once the linked Rails PR has landed, we can remove this constraint.

[1]: rails/rails#46594
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeremyevans jeremyevans jeremyevans left review comments

@rafaelfranca rafaelfranca rafaelfranca left review comments

@simi simi simi left review comments

@JoeDupuis JoeDupuis JoeDupuis left review comments

@casperisfine casperisfine casperisfine left review comments

Assignees
No one assigned
Labels
actioncable actionmailbox actionmailer actionpack actionview railties
Projects
None yet
Milestone
7.1.0
Development

Successfully merging this pull request may close these issues.

None yet