search

nats-io / nats.rs

Rust client for NATS, the cloud native messaging system.
Apache License 2.0
1.04k stars 164 forks source link

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

Closed rvolosatovs closed 1 month ago

rvolosatovs commented 4 months ago

Proposed change

I'd like to expose https://github.com/nats-io/nats.rs/blob/ec6331cb05b81e6b3c41bd75762ddab18f256ecd/async-nats/src/client.rs#L69 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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

I've reworked #1267 and added https://github.com/nats-io/nats.rs/pull/1267/commits/5354f827071197d9771ab4ce8083237ad526bd5a, which now adds a Publisher struct, which is essentially the opposite side of a Subscriber, which already exists.

WDYT?

thomastaylor312 commented 4 months ago

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