Skip to content

Conversation

OliverNChalk
Copy link

The use of impl Interceptor in the return type for these functions prevents downstream consumers from storing the client in a struct (without the use of dyn). This is because impl Interceptor cannot be named. Given these functions actually return InterceptorXToken we seem to just be destroying type information for no reason?

Copy link
Contributor

@kespinola kespinola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG2M.

The struct can also be made generic if something like mocking requires working with a trait instead of a struct.

struct MyClient<S, St> {
    sink: S,
    stream: St,
}

impl<S, St> MyClient<S, St>
where
    S: Sink<SubscribeRequest, Error = mpsc::SendError>,
    St: Stream<Item = Result<SubscribeUpdate, Status>>,
{
    fn do_something(&mut self) { ... }
}

@@ -102,8 +96,8 @@ impl<F: Interceptor> GeyserGrpcClient<F> {
pub async fn subscribe(
&mut self,
) -> GeyserGrpcClientResult<(
impl Sink<SubscribeRequest, Error = mpsc::SendError>,
impl Stream<Item = Result<SubscribeUpdate, Status>>,
mpsc::UnboundedSender<SubscribeRequest>,
Copy link
Contributor

@lvboudre lvboudre May 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Streaming is ok,
but going from impl Sink to UnboundedSender prevent us from changing this to another of channel in future say bounded Sender.

However, my main issue with this proposal is : itwill break current client implementation because Sink is async API all-the-way:
the method Sink::send from SinkExt is async, while the concrete type UnboundedSender::send is not async.

Can we use add + Clone to trait bound instead for the Sink?
By doing so it will not break any existing client code, but allow you to Clone it.
wdyt?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants