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

Merged
merged 1 commit into from
Aug 17, 2024

Conversation

danielgindi
Copy link
Contributor

@danielgindi
Copy link
Contributor Author

How are we doing?

Copy link
Member

@UlisesGascon UlisesGascon left a comment

Choose a reason for hiding this comment

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

LGTM @danielgindi!

Thanks fo the links attached to the PR. I will love to be able to try that compression in my next project 💪

@danielgindi
Copy link
Contributor Author

danielgindi commented Aug 4, 2020

LGTM @danielgindi!

Thanks fo the links attached to the PR. I will love to be able to try that compression in my next project 💪

Yeah this brotli is truly amazing :)

Btw we have a PR in the compression repo, to allow responses to be encoded too. It's two sides of the same coin

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 moved over one comment that was still outstanding from the first repo and a general comment to make sure our detection makes sense from the noode.js project.

README.md Outdated Show resolved Hide resolved
lib/read.js Outdated Show resolved Hide resolved
lib/read.js Outdated Show resolved Hide resolved
@danielgindi
Copy link
Contributor Author

@dougwilson Have you noticed that we suddenly have a "leak" in the older node versions in CI?
These happens without any apparent reason, and the change in the code was insignificant, only DRYed a throw statement.
That's why I disabled that notification to begin with.

@danielgindi
Copy link
Contributor Author

@dougwilson Anything preventing this from being published?

package.json Outdated Show resolved Hide resolved
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

** spam content removed **

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

** spam content removed **

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

** spam content removed **

@ignlg
Copy link

ignlg commented Jun 30, 2023

Is this feature stuck for some specific reason? Is there something that can be done to fix it?

Copy link

@rubiin rubiin left a comment

Choose a reason for hiding this comment

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

lgtm

@wesleytodd wesleytodd changed the base branch from master to 2.x July 26, 2024 21:50
@wesleytodd
Copy link
Member

@danielgindi I re-targeted this to the 2.0 branch where we are preparing the major release for use in express@5. If you are interested in resolving the remaining threads and re-basing it onto 2.x that would be awesome. If not I can take care of it when sometime next week when we work on landing the final changes for the v2 release.

@wesleytodd wesleytodd force-pushed the feature/brotli branch 5 times, most recently from 9f5310b to 69e7f71 Compare August 17, 2024 18:34
@wesleytodd wesleytodd merged commit 07ce14d into expressjs:2.x Aug 17, 2024
4 checks passed
@wesleytodd wesleytodd mentioned this pull request Aug 17, 2024
12 tasks
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.

6 participants