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

Allow any custom agent instead of just follow-redirects #1285

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hasnat
Copy link

@hasnat hasnat commented Jul 28, 2018

No description provided.

@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@a3fe02d). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1285   +/-   ##
=========================================
  Coverage          ?   92.33%           
=========================================
  Files             ?        6           
  Lines             ?      313           
  Branches          ?        0           
=========================================
  Hits              ?      289           
  Misses            ?       24           
  Partials          ?        0
Impacted Files Coverage Δ
lib/http-proxy/passes/web-incoming.js 98.3% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a3fe02d...52ac7cd. Read the comment docs.

@codebling
Copy link

There's already an agent option, but it doesn't look like it's fully implemented. What do you think about using agent instead of httpAgent?

@hasnat
Copy link
Author

hasnat commented Jan 22, 2022

There's already an agent option, but it doesn't look like it's fully implemented. What do you think about using agent instead of httpAgent?

you're right; agent + httpAgent certainly makes things confusing; possibly docs update to clarify better?

code wise it looks like this

http.request(options);
  ^               ^  
  |               |
httpAgent         |
                agent

e.g. lib/http-proxy/passes/web-incoming.js:125

my use case was to replace tls with custom module for https requests

@codebling
Copy link

Oh, I see - this is where we could use something like e.g. axios or got instead of actually using http module's .request() with something that http.Agent, right? But httpAgent here would still need to implement the http.ClientRequest interface? So...not axios or got? Wouldn't the TLS changes have mostly been specific to the agent anyhow?

There's probably room for refactoring to provide a single configuration that allows both (e.g. hooks for creating connection and getting the stream), but it's probably moot anyhow, since this your PR is going on 4 years old, there have been no commits in almost 2 years. Perhaps a strong ping in the "New Maintainers" issue would receive a response, as indexzero does seem to still be relatively active on GitHub.

@hasnat
Copy link
Author

hasnat commented Jan 24, 2022

thanks, your mentions probably already have alerted maintainers. let's see what's suggested.

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