lib: Match proxy support with request module

[imatlopez]

Checklist

• npm install && npm test passes
• documentation is changed or added
• commit message follows commit guidelines

Description of change

Per #1176 (comment), closes#1751

When using the no_proxy environmental variable I noticed that some installations would fail when NODE_CONFIG_PROXY was set (or its equivalent). From inspecting this code plus how its dependency, https://github.com/request/request, I noticed it already has full proxy support. Changing env itself might lead to side effects.

• Prevents --proxy to blocking no_proxy from https://github.com/request/request/blob/212570b6971a732b8dd9f3c73354bcdda158a737/request.js#L276-L278
• Adds no_proxy support through CLI --noproxy or equivalent NPM alternatives.
• Possibly breaking: Passing proxy effectively blocks https_proxy currently, this also unblocks that variable (which would override ourproxy variable). However I am mirroring the http_proxy and https_proxy definitions and then letting the dependency to pick either. (Implemention downstream in: https://github.com/request/request/blob/212570b6971a732b8dd9f3c73354bcdda158a737/lib/getProxyFromURI.js#L40)

[rvagg]

It's unfortunate we have to include so much additional logic just to get proper no_proxy support, we're basically digging a bigger hole because of the support for non-standard methods of specifying proxies. One option might be to just rip it out and tell the user to just use standard *_PROXY env vars and be done with it but that's probably going to be too painful for enterprise users that have this baked in already.

Regarding the breaking change - would it really be breaking? In this and the current version, gyp.opts.proxy clobbers whatever you have set. If you have HTTPS_PROXY set only then in the current version it'll flow through and request will handle it properly, in this version you're handling it in the same way as request would have handled it previously. When you provide a proxy option to request, it won't even invoke the getProxyFromURI.js logic as far as I can tell, so it's clobbered by what's done here.

I could imagine some weirdness if you had a proxy setting that didn't pass our basic regex: if (/^https?:\/\//i.test(proxyUrl)) {. request does a url.parse() which I think is assumed to throw if it's bad and so there's scope for some permutation of a proxy setting in an env var that's not picked up here but is picked up by request. But, you'd get an informational message here and you're probably not doing something sensible if you're getting that anyway .. I think.

[imatlopez]

but that's probably going to be too painful for enterprise users that have this baked in already

This is actually why I am here, the only way I make proxies work is to not specify them outside env var.

When you provide a proxy option to request, it won't even invoke the getProxyFromURI.js logic as far as I can tell, so it's clobbered by what's done here.

Exactly, which is why no_proxy is never handled correctly. My initial concern was with env vars leaking elsewhere if they were changed within the program, so I ripped out all the request code related to it (including the url.parse part--which is deprecated sadly). A benefit is that proxy behavior is now set in stone if request goes away.

[imatlopez]

@rvagg anything pending here?

[rvagg]

landed in 3bcba2a, labelled semver-minor

[imatlopez]

Checklist

• npm install && npm test passes
• documentation is changed or added
• commit message follows commit guidelines

Description of change

Per #1176 (comment), closes#1751

When using the no_proxy environmental variable I noticed that some installations would fail when NODE_CONFIG_PROXY was set (or its equivalent). From inspecting this code plus how its dependency, https://github.com/request/request, I noticed it already has full proxy support. Changing env itself might lead to side effects.

• Prevents --proxy to blocking no_proxy from https://github.com/request/request/blob/212570b6971a732b8dd9f3c73354bcdda158a737/request.js#L276-L278
• Adds no_proxy support through CLI --noproxy or equivalent NPM alternatives.
• Possibly breaking: Passing proxy effectively blocks https_proxy currently, this also unblocks that variable (which would override ourproxy variable). However I am mirroring the http_proxy and https_proxy definitions and then letting the dependency to pick either. (Implemention downstream in: https://github.com/request/request/blob/212570b6971a732b8dd9f3c73354bcdda158a737/lib/getProxyFromURI.js#L40)

[rvagg]

It's unfortunate we have to include so much additional logic just to get proper no_proxy support, we're basically digging a bigger hole because of the support for non-standard methods of specifying proxies. One option might be to just rip it out and tell the user to just use standard *_PROXY env vars and be done with it but that's probably going to be too painful for enterprise users that have this baked in already.

Regarding the breaking change - would it really be breaking? In this and the current version, gyp.opts.proxy clobbers whatever you have set. If you have HTTPS_PROXY set only then in the current version it'll flow through and request will handle it properly, in this version you're handling it in the same way as request would have handled it previously. When you provide a proxy option to request, it won't even invoke the getProxyFromURI.js logic as far as I can tell, so it's clobbered by what's done here.

I could imagine some weirdness if you had a proxy setting that didn't pass our basic regex: if (/^https?:\/\//i.test(proxyUrl)) {. request does a url.parse() which I think is assumed to throw if it's bad and so there's scope for some permutation of a proxy setting in an env var that's not picked up here but is picked up by request. But, you'd get an informational message here and you're probably not doing something sensible if you're getting that anyway .. I think.

[imatlopez]

but that's probably going to be too painful for enterprise users that have this baked in already

This is actually why I am here, the only way I make proxies work is to not specify them outside env var.

When you provide a proxy option to request, it won't even invoke the getProxyFromURI.js logic as far as I can tell, so it's clobbered by what's done here.

Exactly, which is why no_proxy is never handled correctly. My initial concern was with env vars leaking elsewhere if they were changed within the program, so I ripped out all the request code related to it (including the url.parse part--which is deprecated sadly). A benefit is that proxy behavior is now set in stone if request goes away.

[imatlopez]

@rvagg anything pending here?

[rvagg]

landed in 3bcba2a, labelled semver-minor

[imatlopez]

Checklist

• npm install && npm test passes
• documentation is changed or added
• commit message follows commit guidelines

Description of change

Per #1176 (comment), closes#1751

When using the no_proxy environmental variable I noticed that some installations would fail when NODE_CONFIG_PROXY was set (or its equivalent). From inspecting this code plus how its dependency, https://github.com/request/request, I noticed it already has full proxy support. Changing env itself might lead to side effects.

• Prevents --proxy to blocking no_proxy from https://github.com/request/request/blob/212570b6971a732b8dd9f3c73354bcdda158a737/request.js#L276-L278
• Adds no_proxy support through CLI --noproxy or equivalent NPM alternatives.
• Possibly breaking: Passing proxy effectively blocks https_proxy currently, this also unblocks that variable (which would override ourproxy variable). However I am mirroring the http_proxy and https_proxy definitions and then letting the dependency to pick either. (Implemention downstream in: https://github.com/request/request/blob/212570b6971a732b8dd9f3c73354bcdda158a737/lib/getProxyFromURI.js#L40)

[rvagg]

It's unfortunate we have to include so much additional logic just to get proper no_proxy support, we're basically digging a bigger hole because of the support for non-standard methods of specifying proxies. One option might be to just rip it out and tell the user to just use standard *_PROXY env vars and be done with it but that's probably going to be too painful for enterprise users that have this baked in already.

Regarding the breaking change - would it really be breaking? In this and the current version, gyp.opts.proxy clobbers whatever you have set. If you have HTTPS_PROXY set only then in the current version it'll flow through and request will handle it properly, in this version you're handling it in the same way as request would have handled it previously. When you provide a proxy option to request, it won't even invoke the getProxyFromURI.js logic as far as I can tell, so it's clobbered by what's done here.

I could imagine some weirdness if you had a proxy setting that didn't pass our basic regex: if (/^https?:\/\//i.test(proxyUrl)) {. request does a url.parse() which I think is assumed to throw if it's bad and so there's scope for some permutation of a proxy setting in an env var that's not picked up here but is picked up by request. But, you'd get an informational message here and you're probably not doing something sensible if you're getting that anyway .. I think.

[imatlopez]

but that's probably going to be too painful for enterprise users that have this baked in already

This is actually why I am here, the only way I make proxies work is to not specify them outside env var.

When you provide a proxy option to request, it won't even invoke the getProxyFromURI.js logic as far as I can tell, so it's clobbered by what's done here.

Exactly, which is why no_proxy is never handled correctly. My initial concern was with env vars leaking elsewhere if they were changed within the program, so I ripped out all the request code related to it (including the url.parse part--which is deprecated sadly). A benefit is that proxy behavior is now set in stone if request goes away.

[imatlopez]

@rvagg anything pending here?

[rvagg]

landed in 3bcba2a, labelled semver-minor

[imatlopez]

Checklist

• npm install && npm test passes
• documentation is changed or added
• commit message follows commit guidelines

Description of change

Per #1176 (comment), closes#1751

When using the no_proxy environmental variable I noticed that some installations would fail when NODE_CONFIG_PROXY was set (or its equivalent). From inspecting this code plus how its dependency, https://github.com/request/request, I noticed it already has full proxy support. Changing env itself might lead to side effects.

• Prevents --proxy to blocking no_proxy from https://github.com/request/request/blob/212570b6971a732b8dd9f3c73354bcdda158a737/request.js#L276-L278
• Adds no_proxy support through CLI --noproxy or equivalent NPM alternatives.
• Possibly breaking: Passing proxy effectively blocks https_proxy currently, this also unblocks that variable (which would override ourproxy variable). However I am mirroring the http_proxy and https_proxy definitions and then letting the dependency to pick either. (Implemention downstream in: https://github.com/request/request/blob/212570b6971a732b8dd9f3c73354bcdda158a737/lib/getProxyFromURI.js#L40)

[rvagg]

It's unfortunate we have to include so much additional logic just to get proper no_proxy support, we're basically digging a bigger hole because of the support for non-standard methods of specifying proxies. One option might be to just rip it out and tell the user to just use standard *_PROXY env vars and be done with it but that's probably going to be too painful for enterprise users that have this baked in already.

Regarding the breaking change - would it really be breaking? In this and the current version, gyp.opts.proxy clobbers whatever you have set. If you have HTTPS_PROXY set only then in the current version it'll flow through and request will handle it properly, in this version you're handling it in the same way as request would have handled it previously. When you provide a proxy option to request, it won't even invoke the getProxyFromURI.js logic as far as I can tell, so it's clobbered by what's done here.

I could imagine some weirdness if you had a proxy setting that didn't pass our basic regex: if (/^https?:\/\//i.test(proxyUrl)) {. request does a url.parse() which I think is assumed to throw if it's bad and so there's scope for some permutation of a proxy setting in an env var that's not picked up here but is picked up by request. But, you'd get an informational message here and you're probably not doing something sensible if you're getting that anyway .. I think.

[imatlopez]

but that's probably going to be too painful for enterprise users that have this baked in already

This is actually why I am here, the only way I make proxies work is to not specify them outside env var.

When you provide a proxy option to request, it won't even invoke the getProxyFromURI.js logic as far as I can tell, so it's clobbered by what's done here.

Exactly, which is why no_proxy is never handled correctly. My initial concern was with env vars leaking elsewhere if they were changed within the program, so I ripped out all the request code related to it (including the url.parse part--which is deprecated sadly). A benefit is that proxy behavior is now set in stone if request goes away.

[imatlopez]

@rvagg anything pending here?

[rvagg]

landed in 3bcba2a, labelled semver-minor

[imatlopez]

Checklist

• npm install && npm test passes
• documentation is changed or added
• commit message follows commit guidelines

Description of change

Per #1176 (comment), closes#1751

When using the no_proxy environmental variable I noticed that some installations would fail when NODE_CONFIG_PROXY was set (or its equivalent). From inspecting this code plus how its dependency, https://github.com/request/request, I noticed it already has full proxy support. Changing env itself might lead to side effects.

• Prevents --proxy to blocking no_proxy from https://github.com/request/request/blob/212570b6971a732b8dd9f3c73354bcdda158a737/request.js#L276-L278
• Adds no_proxy support through CLI --noproxy or equivalent NPM alternatives.
• Possibly breaking: Passing proxy effectively blocks https_proxy currently, this also unblocks that variable (which would override ourproxy variable). However I am mirroring the http_proxy and https_proxy definitions and then letting the dependency to pick either. (Implemention downstream in: https://github.com/request/request/blob/212570b6971a732b8dd9f3c73354bcdda158a737/lib/getProxyFromURI.js#L40)

[rvagg]

It's unfortunate we have to include so much additional logic just to get proper no_proxy support, we're basically digging a bigger hole because of the support for non-standard methods of specifying proxies. One option might be to just rip it out and tell the user to just use standard *_PROXY env vars and be done with it but that's probably going to be too painful for enterprise users that have this baked in already.

Regarding the breaking change - would it really be breaking? In this and the current version, gyp.opts.proxy clobbers whatever you have set. If you have HTTPS_PROXY set only then in the current version it'll flow through and request will handle it properly, in this version you're handling it in the same way as request would have handled it previously. When you provide a proxy option to request, it won't even invoke the getProxyFromURI.js logic as far as I can tell, so it's clobbered by what's done here.

I could imagine some weirdness if you had a proxy setting that didn't pass our basic regex: if (/^https?:\/\//i.test(proxyUrl)) {. request does a url.parse() which I think is assumed to throw if it's bad and so there's scope for some permutation of a proxy setting in an env var that's not picked up here but is picked up by request. But, you'd get an informational message here and you're probably not doing something sensible if you're getting that anyway .. I think.

[imatlopez]

but that's probably going to be too painful for enterprise users that have this baked in already

This is actually why I am here, the only way I make proxies work is to not specify them outside env var.

When you provide a proxy option to request, it won't even invoke the getProxyFromURI.js logic as far as I can tell, so it's clobbered by what's done here.

Exactly, which is why no_proxy is never handled correctly. My initial concern was with env vars leaking elsewhere if they were changed within the program, so I ripped out all the request code related to it (including the url.parse part--which is deprecated sadly). A benefit is that proxy behavior is now set in stone if request goes away.

[imatlopez]

@rvagg anything pending here?

[rvagg]

landed in 3bcba2a, labelled semver-minor

[imatlopez]

Checklist

• npm install && npm test passes
• documentation is changed or added
• commit message follows commit guidelines

Description of change

Per #1176 (comment), closes#1751

When using the no_proxy environmental variable I noticed that some installations would fail when NODE_CONFIG_PROXY was set (or its equivalent). From inspecting this code plus how its dependency, https://github.com/request/request, I noticed it already has full proxy support. Changing env itself might lead to side effects.

• Prevents --proxy to blocking no_proxy from https://github.com/request/request/blob/212570b6971a732b8dd9f3c73354bcdda158a737/request.js#L276-L278
• Adds no_proxy support through CLI --noproxy or equivalent NPM alternatives.
• Possibly breaking: Passing proxy effectively blocks https_proxy currently, this also unblocks that variable (which would override ourproxy variable). However I am mirroring the http_proxy and https_proxy definitions and then letting the dependency to pick either. (Implemention downstream in: https://github.com/request/request/blob/212570b6971a732b8dd9f3c73354bcdda158a737/lib/getProxyFromURI.js#L40)

[rvagg]

It's unfortunate we have to include so much additional logic just to get proper no_proxy support, we're basically digging a bigger hole because of the support for non-standard methods of specifying proxies. One option might be to just rip it out and tell the user to just use standard *_PROXY env vars and be done with it but that's probably going to be too painful for enterprise users that have this baked in already.

Regarding the breaking change - would it really be breaking? In this and the current version, gyp.opts.proxy clobbers whatever you have set. If you have HTTPS_PROXY set only then in the current version it'll flow through and request will handle it properly, in this version you're handling it in the same way as request would have handled it previously. When you provide a proxy option to request, it won't even invoke the getProxyFromURI.js logic as far as I can tell, so it's clobbered by what's done here.

I could imagine some weirdness if you had a proxy setting that didn't pass our basic regex: if (/^https?:\/\//i.test(proxyUrl)) {. request does a url.parse() which I think is assumed to throw if it's bad and so there's scope for some permutation of a proxy setting in an env var that's not picked up here but is picked up by request. But, you'd get an informational message here and you're probably not doing something sensible if you're getting that anyway .. I think.

[imatlopez]

but that's probably going to be too painful for enterprise users that have this baked in already

This is actually why I am here, the only way I make proxies work is to not specify them outside env var.

When you provide a proxy option to request, it won't even invoke the getProxyFromURI.js logic as far as I can tell, so it's clobbered by what's done here.

Exactly, which is why no_proxy is never handled correctly. My initial concern was with env vars leaking elsewhere if they were changed within the program, so I ripped out all the request code related to it (including the url.parse part--which is deprecated sadly). A benefit is that proxy behavior is now set in stone if request goes away.

[imatlopez]

@rvagg anything pending here?

[rvagg]

landed in 3bcba2a, labelled semver-minor

[imatlopez]

Checklist

• npm install && npm test passes
• documentation is changed or added
• commit message follows commit guidelines

Description of change

Per #1176 (comment), closes#1751

When using the no_proxy environmental variable I noticed that some installations would fail when NODE_CONFIG_PROXY was set (or its equivalent). From inspecting this code plus how its dependency, https://github.com/request/request, I noticed it already has full proxy support. Changing env itself might lead to side effects.

• Prevents --proxy to blocking no_proxy from https://github.com/request/request/blob/212570b6971a732b8dd9f3c73354bcdda158a737/request.js#L276-L278
• Adds no_proxy support through CLI --noproxy or equivalent NPM alternatives.
• Possibly breaking: Passing proxy effectively blocks https_proxy currently, this also unblocks that variable (which would override ourproxy variable). However I am mirroring the http_proxy and https_proxy definitions and then letting the dependency to pick either. (Implemention downstream in: https://github.com/request/request/blob/212570b6971a732b8dd9f3c73354bcdda158a737/lib/getProxyFromURI.js#L40)

[rvagg]

It's unfortunate we have to include so much additional logic just to get proper no_proxy support, we're basically digging a bigger hole because of the support for non-standard methods of specifying proxies. One option might be to just rip it out and tell the user to just use standard *_PROXY env vars and be done with it but that's probably going to be too painful for enterprise users that have this baked in already.

Regarding the breaking change - would it really be breaking? In this and the current version, gyp.opts.proxy clobbers whatever you have set. If you have HTTPS_PROXY set only then in the current version it'll flow through and request will handle it properly, in this version you're handling it in the same way as request would have handled it previously. When you provide a proxy option to request, it won't even invoke the getProxyFromURI.js logic as far as I can tell, so it's clobbered by what's done here.

I could imagine some weirdness if you had a proxy setting that didn't pass our basic regex: if (/^https?:\/\//i.test(proxyUrl)) {. request does a url.parse() which I think is assumed to throw if it's bad and so there's scope for some permutation of a proxy setting in an env var that's not picked up here but is picked up by request. But, you'd get an informational message here and you're probably not doing something sensible if you're getting that anyway .. I think.

[imatlopez]

but that's probably going to be too painful for enterprise users that have this baked in already

This is actually why I am here, the only way I make proxies work is to not specify them outside env var.

When you provide a proxy option to request, it won't even invoke the getProxyFromURI.js logic as far as I can tell, so it's clobbered by what's done here.

Exactly, which is why no_proxy is never handled correctly. My initial concern was with env vars leaking elsewhere if they were changed within the program, so I ripped out all the request code related to it (including the url.parse part--which is deprecated sadly). A benefit is that proxy behavior is now set in stone if request goes away.

[imatlopez]

@rvagg anything pending here?

[rvagg]

landed in 3bcba2a, labelled semver-minor

[imatlopez]

Checklist

• npm install && npm test passes
• documentation is changed or added
• commit message follows commit guidelines

Description of change

Per #1176 (comment), closes#1751

When using the no_proxy environmental variable I noticed that some installations would fail when NODE_CONFIG_PROXY was set (or its equivalent). From inspecting this code plus how its dependency, https://github.com/request/request, I noticed it already has full proxy support. Changing env itself might lead to side effects.

• Prevents --proxy to blocking no_proxy from https://github.com/request/request/blob/212570b6971a732b8dd9f3c73354bcdda158a737/request.js#L276-L278
• Adds no_proxy support through CLI --noproxy or equivalent NPM alternatives.
• Possibly breaking: Passing proxy effectively blocks https_proxy currently, this also unblocks that variable (which would override ourproxy variable). However I am mirroring the http_proxy and https_proxy definitions and then letting the dependency to pick either. (Implemention downstream in: https://github.com/request/request/blob/212570b6971a732b8dd9f3c73354bcdda158a737/lib/getProxyFromURI.js#L40)

[rvagg]

It's unfortunate we have to include so much additional logic just to get proper no_proxy support, we're basically digging a bigger hole because of the support for non-standard methods of specifying proxies. One option might be to just rip it out and tell the user to just use standard *_PROXY env vars and be done with it but that's probably going to be too painful for enterprise users that have this baked in already.

Regarding the breaking change - would it really be breaking? In this and the current version, gyp.opts.proxy clobbers whatever you have set. If you have HTTPS_PROXY set only then in the current version it'll flow through and request will handle it properly, in this version you're handling it in the same way as request would have handled it previously. When you provide a proxy option to request, it won't even invoke the getProxyFromURI.js logic as far as I can tell, so it's clobbered by what's done here.

I could imagine some weirdness if you had a proxy setting that didn't pass our basic regex: if (/^https?:\/\//i.test(proxyUrl)) {. request does a url.parse() which I think is assumed to throw if it's bad and so there's scope for some permutation of a proxy setting in an env var that's not picked up here but is picked up by request. But, you'd get an informational message here and you're probably not doing something sensible if you're getting that anyway .. I think.

[imatlopez]

but that's probably going to be too painful for enterprise users that have this baked in already

This is actually why I am here, the only way I make proxies work is to not specify them outside env var.

When you provide a proxy option to request, it won't even invoke the getProxyFromURI.js logic as far as I can tell, so it's clobbered by what's done here.

Exactly, which is why no_proxy is never handled correctly. My initial concern was with env vars leaking elsewhere if they were changed within the program, so I ripped out all the request code related to it (including the url.parse part--which is deprecated sadly). A benefit is that proxy behavior is now set in stone if request goes away.

[imatlopez]

@rvagg anything pending here?

[rvagg]

landed in 3bcba2a, labelled semver-minor