-
Notifications
You must be signed in to change notification settings - Fork 174
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
fix(server): make tower::Service impl generic over HttpBody #1475
Conversation
@@ -970,28 +970,28 @@ impl<RpcMiddleware, HttpMiddleware> TowerService<RpcMiddleware, HttpMiddleware> | |||
} | |||
} | |||
|
|||
impl<Body, RpcMiddleware, HttpMiddleware> Service<HttpRequest<Body>> for TowerService<RpcMiddleware, HttpMiddleware> | |||
impl<RequestBody, ResponseBody, RpcMiddleware, HttpMiddleware> Service<HttpRequest<RequestBody>> for TowerService<RpcMiddleware, HttpMiddleware> |
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.
Ok, this is the fix right i.e, to make it generic over the body?
I would rather just include this fix and avoid updating tower so we don't have make breaking release for this but looks good to me overall and fix it for folks of v0.24.x release as well
Then release v0.25 as well with tower 0.5
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.
@niklasad1 thanks for your quick review! Just reverted the tower*
upgrade, will do that in a separate PR. BTW, should I bump the jsonrpsee patch versions as well?
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.
Usually we do a separate release PR for such things but since you already done it's fine. Nice work
24af68b
to
5ab818e
Compare
@hanabi1224 one tiny thing, can you just change the links in the README from |
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.
Nice one, thank you
@niklasad1 Do you mean the dependency status badge? I can change it in this PR but the badge will be down until 0.24.7 is actually published. |
Yes, exactly I'll release after this is merged. |
@niklasad1 Thanks! (Just curious, why not use https://deps.rs/crate/jsonrpsee/latest/status.svg , like |
Sure, that's better. Changed now |
Close #1133
tower_http::compression::CompressionLayer
as http middlewareupgradetower
andtower-http