Remove webocket native dependency. #3578
Conversation
| @@ -16,7 +16,7 @@ | |||
| "eventemitter3": "^4.0.0", | |||
| "underscore": "1.9.1", | |||
| "web3-core-helpers": "1.2.8", | |||
| "websocket": "^1.0.31" | |||
| "ws": "^7.3.0" | |||
There was a problem hiding this comment.
According to the documentation, this module doesn't work in the browser.
I think we need two implementations of this provider, one for node using ws, and another one for the browser using the native WebSocket implementation. They can share most of their code.
The browser version should be declared in the package.json's browser field: https://docs.npmjs.com/files/package.json#browser
| @@ -101,7 +101,10 @@ WebsocketProvider.prototype.constructor = WebsocketProvider; | |||
| * @returns {void} | |||
| */ | |||
| WebsocketProvider.prototype.connect = function () { | |||
| this.connection = new Ws(this.url, this.protocol, undefined, this.headers, this.requestOptions, this.clientConfig); | |||
| // TODO: include this.requestOptions and this.clientConfig | |||
test/websocket.ganache.js
Outdated
| assert(err.message.includes('1012')); | ||
| assert(err.message.includes('restart')); | ||
| web3.currentProvider.once('close', function(event){ | ||
| assert.strictEqual(event.code, 1000); |
There was a problem hiding this comment.
Believe this test should continue work as expected. The disconnect uses an error code. Test was added in #3486.
|
There's an (abandoned) PR converting to N-API in the current Websocket repo fwiw. |
|
For background - this PR is part of a NomicLabs led initiative to purge the JS Eth stack of dependencies that require node-gyp to install. It's the source of many problems as new Node versions come out and has bad installation UX. In some cases this is straightforward and the substitution is painless. In others there are tradeoffs to consider and this looks like one of them... So far the proposed replacement's issues include:
cc: @GregTheGreek |
|
Thanks for raising these points, @cgewecke! We are now exploring fixing this in |
|
This PR has been automatically marked as stale beacause it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. If you believe this was a mistake, please comment. |
|
This as been superseded by #3686 |
Description
This PR removes websocket dependency and instead uses ws since it doesn't require native dependency compiling.
Type of change
Checklist:
npm run dtslintwith success and extended the tests and types if necessary.npm run test:unitwith success.npm run test:covand my test cases cover all the lines and branches of the added code.npm run build-alland tested the resulting files fromdistfolder in a browser.CHANGELOG.mdfile in the root folder.