Headers respond with the correct version #80
Conversation
In issue stitchfix#70 we see that response headers do not respond with the correct version. This adds a rack middleware that overrides the Content-Type header to show the version. Co-authored-by: Juan Carlos Ruiz<[email protected]>
|
@Aliciawyse thank you for this PR. I am requesting more information from @nicolaa on the issue. Will circle back with you soon. |
|
Thanks @brettfishman! |
|
Hi again, @Aliciawyse - apologies for the delay. I have been considering the issue your PR addresses, and conferring with a few folks about it (see my comment here). I'm not sure we want to solve the issue your PR addresses at this point, but I wanted to give you feedback on your solution as it looks like you are newer to Ruby development - welcome! :) |
| @@ -1,3 +1,3 @@ | |||
| module Stitches | |||
| VERSION = '3.7.3' | |||
| VERSION = '3.8.3' | |||
There was a problem hiding this comment.
Our pull request template contained some outdated instructions for updating the VERSION, so doing this is unnecessary. If this PR is approved and merged, one of the maintainers would take care of this when releasing a new version of this gem.
On a side note, the version here should be 3.8.0 following SEMVER guidelines. The patch portion of the version (the last digit) gets reset to 0. If this had been a major version change, it would be 4.0.0.
| protected | ||
| def do_call(env) | ||
| status, headers, body = @app.call(env) | ||
| headers["Content-Type"] = env["CONTENT_TYPE"] |
There was a problem hiding this comment.
This was one of our main concerns with this solution. This overwrites the response header for Content-Type with a value that we believe is non-standard. For example, what was returned as:
Content-Type: application/json; charset=utf-8
would become
Content-Type: application/json; version=x, overwriting charset=utf-8. According to RFC 7231 Section 3.1.1.5, this appears to be a non-standard value for this header.
|
Since we decided not to fix #70 at this time, I'm going to close down this PR. |
Problem
This resolves #70. Response headers do not respond with the correct version.
Solution
This adds a rack middleware that overrides the Content-Type header to show the version.
Checklist
Before Merging
version.rbfollowing SemVer on this branch (if you were testing an RC, make sure you remove the RC before merging)After Merging
masterlocally and runrake releaseonmasterto release the new version on Gemfury