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

Basic proxy support #159

Closed
wants to merge 1 commit into from
Closed

Basic proxy support #159

wants to merge 1 commit into from

Conversation

tgandrews
Copy link

This PR is intended to start a discussion, possibly get some extra help and feedback on the direction It aims to resolve the issue #79

N.B. I am working on removing the spacing and line ending changes to make the PR more readable.


Added really basic proxy support. It works for the simple use case of
got("reddit.com") which will first make a http request and then be
redirected to https. This also supports requests that are redirected and
over to unsecure locations i.e. they start http and stay http.

Currently requests that require https to begin with are not supported.

Added really basic proxy support. It works for the simple use case of
got("reddit.com") which will first make a http request and then be
redirected to https. This also supports requests that are redirected and
over to unsecure locations i.e. they start http and stay http.

Currently requests that require https to begin with are not supported.
@sindresorhus
Copy link
Owner

From that issue:

I would prefer for the solution to be an external module got can just depend on as I don't want to have to maintain it. Maybe https://github.com/mikeal/tunnel-agent.

@tgandrews
Copy link
Author

Sorry, I should have read the issue properly.

I will move it. I guess you would rather have it replace http/https?

Probably will make it easier from my PoV as well.
On Wed, 20 Jan 2016 at 10:59, Sindre Sorhus notifications@github.com
wrote:

From that issue:

I would prefer for the solution to be an external module got can just
depend on as I don't want to have to maintain it. Maybe
https://github.com/mikeal/tunnel-agent.


Reply to this email directly or view it on GitHub
#159 (comment).

@tgandrews
Copy link
Author

Closing as will create a new one referencing a new npm lib.

@tgandrews tgandrews closed this Jan 20, 2016
@sindresorhus
Copy link
Owner

I guess you would rather have it replace http/https?

Yes, or even better, a custom http agent.

@sindresorhus sindresorhus mentioned this pull request Jan 23, 2016
@stevenvachon
Copy link
Contributor

@tgandrews
Copy link
Author

I have a branch with this working. No tests yet. I will push on Monday once
I have some tests working.
On Sat, 23 Jan 2016 at 16:45, Steven Vachon notifications@github.com
wrote:

https://github.com/TooTallNate/node-proxy-agent ?


Reply to this email directly or view it on GitHub
#159 (comment).

@sindresorhus sindresorhus mentioned this pull request Feb 25, 2016
@alexsantos
Copy link

@tgandrews , I've seen your branch but what I miss is a way where got uses proxies in a transparent way, without passing a opts.proxy value. This follows the need for proxies when using tools like npm-name, generator-node, etc... I've did some work on #178 following the approach from request but @sindresorhus pointed out how wrong it could get 👎

@tgandrews
Copy link
Author

@alexsantos I'll try and dig out the branch. I had a bit of down time in a new job (with big company network setup) but have not looked at this for a while.

@alexsantos
Copy link

@tgandrews and @sindresorhus , I have another branch which I think is the minimalist solution for this. It depends on proxy-from-env and proxy-agent modules. Can you have a look here? https://github.com/alexsantos/got/tree/proxy-from-env/index.js
Basically, it's 6 lines of code...

if (!opts.agent) {
  const proxy = getProxyForUrl(url);
  if (proxy) {
    opts.agent = new ProxyAgent(proxy);
  }
}

I could have done a PR but didn't want to bug Sindre again :-)

@sindresorhus
Copy link
Owner

@alexsantos Yes, something like that is what I was thinking. Unfortunately the proxy-agent dependency takes up 7.4 MB, which is way too much.

@sindresorhus
Copy link
Owner

// @floatdrop

@floatdrop
Copy link
Contributor

May be same trick as in http2 issue can be applied here as well?

@sindresorhus
Copy link
Owner

@floatdrop Unfortunately not to achieve what most people requesting this want, as they want modules or modules of modules using got somewhere in the chain to support proxies transparently. Should be possible to find/create a smaller dependency though.

@alexsantos
Copy link

@floatdrop , just to back @sindresorhus message, I came here because I want to use yeoman generators at work and I can't because of the lack of proxy support on got.

@tgandrews
Copy link
Author

@sindresorhus @alexsantos My branch was using proxy-agent as well. I guess we need to support just HTTP and HTTPS proxies? If we are doing the whole whack then I doubt we are going to do better than node-proxy-agent

@stevenvachon
Copy link
Contributor

This feature is probably one of the things that makes request lib as big as it is.

@alexsantos
Copy link

@tgandrews The size of node-proxy-agent is derived from the fact that it supports PAC and that module specfically needs degenerator/esprima which is a huge dependency. If we abandon PAC then it gets pretty small. I've been doing some work on that module so I'll might drop PAC and fork it to a smaller one.

@tgandrews
Copy link
Author

OK, I guess there are two options:

  1. Fork node-proxy-agent and remove PAC support.
  2. Work on node-pac-proxy-agent and reduce the external dependencies to make it smaller. Then node-proxy-agent can be taken wholesale with PAC support.

And @alexsantos is looking(?) into number 1, so I'll have a look at number 2 then if I get some free time.

@tgandrews tgandrews deleted the adding-proxy-support branch March 7, 2016 14:58
@alexsantos
Copy link

@tgandrews , I already have a stripped version of node-pac-resolver (now with Promises) that gets rid of regenerator. https://github.com/alexsantos/node-pac-resolver/tree/nodev4-compat
Forking the whole node-proxy-agent would also bring this modules to got standards, like v4, xo, nyc, etc...

@tgandrews
Copy link
Author

@alexsantos Let me know if there is anything I can do to help.

@stevenvachon
Copy link
Contributor

Why fork node-proxy-agent when you could create a PR to bring it up to date? Or was that the intent?

@alexsantos
Copy link

@stevenvachon , node-proxy-agent has pac-proxy support, which is a huge dependency for got.

@stevenvachon
Copy link
Contributor

Do we not need to support pac and socks proxies?

@bhushankummar
Copy link

Does it support socks5 proxy?
socks5://127.0.0.1:3526

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.

None yet

6 participants