-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
feat(devserver/http): support gzipped responses #4011
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
9 Ignored Deployments
|
🟢 CI successful 🟢Thanks |
b34ae77
to
029759c
Compare
Benchmark for 072559eClick to view benchmark
|
let stream_ext = content | ||
.read() | ||
.into_stream() | ||
.map_err(|err| std::io::Error::new(std::io::ErrorKind::Other, err)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: (with appropriate use std::io;
statement at the top)
.map_err(|err| std::io::Error::new(std::io::ErrorKind::Other, err)); | |
.map_err(|err| io::Error::new(io::ErrorKind::Other, err)); |
let gzipped_stream = tokio_util::io::ReaderStream::new( | ||
async_compression::tokio::bufread::GzipEncoder::new( | ||
tokio_util::io::StreamReader::new(stream_ext), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: with use tokio_util::io::{ReaderStream, StreamReader}; use async_compression::tokio::bufread::GzipEncoder;
let gzipped_stream = tokio_util::io::ReaderStream::new( | |
async_compression::tokio::bufread::GzipEncoder::new( | |
tokio_util::io::StreamReader::new(stream_ext), | |
let gzipped_stream = ReaderStream::new( | |
GzipEncoder::new( | |
StreamReader::new(stream_ext), |
.into_stream() | ||
.map_err(|err| std::io::Error::new(std::io::ErrorKind::Other, err)); | ||
|
||
let gzipped_stream = tokio_util::io::ReaderStream::new( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not all content should be compressed, mainly images except SVG. Compressing binary formats will only increase the file size. Can we do an inspection for the content-type to ensure it's a textual format?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably yes, could have some basic mime check for the content types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Express's compression middleware uses compressible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, that's what I figured. But we should have rust corresponding one instead, isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rust crate seems out of date? We could get most of the way by just using the fallback algorithm from compressible
:
text/*
*/*+json
*/*+text
*/*+xml
and a few extras for application/json
and application/javascript
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean only compress for those, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
029759c
to
0a6892f
Compare
Benchmark for 6434755Click to view benchmark
|
@@ -92,14 +100,28 @@ pub async fn process_request_with_content_source( | |||
); | |||
} | |||
|
|||
// naively checking if content is `compressible`. | |||
let mut should_compress = false; | |||
let should_compress_predicate = |mime: &Mime| match (mime.type_(), mime.subtype()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also be checking mime.suffix()
for:
- xml
- json
- text
0a6892f
to
a88aefb
Compare
Benchmark for 3d84ec8
Click to view full benchmark
|
# New Features * vercel/turborepo#4011 # Performance Improvements * vercel/turborepo#3955 * vercel/turborepo#4018 # Bug Fixes * vercel/turborepo#4037 * vercel/turborepo#4028 # Other * vercel/turborepo#3974 * vercel/turborepo#4015 * vercel/turborepo#3999 * vercel/turborepo#4026 * vercel/turborepo#4053 * vercel/turborepo#3891 Co-authored-by: JJ Kasper <[email protected]>
Implements WEB-666. This PR enables gzip compressions for the responses by default. This can be extended to other compression method (i.e brotli), but for now implements minimal default compression only to pass existing test case. Fixes `test/integration/compression/test/index.test.js`.
Implements WEB-666. This PR enables gzip compressions for the responses by default. This can be extended to other compression method (i.e brotli), but for now implements minimal default compression only to pass existing test case. Fixes `test/integration/compression/test/index.test.js`.
Implements WEB-666. This PR enables gzip compressions for the responses by default. This can be extended to other compression method (i.e brotli), but for now implements minimal default compression only to pass existing test case. Fixes `test/integration/compression/test/index.test.js`.
Implements WEB-666.
This PR enables gzip compressions for the responses by default. This can be extended to other compression method (i.e brotli), but for now implements minimal default compression only to pass existing test case.
Fixes
test/integration/compression/test/index.test.js
.