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

Added support for custom http agent #2980

Merged
merged 11 commits into from
Jan 8, 2020

Conversation

AsyncLegs
Copy link

@coveralls
Copy link

coveralls commented Jul 29, 2019

Coverage Status

Coverage decreased (-0.07%) to 84.804% when pulling 60d1b93 on AsyncLegs:custom-http-agent-support into 7918661 on ethereum:1.x.

@nivida nivida added 1.x 1.0 related issues Enhancement Includes improvements or optimizations In Progress Currently being worked on labels Jul 29, 2019
@AsyncLegs
Copy link
Author

Hi guys, is there any chances to merge these enhancements to version 1.x?

packages/web3-providers-http/src/index.js Outdated Show resolved Hide resolved
@0xjjpa 0xjjpa mentioned this pull request Sep 6, 2019
@nivida nivida added Review Needed Maintainer(s) need to review and removed In Progress Currently being worked on labels Jan 6, 2020
@nivida nivida requested a review from cgewecke January 6, 2020 15:02
@nivida
Copy link
Contributor

nivida commented Jan 6, 2020

@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.

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.

@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?

@nivida
Copy link
Contributor

nivida commented Jan 7, 2020

@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.

I will add those missing test cases 👌

Also WDYT about adding withCredentials and agent to the usage example in the web3-providers-http README?

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).

@nivida nivida requested a review from cgewecke January 7, 2020 09:30
test/httpprovider.js Outdated Show resolved Hide resolved
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.

@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).

@nivida nivida removed the Review Needed Maintainer(s) need to review label Jan 8, 2020
@nivida nivida merged commit b89a4da into web3:1.x Jan 8, 2020
@nivida nivida mentioned this pull request Jan 16, 2020
10 tasks
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 Enhancement Includes improvements or optimizations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants