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

Clear reconnectAttemptTimeout in serverSentEvents.stop #2995

Merged
merged 0 commits into from
Apr 18, 2014
Merged

Conversation

halter73
Copy link
Member

@@ -63,7 +62,7 @@
}

if (reconnecting) {
reconnectTimeout = window.setTimeout(function () {
connection._.reconnectAttemptTimeout = window.setTimeout(function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

consider renaming to reconnectAttemptTimeoutId

@moozzyk
Copy link
Contributor

moozzyk commented Apr 16, 2014

🚢🇮🇹

@DamianEdwards
Copy link
Member

:shipit:

@@ -63,7 +62,7 @@
}

if (reconnecting) {
reconnectTimeout = window.setTimeout(function () {
connection._.reconnectAttemptTimeoutHandle = window.setTimeout(function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Capture connection._. and save in a variable, cleaner since you use it a few times below.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, this is dependent on stop clearing the timeout before calling start again, may want to clear the timeout before creating a new one just to ensure nothing wrong ever happens here. If it was C# there'd be a Debug.Assert 😄

Completely optional since it is functionally correct based on how the code is today though.

@NTaylorMullen
Copy link
Contributor

🐑 it

@halter73 halter73 merged commit 41cba31 into dev Apr 18, 2014
@halter73 halter73 deleted the bug-2925 branch May 21, 2014 21:37
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.

4 participants