-
Notifications
You must be signed in to change notification settings - Fork 27.2k
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
Add support for customizing compress encodings #657
Conversation
// Otherwise, it'll server gzipped version. | ||
compress: { | ||
br: function(filePath) { | ||
return fs.createReadStream(filePath).pipe(iltorb.compressStream()) |
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.
I think we should accept Promise
instead of ReadableStream since errors might occur in the middle of compression process.
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.
But we capture it inside our code. We convert this into a promise.
Why I like this approach is, this is how most of the compression API works. So, it's pretty simple for the user. We handle the rest.
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.
But we capture it inside our code.
Actually, it doesn't capture all errors since errors are emitted on each stream. http://stackoverflow.com/questions/21771220/error-handling-with-node-js-streams
Additionally, we should consider the case you do something async inside the callback.
br (path) {
something(path).then(() => {
..
}).catch((err) => {
// handle 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.
@nkzawa so what what do get from the promise?
- Stream
- Nothing: Let them to write to the disk.
We can do something like this too:
{
br: function(inputPath, outputPath) {
return new Promise()
}
}
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.
👍 That seems the simplest way.
Another idea is to accept only a transform stream.
gzip: function() {
return zlib.createGzip()
}
- We can catch all errors since other streams are prepared by us.
- Maybe you can still do any async operations by wrapping them inside a stream.
I'm not so sure if this would cover all cases though.
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.
@nkzawa I like that approach. And we can also also accept a promise from that function. (Which returns a stream)
What's |
Will leave this for post 2.0 |
@rauchg this is br: https://github.com/google/brotli (AKA: Realistic version of Pied Piper) |
@nkzawa check now. |
What I don't get is why we need the br key. Why don't we have a general
callback directly?
…On Thu, Jan 5, 2017 at 10:17 PM Arunoda Susiripala ***@***.***> wrote:
@nkzawa <https://github.com/nkzawa> check now.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#657 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAAy8WRxV1IE19zVFcaT9DeSjfFEnvwCks5rPdxigaJpZM4La59U>
.
|
@rauchg That's for the type of compress encodings we support. We use this key (like 'br', 'gzip') to save the compressed file with that extension. Also we use this to detect which type of encodings we support and serve which is suits for the given browser. |
Looks great, many thanks! |
@arunoda should next do the accept-encoding parsing, or should we hand the header (or even request) to the function? |
@rauchg The compression is currently done statically on build, and next checks the supported and accepted encoding to decide what file to serve, which seems reasonable. Compress on build: https://github.com/zeit/next.js/pull/657/files#diff-1634f71f56280340be4063732d469343R12 |
</Link> | ||
|
||
<Link href='/about'> | ||
<a style={styles.a} >About</a> |
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.
It would be cool to use styled-jsx
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.
I just wanna talk about this feature in this example.
Anyway, I don't mind using that too.
// In this case, first it'll try to serve the `br` version if the browser supports it. | ||
// Otherwise, it'll serve the gzipped version. | ||
compress: { | ||
br: function () { |
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.
How about compress the syntax to something like:
br: iltorb.compressStream,
gzip: () => new Promise(resolve => resolve(zlib.createGzip()))
}
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.
Isn't is the same as right now except it should be br: iltorb.compressStream()
?
If you are looking at ES2015 features, I didn't use them because we don't transpile the next.config.js
.
@arunoda since we're moving to not use compression in next.js but let it be handled by the server, should this PR be closed? |
Yeah! let's close this. |
Related: #555
By default we only do if for
gzip
.For an example, here how to add support for both
br
andgzip
compress encodings.