Use no_proxy when self.proxy is falsey

[tauren]

When self.proxy === undefined or self.proxy === null, the no_proxy logic is not executed. Change test to look for falsey value of self.proxy instead of existence of property.

See #1194 for more details.

[nylen]

It looks to me like when (for example) self.proxy === null, that block will be skipped, so no proxy will be used? How does this PR change the behavior?

[nylen]

Also, this breaks the proxy: null should override HTTP_PROXY test in tests/test-proxy.js (see the Travis build output for details.)

[FredKSchott]

@nylen: @tauren explains it pretty well in #1194, but !{proxy: null}.hasOwnProperty('proxy') evaluates to false.

@tauren +1 for the concept, but you'll need to be more careful since it looks like there's some side effects around that code/behavior (see broken test)

[nylen]

@FredKSchott sure, so if proxy: null we skip that block, which has the desired effect of never using a proxy, right?

[FredKSchott]

Ahhhhh I see what you mean now :) Yea, that definitely needs to be handled

[nylen]

@FredKSchott @tauren if there's a problem we want to fix it. However, given the discussion above, I don't understand what this change is actually doing / supposed to do.

[tauren]

My objective is to get NO_PROXY settings working with npm. At this time, if HTTP_PROXY (etc) is defined, npm sets an explicit proxy setting when calling request. Request then doesn't honor the NO_PROXY setting.

I've submitted a PR to npm that removes any knowledge of HTTP_PROXY (etc) from npm so that request can handle the env vars itself. But npm requires a default proxy value, which is null if no proxy env vars exist. So now with my PR, npm will be passing proxy: null to request.
npm/npm#6525

Bottom line is npm needs to honor HTTP_PROXY, HTTPS_PROXY, and NO_PROXY. Currently, it doesn't honor NO_PROXY. It seems to me the best solution is for npm to pass that responsibility on to request and remove env-var based proxy knowledge from npm. It still should override the proxy with .npmrc and command-line options, but let request handle env-vars.

I'm open to suggestions on how to solve this. Should npm or request be changed? Or both? Obviously, its not a good idea to change current request behavior. I had previously missed where the docs indicate that proxy: null forces no proxy in request.

Maybe npm should do something like delete config.proxy before making requests, since by default it sets config.proxy to null. But that means npm has to handle the proxy setting differently than it does other settings.

[tauren]

Current status of adding NO_PROXY support to npm:

1. @othiym23 indicated he would like to land PR Remove proxy settings from defaults npm/npm#6525 into npm. I've updated the PR to address some concerns and hope to have it accepted soon.
2. npm-request-client has merged PR Support no_proxy with latest request package npm/npm-registry-client#75, so npm is now using a version of request that supports NO_PROXY
3. the request PR Use no_proxy when self.proxy is falsey #1195 conversation has pointed out that proxy: null has special meaning (forces no proxying), which is why hasOwnProperty is used: https://github.com/request/request/blob/master/request.js#L418

At this point, I'm not convinced my npm PR will help considering the proxy: null behavior of request. Furthermore, it looks like my request PR would break existing functionality. A clear solution isn't obvious to me, and I think it might be best for @othiym23 and @nylen to discuss this issue and determine a good approach to solve it. I'm more than happy to help, but the two of you can better identify an appropriate solution.

[tauren]

After thinking about this further, it seems an effective solution would be for self.proxy === null and self.proxy === undefined and !self.hasOwnProperty('proxy') to use the environment variable settings (HTTP_PROXY, NO_PROXY, etc.). Only if proxy === false would proxying be skipped altogether. In other words:

  if(self.proxy !== false) {
    self.proxy = getProxyFromURI(self.uri)
  }

However, I assume this would be a breaking change and probably isn't possible.

[nylen]

To avoid a breaking change, we could keep null as meaning no proxy, but make it so that undefined uses the environment variable settings.

Though, wouldn't it be easier for npm to just do delete requestOptions.proxy? Is there a reason why that wouldn't work?

[othiym23]

Landed in npm/npm@40afd6a and npm/npm@294b13a, with npm-registry-client@4.0.2. I may make a few tweaks to the docs to describe the change more clearly (because I bet something about this is going to be surprising to a certain number of proxy users). Thanks for putting this together and riding herd on it, @tauren!

[othiym23]

In other words, nothing needed to be done for request! 🎉

[nylen]

Closing this issue then, thanks guys!