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

Merged
merged 7 commits into from
Sep 13, 2022
Merged

Rack 3 support #758

merged 7 commits into from
Sep 13, 2022

Conversation

lsylvester
Copy link
Contributor

@lsylvester lsylvester commented Sep 8, 2022

Add support for rack 3 by relaxing the version constraints and downcasing all of the headers.

Fixes #753

@@ -2,7 +2,7 @@

Get upgrade notes from Sprockets 3.x to 4.x at https://github.com/rails/sprockets/blob/master/UPGRADING.md

- Fix `Sprockets::Server` to return response headers to compatible with with Rack::Lint 2.0.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a reverting of the lower case headers which caused a performance issue - but this was resolved in rack 2.2.4 according to #746 (comment)

@skipkayhil
Copy link
Member

I think this will have the same issues as the commit that was reverted. If Sprockets want to support both Rack 2 and 3 then it will need to be able to handle both Case-Sensitive and downcase only headers.

@lsylvester
Copy link
Contributor Author

I think this will have the same issues as the commit that was reverted. If Sprockets want to support both Rack 2 and 3 then it will need to be able to handle both Case-Sensitive and downcase only headers.

Rack 2 has case insensitive headers so the downcase headers should be should still be valid. The issue that caused the regression was a bug in rack that was resolved by rack/rack#1919. We may wish to lock rack to >= 2.2.4 (which I have added)- but I think that may be overly restrictive as I think it was only a performance regression in test (and maybe dev) - but not production. Not sure if this (either changing the case of headers that may be accessed elsewhere, or the restriction of rack versions) could be considered a breaking change.

@skipkayhil
Copy link
Member

Rack 2 has case insensitive headers so the downcase headers should be should still be valid. The issue that caused the regression was a bug in rack that was resolved by rack/rack#1919

Rack fixed one instance of the issue, but as long as Sprockets claims compatability with Rack 2 it needs to work with any other library that supports Rack 2. I believe the casing of the headers needs to be dependent on the version of Rack, since we can't assume other libraries will have already updated.

@lsylvester
Copy link
Contributor Author

but as long as Sprockets claims compatability with Rack 2 it needs to work with any other library that supports Rack 2

I do not think that this is correct. Any library using Rack 2 could make any assumptions about the case of the header that it likes - so maybe sprockets is already incompatible with existing Rack 2 middleware. I think it is reasonable it argue that this could be considered a breaking change and that we need to bump the major version of sprockets - so that app maintainers would know to check for any incompatibilities - but it should still be able to support both Rack 2 and 3 using the lower case headers.

The decision to bump the major version is something that I will leave to the maintainers of the project and I will not presume what they would consider a breaking change.

@rafaelfranca
Copy link
Member

Yeah, this patch is fine since sprockets will require the version of rack to the update to 2.2.4 which allow headers to be head with both cases.

@rafaelfranca rafaelfranca merged commit e03fc2a into rails:main Sep 13, 2022
@ioquatix
Copy link

Thanks everyone!

@ghiculescu
Copy link
Member

There's a user reported issue on Rack 2.2.6 that seems related to this, could someone take a look? rails/rails#47096

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.

We cannot test on Rack 3
5 participants