Send commands based on transport state instead of connected state #278
Conversation
This file contains hidden or 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
…ent connected state
|
Many thanks @ThisIsEsh Will take a look today and during the weekend. Regarding local tests, there is a single command used in CI: centrifuge-js/.github/workflows/ci.yml Line 21 in e8d553b But adding docker-compose makes sense anyway - let's solve separately - #279 |
FZambia
requested changes
Apr 5, 2024
FZambia
reviewed
Apr 5, 2024
This was referenced Apr 6, 2024
FZambia
approved these changes
Apr 7, 2024
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.
Change sending commands logic, rely on transport state instead of client connected state,
Fixes missing subscribe frames bug for unsubscribes, that have been called between transport open and connected frame response for server.
Also it will allow to send subscribe requests after transport has been opened without waiting for response from server.
Notes:
Removed
optimisticflag from Subscription, it does not make sense, because we can just rely on transport state.Removed
skipSendingflag from Subscription, it's not longer required, because it have been used only for sending optimistic subs inemulationtransport and this optimistic subs are removed right now, because they would require implementation of queue of unsubscribe frames, which would lead to way more complicated logic. But it will be easy to return, if will be required later.Also I have made a public method in centrifuge, that indicates that transport is ready, so it can be used inside subscription without
@ts-ignore, but I think it would be good idea to move all logic of skipping commands sending into Centrifuge client and remove early returns from Subscription.P.S. I have spent some time trying to properly configure centrifugo server instance to be able to run all tests locally, what do you think about adding docker-compose file + config.json to this repo?