Rework connect saga to support connectivity status and reconnection#1649
Merged
compulim merged 16 commits intomicrosoft:masterfrom Jan 30, 2019
Merged
Rework connect saga to support connectivity status and reconnection#1649compulim merged 16 commits intomicrosoft:masterfrom
compulim merged 16 commits intomicrosoft:masterfrom
Conversation
compulim
requested review from
a-b-r-o-w-n,
corinagum,
cwhitten,
justinwilaby and
tonyanziano
as code owners
Pull Request Test Coverage Report for Build 888
💛 - Coveralls |
a-b-r-o-w-n
requested changes
Jan 28, 2019
Contributor
a-b-r-o-w-n
left a comment
a-b-r-o-w-n
left a comment
There was a problem hiding this comment.
Looks good. I mostly just have questions, but a couple of suggestions as well.
| fetch, | ||
| token: directLineToken, | ||
| webSocket: webSocket === 'true' || +webSocket | ||
| webSocket: webSocket === 'true' || !!+webSocket |
Contributor
There was a problem hiding this comment.
Could just be Boolean(webSocket)
Contributor
Author
There was a problem hiding this comment.
Today, it is either ?ws=true, ?ws=0 or ?ws=1. Otherwise, if we are treating it as boolean, I think we could just do webSocket !== 'false' (non-true/false, will by default turns to true).
Contributor
There was a problem hiding this comment.
I specifically mean !!+webSocket can be Boolean(webSocket)
compulim
force-pushed
the
feat-rework-connect-saga
branch
from
99888fb to
2e64d55
Compare
a-b-r-o-w-n
approved these changes
Jan 29, 2019
compulim
force-pushed
the
feat-rework-connect-saga
branch
from
000730e to
c3c02dd
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Today, we do not support failed to connect initially and interrupted connection. Both features are also not supported in DLJS currently. In addition, we also do not support abort before a connection is made.
This PR will enable those scenarios. And we will work on support from DLJS side.
Changelog
Changed
core: Reworked logic on connect/disconnect for reliability on handling corner cases, by @compulim in PR #1649