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

Subscriptions persist across reconnects &simplified client/communicator #8

Closed
wants to merge 2 commits into from

Conversation

dendle
Copy link

@dendle dendle commented Jan 25, 2018

Hello Marfusios,

I had trouble with subscriptions being lost after a reconnect - In this PR i made it the communicators responsibility to re-send subscriptions in reconnect - this prompted me to also change the surface of the client - not sure if you like this way of doing things, but it seemed to make things cleaner in my head.

Cheers,
Matt

@dendle
Copy link
Author

dendle commented Jan 27, 2018

It might be luck, but this version is the first version that has remained connected for over 24hrs. By remained connected, I mean that it it continued streaming tickers after a reconnect occured - the only missing data was when the timeouts were waiting to kick in.

@henningms
Copy link

Can this be achieved without removing the Communicator as a constructor dependency?

@Marfusios
Copy link
Owner

Marfusios commented Jul 9, 2018

Hello, thanks for the PR, but unfortunately I can't merge it.
It is too much groundbreaking and it removes the open-closed principle - adding a new request (api coverage) shouldn't force to modify existing code.

Please try the latest version of the library, reconnection should work correctly.
And resubscribing to channels should be handled by client, see #12

@Marfusios Marfusios closed this Jul 9, 2018
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

Successfully merging this pull request may close these issues.

3 participants