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

Add brotli support #134

Merged
merged 1 commit into from
Aug 17, 2017
Merged

Add brotli support #134

merged 1 commit into from
Aug 17, 2017

Conversation

jabley
Copy link
Contributor

@jabley jabley commented Mar 30, 2016

Brotli is a new open-source compression algorithm from Google which can
give more dense compression, while compression and decompression speeds
are roughly the same.

Brotli responses are requested by setting Accept-Encoding: br.

Transferring less data over the network will typically result in a
faster experience for users.

This change adds support for Brotli decompression of responses.

jabley added a commit to jabley/our-boxen that referenced this pull request Mar 30, 2016
@iMacTia
Copy link
Member

iMacTia commented Jul 27, 2017

Hi @jabley, I'm really sorry your PR went ignored for so long!
I'm now in the process of catching up and this PR looks interesting but we have some issues with ruby < 2.0.
In order to get this merged in the next release, I'd need support for at least ruby 1.9.3. Is there any backport you can add to your PR to make tests passing on ruby 1.9.3 (please rebase with master to get rid of ruby 1.8.7 tests)

@iMacTia iMacTia modified the milestone: 0.12.0 Jul 27, 2017
@jabley
Copy link
Contributor Author

jabley commented Jul 29, 2017

I'll see if the brotli gem will accept a pull request to support < Ruby 2.x.

jabley added a commit to jabley/our-boxen that referenced this pull request Jul 29, 2017
Want to see if they'll take a PR which adds 1.9 support. See
lostisland/faraday_middleware#134
@jabley
Copy link
Contributor Author

jabley commented Jul 29, 2017

miyucy/brotli#23

If there are problems with that, what's your support policy on 1.9.3? It's been EOLed for quite a while.

@iMacTia
Copy link
Member

iMacTia commented Jul 29, 2017

The idea is that we keep supporting as many ruby versions as possible, depending on how this impact our work. Reality is that even though 1.9.3, 2.0 and 2.1 have been discontinued, there are still many "legacy" applications using them.

I understand brotli support is a nice-to-have feature, but it's hard to justify the loss of support for some older ruby version to provide a minor feature. That will prevent people using those rubies to update Faraday and get the latest features and fixes.

On the other hand, we can't keep supporting ancient ruby versions. That's why we'll definitely abandon 1.9.3 with the release of Faraday v1.0. I think a major release is the perfect time to decide which rubies we should support and which we shouldn't. My initial plan was to just drop 1.9.3, but given that 2.0 and 2.1 are both already EOL, we might think to push the threshold even further.

In short, I appreciate you trying to add support for 1.9.3 on brotli and I'll be happy to merge this in for release 0.13.0 if that happens. However, should that not be the case, we can simply move your PR against the v1.0 branch 👍

@jabley
Copy link
Contributor Author

jabley commented Aug 16, 2017

It is done. :shipit: 😀

Brotli is a new open-source compression algorithm from Google which can
give more dense compression, while compression and decompression speeds
are roughly the same.

Brotli responses are requested by setting `Accept-Encoding: br`.

Transferring less data over the network will typically result in a
faster experience for users.

This change adds support for Brotli decompression of responses.
@iMacTia iMacTia added this to the v0.13.0 milestone Aug 17, 2017
@iMacTia
Copy link
Member

iMacTia commented Aug 17, 2017

Great work in maintaining support for ruby 1.9.3.
I really appreciate you having to update other gems as well to accomplish this.
Really happy to merge and mark for next release 👍

@iMacTia iMacTia merged commit 12bbb51 into lostisland:master Aug 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants