-
Notifications
You must be signed in to change notification settings - Fork 91
feat: add gzip handling to the hub_service
#385
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
base: main
Are you sure you want to change the base?
Conversation
|
@Quazia is attempting to deploy a commit to the farcaster Team on Vercel. A member of the Team first needs to authorize it. |
| let grpc_shutdown_tx = shutdown_tx.clone(); | ||
| let hub_service = HubServiceServer::from_arc(service.clone()) | ||
| .accept_compressed(CompressionEncoding::Gzip) | ||
| .send_compressed(CompressionEncoding::Gzip); |
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.
Is this always or only if the header is present in the request? Just want to make sure it won't break existing clients.
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.
I'm 95% sure that it accepts it, but it doesn't require it.
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.
Yep - accepts but doesn't require it.
| serde_json = "1.0" | ||
| sha2 = "0.10.6" | ||
| tonic = "0.12.3" | ||
| tonic = { version = "0.12.3", features = ["gzip"] } |
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.
I think there were some issues upgrading tonic-build. Worth a shot if build passes, but if it requires a bunch of patches, probably not worth it.
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.
It requires a fair number of patches but still would be worth picking up at some point.
Closses #373
I don't know if we want to add a test for this - not sure how we'd go about it since we probably don't want to test
main.rsdirectly.Also noticed that the
tonicandtonic-buildversions are quite different. Worth updating them to be consistent or is there a reason for the oldertonic-buildversion?