no_proxy support #1176
no_proxy support #1176
Conversation
|
Sorry for the delay. This needs some tests but I'm kind of surprised at how complex it still is. Is |
|
I'd really like to see that PR get merged. For npm config exporting |
|
Hi, sorry I thought I replied to this last week, must have not pressed send. Yeah I noticed that PR was open and included it to future proof it but their work on it seems to have gone by the wayside. I can have a look at the tests I'm just trying to understand them at the moment and work out something that works correctly. |
|
@bnoordhuis wrote some small unit tests as I got some free time |
| @@ -423,6 +424,44 @@ function install (gyp, argv, callback) { | |||
|
|
|||
| } | |||
|
|
|||
| function formatHostname (hostname) { | |||
| // canonicalize the hostname, so that 'oogle.com' won't match 'google.com' | |||
| return hostname.replace(/^\.*/, '.').toLowerCase() | |||
There was a problem hiding this comment.
This transforms google.com into .google.com. I infer that is intentional but can you update the comment to make that more explicit? This behavior also seems only lightly tested.
Aside: can you capitalize and punctuate comments?
| var zoneParts = zone.split(':', 2) | ||
| var zoneHost = formatHostname(zoneParts[0]) | ||
| var zonePort = zoneParts[1] | ||
| var hasPort = zone.indexOf(':') > -1 |
There was a problem hiding this comment.
Not wrong per se but can also be derived without re-scanning from zonePort !== undefined or zoneParts.length === 2.
| var isMatchedAt = hostname.indexOf(noProxyZone.hostname) | ||
| var hostnameMatched = ( | ||
| isMatchedAt > -1 && | ||
| (isMatchedAt === hostname.length - noProxyZone.hostname.length) |
There was a problem hiding this comment.
Correct me if I'm wrong but isn't this just a circuitous way of saying hostname === noProxyZone.hostname?
| var noProxyUrl = gyp.opts.no_proxy | ||
| || env.no_proxy | ||
| || env.NO_PROXY | ||
| || env.npm_config_no_proxy |
There was a problem hiding this comment.
This should probably go; the npm pull request wasn't merged.
| || env.npm_config_no_proxy | ||
| || '' | ||
|
|
||
| if (noProxyUrl === '*' || (noProxyUrl !== '' && uriInNoProxy(url, noProxyUrl))) { |
There was a problem hiding this comment.
Long line; can you keep it under 80 columns? The parens around the && aren't necessary.
| log.verbose('download', 'using proxy url: "%s"', proxyUrl) | ||
| requestOpts.proxy = proxyUrl | ||
| } else { | ||
| log.warn('download', 'ignoring invalid "proxy" config setting: "%s"', proxyUrl) |
| @@ -0,0 +1,63 @@ | |||
| var test = require('tape') | |||
|
|
||
| t.equal(req.opts.proxy, gyp.opts.proxy) | ||
| t.equal(req.opts.uri, url) | ||
|
|
There was a problem hiding this comment.
Can you drop the blank line here and below?
|
ping @ballwood |
|
@bnoordhuis is someone taking care of this PR? Seems like @ballwood is no longer working on it. It would be really nice to have in master. |
|
This looks pretty good and we have |
|
@rvagg I was looking at doing just this given this is quite a pain point inside our corporate network, however is of this approach to make it independent of requests? Would it be simpler to set the environment variables from the cli flags as needed and let requests do its job? |
|
@imatlopez you mean |
Ported code across from the request library, which I know to work correctly - https://github.com/request/request.
Unsure of where to put the unit/integration tests, if someone could guide me I'll write them. There were 11 tests failing when running npm test when I forked and the same amount after I made the code change.