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

types(web3-core-helpers): fix disconnect args #3218

Closed
wants to merge 1 commit into from

Conversation

lautarodragan
Copy link
Contributor

@lautarodragan lautarodragan commented Nov 19, 2019

This PR makes WebsocketProviderBase.disconnect arguments optional in the type declaration file, since they have a default value in the JavaScript implementation for the 2.x branch and the arguments aren't even present in the 1.x branch, and they should be optional per the MDN WebSocket.close() docs.

The correct change would be to actually remove the arguments in the 1.x branch and make them optional in the 2.x branch, but that would be an unnecessary breaking change for the 1.x branch without much added value (other than to warn users that the arguments they are passing won't do anything).

I have only tested this by manually introducing the change to the installed library in my project under node_modules and running npm run build in my project, which uses Web3.

Will open a PR with the same change for the 2.x branch if this one is accepted.

Relevant code:

https://github.com/ethereum/web3.js/blob/5c78c2b7cfa37afedfe1ac020bf912b3e85cfd8a/packages/web3-providers-ws/src/index.js#L401-L405

https://github.com/ethereum/web3.js/blob/8a3b5a7e1e07a5bd7b76a92005870c80cb96d521/packages/web3-providers/src/providers/WebsocketProvider.js#L132-L144

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 test with success and extended the tests if necessary.
  • I ran npm run build and tested the resulting file from dist folder in a browser.
  • I have tested my code on the live network.

This commit makes WebsocketProviderBase.disconnect arguments optional in the type declaration file, since they have a default value in the JavaScript implementation for the 2.x branch and the arguments aren't even present in the 1.x branch.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 84.347% when pulling 27a5e7f on lautarodragan:patch-3 into 5c78c2b on ethereum:1.x.

@nivida
Copy link
Contributor

nivida commented Nov 19, 2019

Thanks for proposing this PR! Because we updated the disconnect method in the on-going RequestManager and WebsocketProvider improvements PR (#3190) is this change no longer required.

@nivida nivida closed this Nov 19, 2019
@lautarodragan
Copy link
Contributor Author

Ohh, sorry I missed that PR! Glad to see providers getting a rework.

Thanks for reviewing this PR though!

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