Skip to content
This repository was archived by the owner on Mar 5, 2025. It is now read-only.

Nikos/5835/websocket provider keeps important error message back #5884

Merged

Conversation

nikoulai
Copy link
Contributor

@nikoulai nikoulai commented Mar 2, 2023

Description

closes #5835

Checklist for 1.x:

  • 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:cov and my test cases cover all the lines and branches of the added code.
  • I ran npm run build with success.
  • I have tested the built dist/web3.min.js in a browser.
  • I have tested my code on the live network.
  • I have checked the Deploy Preview and it looks correct.
  • I have updated the CHANGELOG.md file in the root folder.

Sorry, something went wrong.

nikoulai added 2 commits March 1, 2023 18:35

Verified

This commit was signed with the committer’s verified signature.
nikoulai Nikos Iliakis

Verified

This commit was signed with the committer’s verified signature.
nikoulai Nikos Iliakis
@nikoulai nikoulai added the 1.x 1.0 related issues label Mar 2, 2023
@nikoulai nikoulai self-assigned this Mar 2, 2023
nikoulai added 4 commits March 2, 2023 13:38

Verified

This commit was signed with the committer’s verified signature.
nikoulai Nikos Iliakis

Verified

This commit was signed with the committer’s verified signature.
nikoulai Nikos Iliakis

Verified

This commit was signed with the committer’s verified signature.
nikoulai Nikos Iliakis

Verified

This commit was signed with the committer’s verified signature.
nikoulai Nikos Iliakis
@coveralls
Copy link

coveralls commented Mar 2, 2023

Pull Request Test Coverage Report for Build 4344683272

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 458 unchanged lines in 9 files lost coverage.
  • Overall coverage decreased (-2.4%) to 72.59%

Files with Coverage Reduction New Missed Lines %
packages/web3-core-helpers/lib/errors.js 1 88.89%
packages/web3-core-requestmanager/src/jsonrpc.js 2 82.76%
packages/web3-providers-ws/lib/index.js 16 89.72%
packages/web3-core-helpers/src/formatters.js 25 80.58%
packages/web3-core-helpers/src/errors.js 34 4.17%
packages/web3-utils/src/soliditySha3.js 55 5.13%
packages/web3-utils/src/index.js 58 36.89%
packages/web3-utils/src/utils.js 98 12.87%
packages/web3-eth-accounts/src/index.js 169 23.26%
Totals Coverage Status
Change from base Build 4341355753: -2.4%
Covered Lines: 3466
Relevant Lines: 4493

💛 - Coveralls

Copy link
Contributor

@jdevcs jdevcs left a comment

Choose a reason for hiding this comment

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

also changelog update is required

nikoulai added 2 commits March 3, 2023 11:52

Verified

This commit was signed with the committer’s verified signature.
nikoulai Nikos Iliakis

Verified

This commit was signed with the committer’s verified signature.
nikoulai Nikos Iliakis
@@ -207,7 +247,11 @@ WebsocketProvider.prototype._addSocketListeners = function () {
this.connection.addEventListener('message', this._onMessage.bind(this));
this.connection.addEventListener('open', this._onConnect.bind(this));
this.connection.addEventListener('close', this._onClose.bind(this));
};
if(this.connection._connection){
this.connection._client.removeAllListeners('connectFailed'); //Override the internal listeners, so they don't trigger a `close` event. We want to trigger `_onClose` manually with a description.
Copy link
Contributor

@jdevcs jdevcs Mar 3, 2023

Choose a reason for hiding this comment

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

As browsers use the native WebSocket object + doesn't use/have ws lib client and node uses ws client. so can we more specifically check _client here?

Copy link
Contributor

Choose a reason for hiding this comment

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

And do we also need to handle the same for the browser WebSocket? Or is it only an issue that we need to handle just for node?

Verified

This commit was signed with the committer’s verified signature.
nikoulai Nikos Iliakis

Verified

This commit was signed with the committer’s verified signature.
nikoulai Nikos Iliakis
Copy link
Contributor

@avkos avkos left a comment

Choose a reason for hiding this comment

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

LGTM

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…-error-message-back
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
1.x 1.0 related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WebsocketProvider keeps important error messages back.
5 participants