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

Feature Suggestion: await new messages #28

Closed
hanayashiki opened this issue Jun 13, 2020 · 19 comments
Closed

Feature Suggestion: await new messages #28

hanayashiki opened this issue Jun 13, 2020 · 19 comments

Comments

@hanayashiki
Copy link
Contributor

I hope this package can enable us to do the following:

await ws.open();

while (true) {
  data = await ws.recv();
  ws.send(data);
}

This is actually not that trivial if we need to handle connection error without a wrapper.

@ITenthusiasm
Copy link

Doesn't the event handler for messages already cover this?

@hanayashiki
Copy link
Contributor Author

yes, but it is not promise-based. if you are doing something async and sequential, you have to write a promise wrapper for it.

@ITenthusiasm
Copy link

Maybe I'm having a hard time imagining your scenario. If you're doing something asynchronous, I'd think you could just make the event handler itself an asynchronous function and use async/await as needed.

I'm also going off the example you gave. It could be that you're imagining something more complex.

@hanayashiki
Copy link
Contributor Author

hanayashiki commented Oct 26, 2020

Maybe I'm having a hard time imagining your scenario. If you're doing something asynchronous, I'd think you could just make the event handler itself an asynchronous function and use async/await as needed.

I'm also going off the example you gave. It could be that you're imagining something more complex.

For example, two parties doing a Diffie-Hellman Key Exchange:
Stage 1:
Alice sends her DH pub key to Bob in a websocket frame, waiting for Bob's pub key
Bob sends his DH pub key to Alice in a websocket frame
Alice receives Bob's pub key, and computes the shared DH private key.

Stage 2:
while True:
Alice waits for encrypted message, Alice receives message and decrypt, Alice sends encrypted message...

while True:
Bob waits for encrypted message, Bob receives message and decrypt, Bob sends encrypted message...

@ITenthusiasm
Copy link

Ah, I see. Well I think one way to get around that is if you "typed" your messages in some way. Then you could circumvent the while true loop and just tell the websockets how to handle the message types.

For instance, you could have one type of message for Stage 1 and then another type of message for Stage 2. You'd end up with some kind of switch/case/if/else block but it would get the job done.

Seeing the while true just reminds me of what (I think) event handlers were made to take care of.

@hanayashiki
Copy link
Contributor Author

Ah, I see. Well I think one way to get around that is if you "typed" your messages in some way. Then you could circumvent the while true loop and just tell the websockets how to handle the message types.

For instance, you could have one type of message for Stage 1 and then another type of message for Stage 2. You'd end up with some kind of switch/case/if/else block but it would get the job done.

Seeing the while true just reminds me of what (I think) event handlers were made to take care of.

The best of promise is that it can eliminate those hard-to-read switch-cases.

@DronNick
Copy link

I implemented similar requirement this way (not sure if really correct, but it works for me):

// promisified onMessage callback
function getResponse(mt) {
    return new Promise(((resolve, reject) => {
        wsp.onMessage.addListener(message => {
            //console.log("<", message);
            if(mt == JSON.parse(message).mt) {
                wsp.removeAllListeners();
                resolve(message);
            } else {
                wsp.removeAllListeners();
                reject();
            }
        });
    }));
}

const openWebSocket = async function (){
    await wsp.open();
    await wsp.send(JSON.stringify({ mt: "AppChallenge" }));
    const challenge = JSON.parse(await getResponse("AppChallengeResult")).challenge;
    await wsp.send(JSON.stringify(loginMessage(challenge)));
    var resp = await getResponse("AppLoginResult");
    await wsp.close();
    return JSON.parse(resp).ok;
}

openWebSocket();

@ITenthusiasm
Copy link

ITenthusiasm commented Oct 27, 2020

The best of promise is that it can eliminate those hard-to-read switch-cases.

Yeah I get that. To be fair though, some people may prefer that over infinite loops, especially since they don't always behave as expected when it comes to asynchronous behavior. Conditional blocks can be simple to read if they're structured well (unless you have a million cases...but hopefully you wouldn't).

I think there may be another library that works with websockets in such a way that you're adding listeners for custom events. That's also an approach if you're trying to avoid conditional blocks.

A promise listener system could potentially be cool. But I think there would be some limitations. I definitely see how the library as it is now enables people to do some things that they wouldn't easily be able to do without it, especially when it comes to testing. But for an infinite loop that awaits messages and responds to them, it feels like it could easily become redundant when compared to the event handling system. Again, that's kind of what it was created for.

@hanayashiki
Copy link
Contributor Author

I think what you said makes sense. There can be another library on top of ws to implement that type of workflow. I also think that there might be clashes if we allow both promise-based onmessage and handler-based onmessage.

@vitalets
Copy link
Owner

vitalets commented Oct 30, 2020

Interesting discussion!
Currently wsp.sendRequest method already returns Promise, waiting for response.
It seems reasonable to me to add more common method:

const messageWithFoo = await wsp.waitUnpackedMessage(data => Boolean(data.foo));

Implementation is similar to @DronNick getResponse example.

What do you think?

@DronNick
Copy link

This would be useful. I talk to an API that can accept and return different or even missing RequestIDs on the same connection, so the request-response mechanism with one static ID does not work for me here.

By the way, great work done on this library, Большое спасибо :)

@ITenthusiasm
Copy link

I agree with DronNick. The library is super neat!

My main concern was that if the solution/implementation turned out to be non-trivial, it could be a lot of work to kind of reinvent the wheel. But if the implementation is easy then I guess it could be useful to have.

In general, I think promises have always been preferred over callbacks due to their superior structure. But as I kind of said before, it's hard for me to imagine a promise-based listener getting very far without turning into a loop with switch/case/if/else blocks -- at which point it basically returns to the form of an event handler.

For complex websocket interactions that can be expressed as a loop with a single, straightforward responsibility/flow (no conditional blocks), a promise-based listener would probably do well. It could potentially be easier to read at face value, though it would require the developer to do a bit more learning to fully understand. So there are trade-offs

@vitalets
Copy link
Owner

Thank you, guys! Reopening the issue.

@vitalets vitalets reopened this Oct 31, 2020
@cameronelliott
Copy link

Interesting discussion!
... chopped
What do you think?

I totally support the addition of a method like this, I was looking for the exact same thing.

@cameronelliott
Copy link

I'm working on this at present, so I thought I'd add some observations.

--

The function from @DronNick basically does this:
Wait for next message, if it matches, then resolve else reject

Is that the only or most general rule we want? I am not sure.

In my particular case I need something like:
Wait for the next message, if it matches, then resolve else
do NOT reject, just wait for next message,
unless X seconds have passed

If we take some time on this issue and I explore it further,
I may be able to add some more developed thoughts
about what is needed at least for my situation.
I'm sure some other people have some good insight also.

@hanayashiki
Copy link
Contributor Author

hanayashiki commented Nov 5, 2020

I'm working on this at present, so I thought I'd add some observations.

--

The function from @DronNick basically does this:
Wait for next message, if it matches, then resolve else reject

Is that the only or most general rule we want? I am not sure.

In my particular case I need something like:
Wait for the next message, if it matches, then resolve else do NOT reject, just wait for next message, unless X seconds have passed

If we take some time on this issue and I explore it further,
I may be able to add some more developed thoughts
about what is needed at least for my situation.
I'm sure some other people have some good insight also.

Maybe the target should be "Wait until the matching message come"
For example, if you are implementing a chatroom, there can be updates of new message from the server, or you can actively send a new message to the server, and wait for the response from the server. There's no guarantee that, there's no updates of new message from the server between you send a new message and get the response.

@vitalets
Copy link
Owner

vitalets commented Nov 10, 2020

Adding a timeout option looks good suggestion!

const messageWithFoo = await wsp.waitUnpackedMessage(data => Boolean(data.foo), { timeout: 1000 });

By default no timeout applied.

@vitalets
Copy link
Owner

If we take some time on this issue and I explore it further,
I may be able to add some more developed thoughts

Of course, you are welcome!

@vitalets
Copy link
Owner

Released as v2.0.1.
Feel free to report any problems!

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

No branches or pull requests

5 participants