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

Rework connect saga to support connectivity status and reconnection #1649

Merged
merged 16 commits into from
Jan 30, 2019

Conversation

compulim
Copy link
Contributor

@compulim compulim commented Jan 27, 2019

This PR is pre-requisite of #1521 and related PRs.

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

@coveralls
Copy link

coveralls commented Jan 27, 2019

Pull Request Test Coverage Report for Build 888

  • 34 of 61 (55.74%) changed or added relevant lines in 3 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.05%) to 52.542%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/core/src/sagas/connectSaga.js 31 58 53.45%
Files with Coverage Reduction New Missed Lines %
packages/core/src/sagas/effects/whileConnected.js 1 83.33%
Totals Coverage Status
Change from base Build 886: 0.05%
Covered Lines: 882
Relevant Lines: 1526

💛 - Coveralls

Copy link
Contributor

@a-b-r-o-w-n a-b-r-o-w-n left a comment

Choose a reason for hiding this comment

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

Looks good. I mostly just have questions, but a couple of suggestions as well.

packages/component/src/BasicWebChat.js Show resolved Hide resolved
packages/core/src/sagas/connectSaga.js Outdated Show resolved Hide resolved
packages/core/src/sagas/connectSaga.js Show resolved Hide resolved
packages/core/src/sagas/connectSaga.js Outdated Show resolved Hide resolved
packages/core/src/sagas/connectSaga.js Outdated Show resolved Hide resolved
@@ -94,7 +95,7 @@ export default class extends React.Component {
domain,
fetch,
token: directLineToken,
webSocket: webSocket === 'true' || +webSocket
webSocket: webSocket === 'true' || !!+webSocket
Copy link
Contributor

Choose a reason for hiding this comment

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

Could just be Boolean(webSocket)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

I specifically mean !!+webSocket can be Boolean(webSocket)

packages/playground/src/App.js Show resolved Hide resolved
packages/playground/src/App.js Show resolved Hide resolved
@compulim compulim merged commit 17b253d into microsoft:master Jan 30, 2019
@compulim compulim deleted the feat-rework-connect-saga branch January 30, 2019 07:39
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