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

Add tests and docs for using the agent, agentClass and agentOptions. Forever option defaults to using http(s).Agent in node 0.12+ #1671

Merged
merged 2 commits into from Jul 20, 2015

Conversation

Projects
None yet
2 participants
@simov
Copy link
Member

commented Jul 10, 2015

Fixes #1637
Fixes #1669

Feedback: @mikeal @devtristan @triccardi-systran and anyone else interested in

  • Take a look at the agent tests here. Just a bunch of simple use cases, also note that the forever option defaults to http(s).Agent in node 0.12+
  • Take a look at the docs as well. A little bit down below there is a section for agent related options. It would be great if you read through all of them carefully and let me know if I'm not getting something right.
Add tests and docs for using the agent, agentClass and agentOptions
Forever option defaults to using http(s).Agent in node 0.12+
@simov

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2015

@mikeal @devtristan @triccardi-systran (+ anyone else) any feedback on this one?

@madd512

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2015

I don't love forever being 0.10 only, as the docs say, but the code clearly supports the option in more recent versions. Why not get rid of the console.warn and keep forever as an option available for all versions?

@simov

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2015

Probably a more appropriate way to document the forever option would be to say that it defaults to http.Agent in node 0.12+ which was suggested by @mikeal, also that's the way to get rid of potential bugs such as the memory leak one in your (@devtristan) issue here #1637. The console.warn can be removed in that case I agree.

@simov

This comment has been minimized.

Copy link
Member Author

commented Jul 17, 2015

@devtristan updated the docs and removed the warning.

simov added a commit that referenced this pull request Jul 20, 2015

Merge pull request #1671 from simov/agent-docs
Add tests and docs for using the agent, agentClass, agentOptions and forever options.
Forever option defaults to using http(s).Agent in node 0.12+

@simov simov merged commit a2aea33 into request:master Jul 20, 2015

3 checks passed

codecov/commit 100.00% (min required 80%)
Details
codecov/project 88.24% (min required 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.