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.

Sign up for GitHub

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

Jump to bottom

gzip responses in adapter-node #367

Merged
merged 3 commits into from
Mar 2, 2021
Merged

gzip responses in adapter-node #367

merged 3 commits into from
Mar 2, 2021

Conversation

benmccann
Copy link
Member

Closes #345

@benmccann benmccann added the feature / enhancement New feature or request label Feb 6, 2021
@benmccann benmccann force-pushed the gzip branch 5 times, most recently from 6b0bfd7 to 10e3d16 Compare February 6, 2021 16:23
samccone
samccone suggested changes Feb 6, 2021
View reviewed changes
Copy link
Contributor

@samccone samccone 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 follow this process: #331 (comment) in order to generate a new version of the package.

packages/adapter-node/src/server.js Show resolved Hide resolved
@benmccann benmccann changed the title gzip responses gzip responses in adapter-node Feb 6, 2021
@samccone
Copy link
Contributor

samccone commented Feb 7, 2021

Make sure to follow this process: #331 (comment) in order to generate a new version of the package.

The only step remaining is this ^, then feel free to merge. Thanks Ben!!

@benmccann benmccann mentioned this pull request Feb 8, 2021
Closed
@Rich-Harris
Copy link
Member

Is compression the right package to use here? If we're offering compression OOTB isn't brotli better than gzip?

@benmccann
Copy link
Member Author

benmccann commented Mar 2, 2021 •
edited
Loading

I was weary of reimplementing compression in our own code. It's a battle-tested library and compressing ourselves isn't quite as simple as just calling gzip/brotli since they need to do a bunch of stuff like content negotiation, considering cache headers, etc. The picture becomes more complicated when we want to support both gzip and Brotli

I looked at Brotli, but unfortunately compression doesn't support Brotli yet. There are a couple PRs in progress so hopefully we can just bump the library in a couple months to get support

@benmccann benmccann force-pushed the gzip branch 2 times, most recently from 9b57986 to 9421abe Compare March 2, 2021 15:16
@Rich-Harris Rich-Harris merged commit 222fe9d into master Mar 2, 2021
@Rich-Harris Rich-Harris deleted the gzip branch March 2, 2021 21:53
@hfjallemark
Copy link

Is there any way to disable gzip compression? We're running Cloudflare and it won't compress responses to brotli if they are already gzipped by the origin.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samccone samccone samccone approved these changes

Assignees
No one assigned
Labels
feature / enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

node adaptor does not serve contents with any compression
4 participants
@benmccann @samccone @Rich-Harris @hfjallemark