Feature/support gzip #30
Conversation
|
Hi @jakobhellermann and @daxpedda . Please review #29 before this PR. Thanks! |
There was a problem hiding this comment.
GitHub has this feature for "Draft Pull Request", which is probably appropriate here.
You should probably not ping jakobhellermann, repo owners get notified anyway.
|
Thank you @daxpedda for all your constructive comments! I should have some time after U.S. Thanksgiving to take another pass. |
There was a problem hiding this comment.
LGTM.
So I'm not sure what jakobhellermann's policy is, but usually it is encouraged to do proper git rebases instead of merging it like this, makes it easier to reason about the commit flow and therefor review.
This PR extracts the logic from `run_server` that creates a new Router into a helper function named `get_router` to make it easier to test without needing to start the full server. This also adds a simple test that verifies current behavior for returning a br compressed wasm.wasm file. This change is in preparation for adding support dynamically change the available compression.
See jakobhellermann#28 for context. This new code will serve up br compressed if supported, but will fall back to gzip compression if not.
heisencoder
force-pushed
the
feature/support-gzip
branch
from
a539414 to
676b5eb
Compare
|
I went ahead and rebased this into two commits: one for the refactoring and one for supporting gzip. These commits are otherwise unchanged from the latest versions. Given that this PR includes the refactoring as a separate commit, I'll go ahead and simplify by closing the refactoring PR and just opening up this for general review. |
|
Thanks a lot for the PR! I'll give it a proper review over the weekend. One thing that I don't like though is that this will increase the startup duration for everyone, not just people who have to use #28. First off, for context, the reason that compression is hand-rolled instead of just slapping a
Now I can see a few solutions:
I don't know yet which I prefer. |
|
I did think about the considerations for increased startup time and thought that maybe using a future for gzip would hide that extra time for users who don't need it. However, if tower_http::compression can take care streaming compression in a layer, then maybe that's the simplest solution. I suspect this would allow for faster server bring-up because the layer could compress the file during the request instead of compressing all of it beforehand. While it's somewhat interesting to see the file size as the server is running, it's also possible to easily get the compressed file size from the browser developer tools. |
|
Alternatively we could move Gzip compression to be generated on the fly and leave brotli as it currently is. |
|
I'm currently using this branch because i need to test my app on a phone... Perhaps it could be enabled by setting an env var, or be the default if not binding to localhost? |
|
Hi @jakobhellermann -- any thoughts following-up from your last comment about which approach you'd prefer to get this issue resolved? Thanks! :) |
honestly I think I should just caring about printing the uncompressed size. I've opened a PR to remove the manual compression and just rely on |
|
@johanhelsing do you wanna try if #41 (now merged into main) works for your mobile use case? |
jakobhellermann
force-pushed
the
main
branch
from
9ac6629 to
0ce4cba
Compare
|
According to #28 (comment) the issue is fixed so I'll close this PR |
See #28 for context.
This new code will serve up br compressed if supported, but will
fall back to gzip compression if not.
This PR is based on the refactoring in #29 .
The gzip support was mostly copied over from PR #22 .