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

WebsocketProvider - Error Handling #3314

Merged
merged 3 commits into from
Jan 16, 2020

Conversation

nivida
Copy link
Contributor

@nivida nivida commented Jan 15, 2020

Description

I've compared the current errors with the errors we do throw in the refactored WebsocketProvider and adjusted those who would introduce a breaking change.

During the comparison of those did I notice that the old WebsocketProvider does have two different timeouts. The first place the old provider does check for a timeout is in the _parseResponse method and is per chunk and the other configurable timeout in _addResponseCallback is on a per request base. I will check that closer to be sure it won't break anything.

Type of change

  • Error Handling

Checklist:

  • I have selected the correct base branch.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • Any dependent changes have been merged and published in downstream modules.
  • I ran npm run dtslint with success and extended the tests and types if necessary.
  • I ran npm run test:unit with success and extended the tests if necessary.
  • I ran npm run build-all and tested the resulting file/'s from dist folder in a browser.
  • I have updated the CHANGELOG.md file in the root folder.
  • I have tested my code on the live network.

…nges removed. Known error objects extended with 'code' and 'reason' property.
@coveralls
Copy link

coveralls commented Jan 15, 2020

Coverage Status

Coverage increased (+0.5%) to 88.042% when pulling 3ca9b09 on pri/error-handling into 7b6a389 on provider-related-improvements.

@nivida nivida requested a review from cgewecke January 15, 2020 10:50
@nivida nivida added 1.x 1.0 related issues Review Needed Maintainer(s) need to review labels Jan 15, 2020
@nivida nivida marked this pull request as ready for review January 15, 2020 10:50
Copy link
Collaborator

@cgewecke cgewecke left a comment

Choose a reason for hiding this comment

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

Yes, makes sense and looks good! 👍

@nivida nivida removed the Review Needed Maintainer(s) need to review label Jan 16, 2020
@nivida nivida merged commit cd680eb into provider-related-improvements Jan 16, 2020
@nivida nivida deleted the pri/error-handling branch January 16, 2020 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.x 1.0 related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants