-
Notifications
You must be signed in to change notification settings - Fork 602
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
Node.js Gzip #2953
Node.js Gzip #2953
Conversation
modificationTime: Option[Nothing] = None, | ||
fileName: Option[Nothing] = None, | ||
comment: Option[Nothing] = None |
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.
Using Option[Nothing]
keeps us source-compatible with JVM while only accepting None
. Furthermore, thanks to type erasure it leaves the door open to add support for these in the future if Node.js ever gets them.
@@ -82,6 +82,36 @@ abstract class CompressionSuite(implicit compression: Compression[IO]) extends F | |||
) | |||
) | |||
|
|||
test("inflate please wrap") { |
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.
Added these tests because actually the existing Node.js inflate/deflate implementation was broken 😅
): Pipe[F, Byte, Byte] = deflateImpl(deflateParams) { options => | ||
require(!deflateParams.fhCrcEnabled, "FHCRC is not supported on Node.js") | ||
deflateParams.header match { | ||
case ZLibParams.Header.GZIP => facade.zlib.createGzip(options) |
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.
Delegating to Node.js is as easy is this.
else | ||
facade.zlib.inflateSync(in, options) | ||
Chunk.uint8Array(out).toArray | ||
} | ||
|
||
test("gzip |> gunzip ~= id") { |
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.
Frustratingly, these tests are cloned from the JVM suite except modified to remove the optional headers unsupported on Node.js. The duplication is unfortunate but I didn't see a clean way to share them.
Closes #2771, with many apologies to @yurique for their heroic effort on that 🙏
In #2771 we attempted to cross-build the existing Gzip implementation on the JVM. Instead, this PR delegates directly to the Node.js Gzip implementation. This is better for performance and is much simpler to implement. However, we are unable to support the optional headers for filename, comment, and timestamp. Fortunately http4s does not require these in its Gzip middlewares.
There is an unfortunate amount of duplication of tests and scaladocs in this PR.