-
Notifications
You must be signed in to change notification settings - Fork 493
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
[Merged by Bors] - Initial stuff for Fluvio::connect_with_connector #1120
Conversation
rustfmt is failing |
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.
LGTM
local cluster test is failing |
"Expected browser binary location, but unable to find binary in default location, no 'moz:firefoxOptions.binary' capability provided, and no binary flag set on the command line" @nacardin is firefox or chrome installed on the custom runner? |
Suggest separating integration test as 2nd PR so code changes can go thru |
pub async fn connect_with_connector( | ||
connector: DomainConnector, | ||
config: &FluvioConfig, | ||
) -> Result<Self, FluvioError> { |
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.
Why are we adding this? And more importantly, what is the usage pattern here? Are we expecting typical users to leverage this or is it more of an implementation detail for the WASM library that users will not directly use?
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.
This allows client to use custom networking. For example: unix socket, websocket, QUIC, etc.
let producer = client.topic_producer(topic.clone()).await; | ||
assert!(producer.is_ok()); | ||
let producer = producer.unwrap(); | ||
let send = producer.send("foo", "bar").await; |
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.
This is a work around because #986 is still an issue.
will merge this after release |
bors r+ |
I also added in a wasm test. I've got mixed feelings about where it is now.
Pull request successfully merged into master. Build succeeded: |
Move changelog for this to 0.8.4 |
I also added in a wasm test. I've got mixed feelings about where it is now.