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

[Relay] Regenerate socket URL on error disconnection #769

Merged
merged 9 commits into from
Apr 11, 2023

Conversation

alexander-lsvk
Copy link
Contributor

@alexander-lsvk alexander-lsvk commented Mar 28, 2023

Due Dilligence

  • Breaking change
  • Requires a documentation update

@alexander-lsvk alexander-lsvk changed the title Regenerate socket url on error disconnection [Relay] Regenerate socket URL on error disconnection Mar 28, 2023
try socketConnectionHandler?.handleDisconnect(closeCode: closeCode)
}

private func setUpSocketConnection() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this function called by anyone?

Comment on lines 142 to 143
host: relayHost,
projectId: projectId
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we maybe add host and projectId to the url factory init? they wan't change anyway

Comment on lines 83 to 86
let dispatcher = Dispatcher(
relayHost: relayHost,
projectId: projectId,
clientIdStorage: clientIdStorage,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would adding RelayClientFactory, like we have for other clients solve the init problem for Dispatcher and RelayClient?

var socket: WebSocketConnecting
var socketConnectionHandler: SocketConnectionHandler

var socket: WebSocketConnecting?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does it have to be optional?

Copy link
Contributor

@llbartekll llbartekll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if tested manually and works then looks good to me

@flypaper0
Copy link
Contributor

lgtm, I think it's better to recreate websocket instance

@llbartekll llbartekll merged commit da3706b into develop Apr 11, 2023
@llbartekll llbartekll deleted the auth-regeneration branch April 11, 2023 10:06
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