Add Brotli support with pure go implementation #1587
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1587 +/- ##
==========================================
- Coverage 84.96% 84.88% -0.08%
==========================================
Files 28 29 +1
Lines 2168 2210 +42
==========================================
+ Hits 1842 1876 +34
- Misses 211 216 +5
- Partials 115 118 +3
Continue to review full report at Codecov.
|
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
The bot was too fast for me, sorry. |
| res.Header().Set(echo.HeaderContentEncoding, brotliScheme) // Issue #806 | ||
| rw := res.Writer | ||
|
|
||
| w := brotli.NewWriterOptions(rw, brotli.WriterOptions{Quality: config.Level}) |
There was a problem hiding this comment.
Please use a sync.Pool here to reduce the amount of allocations performed by this middleware. Yo can check the current implementation of compress.go or use #1672 as a reference
| assert.Equal("test\n", string(chunkBuf)) | ||
|
|
||
| // Write and flush the second part of the data | ||
| c.Response().Write([]byte("test\n")) |
There was a problem hiding this comment.
Please use other body here to really be sure that you are getting the second part of the data and not the first part again
| assert.Equal("test\n", string(chunkBuf)) | ||
|
|
||
| // Write the final part of the data and return | ||
| c.Response().Write([]byte("test")) |
| Skipper Skipper | ||
|
|
||
| // Brotli compression level. | ||
| // Optional. Default value -1. |
There was a problem hiding this comment.
This comment is not correct. The default value is brotli.DefaultCompression
| if config.Skipper == nil { | ||
| config.Skipper = DefaultBrotliConfig.Skipper | ||
| } | ||
| if config.Level == 0 { |
There was a problem hiding this comment.
Be aware that for the Brotli library that you added a Level equal to 0 is a valid level. I thinkit is better to remove this check and keep the Level received by parameter.
https://github.com/andybalholm/brotli/blob/729edfbcfeaad43a6412c185205501ec80773250/writer.go#L8-L12
const (
BestSpeed = 0
BestCompression = 11
DefaultCompression = 6
)
|
Please, don't forget to update your PR with the latest changes on master |
|
This PR will be considered for v5. Concerns:
It might also be an option to make this a contrib middleware and keep it seperate of core echo. |
|
@delaneyj Could you please update this PR with the latest changes on master and also address the comments if they make sense? |
|
Ping @delaneyj |
|
As this middleware uses external library separate repo would + PR to add it to list would be best as then owner could iterate and have breaking changes as they choose. |
|
Closing this PR due to inactivity and required additional dependencies. |
|
Is Brotili compression an Echo v5 middleware? |
It appears both #1223 was closed due to
cgousage. Found a pure go port and used Gzip middleware as a launch point. Tests pass and first contribution so let me know if I'm missing anything!