Skip to content
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

Expose internal command mpsc::channel in public API #1266

Closed
rvolosatovs opened this issue May 21, 2024 · 4 comments · Fixed by #1267
Closed

Expose internal command mpsc::channel in public API #1266

rvolosatovs opened this issue May 21, 2024 · 4 comments · Fixed by #1267
Labels
proposal Enhancement idea or proposal

Comments

@rvolosatovs
Copy link
Contributor

rvolosatovs commented May 21, 2024

Proposed change

I'd like to expose

pub(crate) sender: mpsc::Sender<Command>,
as a Sink<async_nats::Command> in some way (or even just a plain channel) - see use case below

This could look like

fn command_sink(&self) -> impl Sink<async_nats::Command>

or simply

fn command_sender(&self) -> mpsc::Sender<async_nats::Command>

This would require making async_nats::Command public

This would be an advanced functionality and I'm happy to e.g. guard this by an opt-in feature flag if there are concerns around that

The sender -> sink mapping itself is facilitated by https://docs.rs/tokio-util/latest/tokio_util/sync/struct.PollSender.html#impl-Sink%3CT%3E-for-PollSender%3CT%3E

Use case

I'm using NATS as a byte stream transport and to facilitate that I'd like to implement a Sink pairing a client and a subject. Unfortunately, plain publish and related functionality does not provide enough data to be able to do that correctly.

Contribution

yes, will do so in a few hours

@Jarema
Copy link
Member

Jarema commented May 22, 2024

Hey!

Thank you for your proposal and suggestion!

I understand that use case and having a nice way to use sink would be good, however I do not like exposing Sender, as this exposes internal API and prevents us from refactoring the internals without breaking users. And we do plan to change those internals at some point in time, depending how tokio evolution, its mpsc and other factors play out.

As usual I'm against any unnecessary layers of indirection, in this case, I would prefer having one.

@rvolosatovs
Copy link
Contributor Author

Hey!

Thank you for your proposal and suggestion!

I understand that use case and having a nice way to use sink would be good, however I do not like exposing Sender, as this exposes internal API and prevents us from refactoring the internals without breaking users. And we do plan to change those internals at some point in time, depending how tokio evolution, its mpsc and other factors play out.

As usual I'm against any unnecessary layers of indirection, in this case, I would prefer having one.

That's understandable, so how about the impl Sink suggestion?

In fact, it could be a publish_sink(&self, subject: impl ToSubject) -> impl Sink<Bytes>, so it would not even require exposing the Command

@rvolosatovs
Copy link
Contributor Author

I've reworked #1267 and added 5354f82, which now adds a Publisher struct, which is essentially the opposite side of a Subscriber, which already exists.

WDYT?

@thomastaylor312
Copy link
Contributor

@Jarema Looking at #1267, it looks like it is now pretty clean and straightforward as it doesn't expose the internals now

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

Successfully merging a pull request may close this issue.

3 participants