Use built in zstd for compression when provided#503
Use built in zstd for compression when provided#503mattmatters wants to merge 1 commit intowojtekmach:mainfrom
Conversation
For >=v28 of Erlang, Zstd is part of Erlang's stdlib. This greatly simplifies dependency requirements in Req. I took some great pains to match how ezstd is called and matching its return type. It did prove to get a little ugly though to prevent compiler type warnings however. Hopefully in several years this can be built with the expection zstd always exists.
| defp supported_accept_encoding do | ||
| "zstd, #{if brotli_loaded?(), do: "br, ", else: ""}gzip" | ||
| end |
There was a problem hiding this comment.
I did this just to preserve the accept encoding order that tests expect as it would be a bit of a headache otherwise.
| if Code.ensure_loaded?(:zstd) do | ||
| def zstd_decompress(arg) do | ||
| try do | ||
| arg | ||
| |> :zstd.decompress() | ||
| |> :erlang.iolist_to_binary() | ||
| rescue | ||
| err in ErlangError -> | ||
| case err do | ||
| %ErlangError{original: {:zstd_error, reason}} -> | ||
| {:error, reason} | ||
|
|
||
| err -> | ||
| reraise err, __STACKTRACE__ | ||
| end | ||
| end | ||
| end | ||
|
|
||
| defp supported_accept_encoding do | ||
| "zstd, #{if brotli_loaded?(), do: "br, ", else: ""}gzip" | ||
| end |
There was a problem hiding this comment.
I feel pretty gross about this code. I'm open to alternative suggestions here. This ended up being the implementation I was able to arrive at that returned similar values to :ezstd.decompress and kept the type checker happy (Code.ensure_loaded?(:zstd) always evaluates to true, I'm a bit unsure why wrapping it in the macro zstd_support?/1 did not trigger the same warnings however).
| assert Exception.message(e) == | ||
| assert Exception.message(e) in [ | ||
| "Could not decompress Zstandard data: \"Unknown frame descriptor\"", | ||
| # TODO remove once Erlang v28 is required |
There was a problem hiding this comment.
I'm happy to blanket the codebase in TODO's like this, but I kinda felt like I was spreading garbage over a nicely manicured lawn.
|
Thank you. The PR looks good to me. I need some more time to figure out if it's worth it to keep support for |
2 participants
For >=v28 of Erlang, Zstd is part of Erlang's stdlib. This greatly simplifies dependency requirements in Req.
I took some great pains to match how ezstd is called and matching its return type. It did prove to get a little ugly though to prevent compiler type warnings however.
Hopefully in several years this can be built with the expection zstd always exists.