Allow custom app logic to trigger when a websocket connects/reconnects #75
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
peterbroadhurst
requested review from
awrichar,
nguyer and
dechdev
as code owners
Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
awrichar
reviewed
Aug 14, 2023
| async close(wait?: boolean): Promise<void> { | ||
| const closedPromise = new Promise<void>(resolve => { | ||
| this.closed = resolve; | ||
| }); | ||
| this.clearPingTimers(); | ||
| if (this.socket) { |
There was a problem hiding this comment.
Rather than relying on the other "close" handler and dealing with callbacks back and forth, would it be possible to just add another handler here?
const closedPromise = new Promise<void>(resolve => this.socket.on('close', resolve));There was a problem hiding this comment.
I'm not sure I can for the use case I'm fighting. Jest cannot handle any logging after a test completes, so what I need to know is that the existing callback that does logging has done it.
If I add another callback as you propose, then I'll be relying on the order of invocation of event handlers. Happy to do that if you'd prefer (I have written up the code for it ready to push, but I'll need to test it).
There was a problem hiding this comment.
Discussed in a message directly with @awrichar and deciding to merge.
awrichar
approved these changes
Aug 14, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When using an ephemeral WebSocket to listen for completion of a FireFly action, you need to know the socket is connected before you submit the action. This isn't currently possible in the SDK.
It's also not possible to send custom commands over the WebSocket, or trigger other processing as a side-effect of a reconnect.
Also in unit tests in a framework like Jest, you cannot allow a test to write log statements after the test exits (see jestjs/jest#9324 (comment)).
This drives a requirement for a
close()function that allows us to await the logging of the close, which is also included as an option in this PR.