-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Conversation
|
Hi guys, is there any chances to merge these enhancements to version 1.x? |
|
@cgewecke I've applied the requested changes as described above from me. I've noticed while improving the options handling that the existing HTTP provider test cases aren't that good as they could be but this has to get addressed with a separate PR. |
cgewecke
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nivida Is it a problem to add a simple test using the new option though?
To make sure the changes work as expected and the option is described by the tests in case someone wants to see a working example.
Also WDYT about adding withCredentials and agent to the usage example in the web3-providers-http README?
I will add those missing test cases 👌
Yep, I will add those options to the README. Btw.: An issue is existing to add and improve the provider documentations to the web3.js documentation on readthedocs (#3117). |
cgewecke
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nivida Looks good! Nice.
There's a .only on the new test which is stopping the complete test suite from running. (Tried to commit it but I don't have permission on this branch).
Thus we will able to use custom HTTP agents such as https://github.com/koichik/node-tunnel or https://www.npmjs.com/package/https-proxy-agent