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

Added support for brotli ('br') content-encoding #172

Closed
wants to merge 22 commits into from

Conversation

danielgindi
Copy link

Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

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

Make sure to update the documentation as well around it supporting br.

test/compression.js Outdated Show resolved Hide resolved
test/compression.js Outdated Show resolved Hide resolved
test/compression.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
test/compression.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@danielgindi
Copy link
Author

@dougwilson There's an issue with object-assign polyfill - it does not support the very outdated versions of node.js

@dougwilson
Copy link
Contributor

There's an issue with object-assign polyfill - it does not support the very outdated versions of node.js

I guess just pick a different module or an older version of that module.

index.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
test/compression.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@danielgindi danielgindi force-pushed the feature/brotli branch 3 times, most recently from d4a01cb to 42dacd1 Compare July 10, 2020 15:37
@danielgindi danielgindi changed the title Feature/brotli Added support for brotli ('br') content-encoding Jul 10, 2020
@danielgindi danielgindi force-pushed the feature/brotli branch 2 times, most recently from d3f283f to d557204 Compare July 10, 2020 15:55
README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

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

I added a comment about the faster statement and still have an open question on how a user can change compression level of br.

README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

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

I added a comment about the faster statement and still have an open question on how a user can change compression level of br. I'm also having trouble to actually get br compression to even work at all with Chrome. I'm trying to figure it out, as our number 1 issue opened here is how to get this module working, so having concrete information for how to get br working with a web browser (Chrome, for instance) would help a lot. For reference I used the example in the README and your branch and Chrome continues to only show it using gzip, even when the connection is https (which my understanding is a requirement for br to work in Chrome).

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@danielgindi
Copy link
Author

I added a comment about the faster statement and still have an open question on how a user can change compression level of br. I'm also having trouble to actually get br compression to even work at all with Chrome. I'm trying to figure it out, as our number 1 issue opened here is how to get this module working, so having concrete information for how to get br working with a web browser (Chrome, for instance) would help a lot. For reference I used the example in the README and your branch and Chrome continues to only show it using gzip, even when the connection is https (which my understanding is a requirement for br to work in Chrome).

Look at Chrome's Accept-Encoding: gzip, deflate, br - it puts br last. I'm guessing they put it last as a transition period, as some poor servers out there fail when brotli is specified, or intermediaries doing even worse.

Or maybe it's because when it first arrived they considered it a good compression for WOFF fonts, but they always tested with the highest compression levels. Today people know that with level 4 you have better results on all kinds of files.

@danielgindi
Copy link
Author

@dougwilson you have a PR pending

@danielgindi
Copy link
Author

I guess everyone are in vacation now

@nicksrandall
Copy link

@danielgindi just want to say that this is good work. Thanks for pushing for this.

@paambaati

This comment has been minimized.

@TheAlexLichter

This comment has been minimized.

@danielgindi
Copy link
Author

At this point I'm convinced this is not going to happen.

@Kle0s
Copy link

Kle0s commented Jan 25, 2022

At this point I'm convinced this is not going to happen.

Would you like to create an npm package from your fork? Or should I? I really think it's much needed (and would like to use it myself)

@nicksrandall
Copy link

If it is helpful, I’ve built this package and am using it in production in several places: https://github.com/nicksrandall/compression

@mmahalwy

This comment has been minimized.

@maggie44
Copy link

maggie44 commented Apr 20, 2022

If it is helpful, I’ve built this package and am using it in production in several places: https://github.com/nicksrandall/compression

Tempting, thanks for this. How has it been running, any issues?

And this one that seems pretty active: https://github.com/Econify/compression-next#readme

@romgrk

This comment was marked as abuse.

Copy link

@vinayakkulkarni vinayakkulkarni left a comment

Choose a reason for hiding this comment

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

LGTM

@nithin-murali-arch
Copy link

Looks like the PR is approved, could you folks resolve the conflicts and merge? Is there anything else we're waiting on?

@dgautsch
Copy link

Any plans on having this merged or is https://www.npmjs.com/package/shrink-ray-current the current best solution?

@barnhill
Copy link

Is there any current movement on getting this approved and merged? Im asking in reference to Apollo Server which we use for GraphQL. It doesn't support Brotli due to this dependency not supporting it.

@MannyPamintuanAtHeb
Copy link

@dougwilson, I was curious if you have the bandwidth to review this PR and provide a secondary approval with @vinayakkulkarni having already reviewed and approved?

Seeing br support land for Express would be impactful to apollo-server-express which currently at this time does not support br for compression.

Anything that the community can do to help see this PR land and become a reality?

@romgrk
Copy link

romgrk commented Jun 26, 2024

Realistically this repository should be considered abandoned, the maintainers haven't responded in a long time and brotli has been out for around 8 years so it's safe to assume there is no interest to add it. The community should probably rally around one of the multiple existing forks for this package. Maybe express-compression would be good?

@bjohansebas
Copy link
Member

@danielgindi @nicksrandall Thank you for this work, it has been used as a reference to move forward with #194. You can continue helping by reviewing #194 so we can launch it soon.

@bjohansebas bjohansebas closed this Nov 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.