FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Added support for brotli ('br') content-encoding by danielgindi · Pull Request #172 · expressjs/compression · GitHub

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

Closed
danielgindi wants to merge 22 commits into
expressjs:masterfrom
danielgindi:feature/brotli
Closed

Added support for brotli ('br') content-encoding#172
danielgindi wants to merge 22 commits into
expressjs:masterfrom
danielgindi:feature/brotli

Conversation

Copy link
Copy Markdown
Contributor

dougwilson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread test/compression.js Outdated
Comment thread test/compression.js Outdated
Comment thread test/compression.js Outdated
Comment thread index.js Outdated
Comment thread index.js Outdated
Comment thread test/compression.js Outdated
Comment thread index.js Outdated

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
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.

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

dougwilson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread README.md Outdated

dougwilson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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).

Comment thread README.md Outdated
Comment thread README.md Outdated
Comment thread README.md Outdated
Comment thread README.md Outdated
Comment thread README.md Outdated

Copy link
Copy Markdown
Contributor 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.

Copy link
Copy Markdown
Contributor Author

@dougwilson you have a PR pending

Copy link
Copy Markdown
Contributor Author

I guess everyone are in vacation now

Copy link
Copy Markdown
Contributor

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

This comment has been minimized.

This comment has been minimized.

Copy link
Copy Markdown
Contributor Author

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

Kle0s commented Jan 25, 2022

Copy link
Copy Markdown

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)

Copy link
Copy Markdown
Contributor

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

This comment has been minimized.

maggie44 commented Apr 20, 2022
edited
Loading

Copy link
Copy Markdown

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

This comment was marked as abuse.

vinayakkulkarni left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

Copy link
Copy Markdown

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

Copy link
Copy Markdown

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

Copy link
Copy Markdown

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.

Copy link
Copy Markdown

@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 commented Jun 26, 2024

Copy link
Copy Markdown

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?

Copy link
Copy Markdown
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 closed this Nov 3, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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.


Back | FazBrowse Home | New Git URL