Replace ZipGenerator with ZipHandler #102
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Move more work into ZipKit, since it already handles the streaming body and the headers. What is important here is that we handle the `files` correctly - which is what the new ZipHandler will do, inside of the streaming ZipKit body. This reduces the number of interacting classes and objects and the number of conditionals. ZipKit, in turn, will now not apply the `chunked` transfer encoding unless explicitly asked to. This is a follow-through on julik/zip_kit#2 because using ZipKit with an HTTP/2 webserver requires the transfer encoding to be disabled - HTTP/2 framing will be used instead. Following the suggestions from Samuel I have removed the chunking, but it is still possible to force it. Logging streaming exceptions is also easier now, as the entire block handling the ZIP is inside the `each` iteration. There is no tempfile spooling either because it changed the flow drastically (the file would write out at body instantiation, not at iteration over the response)
|
@fringd wdyt? |
|
this looks good. merging... do you think this is worth making a major version bump? will the change to encoding potentially break some people on certain servers? |
|
I don't think it will, zipline wasn't applying the encoding previously and the change with zip_kit didn't get released yet. What may break is the fact that |
|
Sorry, if I wasn't clear - yes, we need a new major version 😉 |
|
cool, just pushed 2.0.0! thanks again julik ^_^ |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Move more work into ZipKit, since it already handles the streaming body and the headers. What is important here is that we handle the
filescorrectly - which is what the new ZipHandler will do, inside of the streaming ZipKit body. This reduces the number of interacting classes and objects and the number of conditionals.ZipKit, in turn, will now not apply the
chunkedtransfer encoding unless explicitly asked to. This is a follow-through on julik/zip_kit#2 because using ZipKit with an HTTP/2 webserver requires the transfer encoding to be disabled - HTTP/2 framing will be used instead. Following the suggestions from Samuel I have removed the chunking, but it is still possible to force it. Logging streaming exceptions is also easier now, as the entire block handling the ZIP is inside theeachiteration. There is no tempfile spooling either because it changed the flow drastically (the file would write out at body instantiation, not at iteration over the response).Note that this removes
ZipGenerator, so user code that overrides it will require modification. TheZipHandleris way smaller though.