Skip to content
This repository has been archived by the owner on Apr 14, 2023. It is now read-only.

fix(NA): Non forced close without connection terminate and also some connection's flow improvements #197

Conversation

mistic
Copy link
Contributor

@mistic mistic commented Jun 30, 2017

@Urigo @dotansimha @DxCx

This PR solves the following issue: usually when a backend receives a connection_terminate will invalidate the previous credentials so we should only send connection_terminate in a forceClose.

Copy link
Contributor

@DxCx DxCx left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@mistic mistic mentioned this pull request Jul 3, 2017
src/client.ts Outdated

if (isForced) {
if (this.checkConnectionIntervalId) {
clearInterval(this.checkConnectionIntervalId);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should set this with undefined or null

src/client.ts Outdated
@@ -424,15 +456,37 @@ export class SubscriptionClient {
}

private checkConnection() {
this.wasKeepAliveReceived ? this.wasKeepAliveReceived = false : this.close(false);
this.wasKeepAliveReceived ? this.wasKeepAliveReceived = false : this.close(false, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please convert into full if and not shortend

Copy link
Contributor

@DxCx DxCx left a comment

Choose a reason for hiding this comment

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

👍

@mistic mistic changed the title fix(NA): Non forced close without connection terminate fix(NA): Non forced close without connection terminate and also some connection's flow improvements Jul 5, 2017
Copy link
Contributor

@dotansimha dotansimha left a comment

Choose a reason for hiding this comment

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

Just fix CI and I think we can merge it :)
@Urigo

clearTimeout(this.maxConnectTimeoutId);
this.maxConnectTimeoutId = null;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Reformat :)

@dotansimha dotansimha merged commit af9bc33 into apollographql:master Jul 7, 2017
@mistic mistic deleted the fix-non-forced-close-without-conneciton-terminate branch July 7, 2017 13:32
@mistic mistic restored the fix-non-forced-close-without-conneciton-terminate branch July 7, 2017 13:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants