Skip to content
This repository has been archived by the owner on Jul 24, 2024. It is now read-only.

Respect NO_PROXY when downloading the binary #1725

Merged
merged 2 commits into from
Nov 2, 2016
Merged

Conversation

xzyfer
Copy link
Contributor

@xzyfer xzyfer commented Sep 22, 2016

Turns our handling proxies is complicated. The sanest thing to do
is try matching npm's behaviour, and liable to change to
our benefit. The TLDR; of which is to let request do whatever
it wants unless npm has been explicitly configured otherwise.

Fixes #1724

/cc @nschonni @mmrath

@xzyfer
Copy link
Contributor Author

xzyfer commented Sep 22, 2016

@mmrath can you please try applying this diff locally and running npm rebuild node-sass to see if it resolves the issue?

@nschonni
Copy link
Contributor

Maybe i'm reading this wrong https://www.npmjs.com/package/request#controlling-proxy-behaviour-using-environment-variables but I think we can just delete all our proxy code and just let request do it's own thing.

Copy link

@mmrath mmrath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! I will test this today. Any chance you can publish a experimental build? If not would you know how to use a local version with this fix?

@mmrath
Copy link

mmrath commented Sep 23, 2016

@xzyfer, @nschonni I tested this. This PR works fine for me. Any chance you can publish this?

@xzyfer
Copy link
Contributor Author

xzyfer commented Sep 23, 2016

@nschonni yes and no

The intent is to match the npm proxy resolution as closely
as possible. As long as a proxy has not been explicitly
configure for npm via npm config set or .npmrc, npm
will simply fallback to letting request determine the
proxy settings.

If a proxy has been explicitly configured for npm
we must respect it above all else.

If the user has not configured a proxy via npm config then must
use that proxy. Request is no aware of the ENV proxy variables
set via npm config.

The mikeal/request package already handles proxy support, including
environment variables http_proxy, https_proxy, no_proxy, and
variants. npm should let request take care of proxying based on
environment variables and only set proxy values if .npmrc has a proxy
specified. By passing a proxy to request, request will always use that
proxy setting and will not honor no_proxy settings. This will enable
the use of a private npm repository that is within a corporate firewall.

npm/npm@40afd6aaf34

@xzyfer xzyfer added this to the 3.11.0 milestone Sep 25, 2016
@mmrath
Copy link

mmrath commented Sep 25, 2016

@xzyfer, @nschonni Any chance this can be merged?

@xzyfer
Copy link
Contributor Author

xzyfer commented Sep 26, 2016

Blocking myself on this. There should be a test around the npm proxy behaviour as outlined

npm should let request take care of proxying based on
environment variables and only set proxy values if .npmrc has a proxy
specified.

@xzyfer
Copy link
Contributor Author

xzyfer commented Sep 26, 2016

I'm comfortable in the test coverage for this now. Waiting on the outcome of #1733.

@mmrath
Copy link

mmrath commented Oct 3, 2016

@xzyfer Any chance this can be released soon?

Turns our handling proxies is complicated. The sanest thing to do
is try matching [npm's behaviour][1], and liable [to change][1] to
our benefit. The TLDR; of which is to let `request` do whatever
it wants unless npm has been explicitly configured otherwise.

Fixes sass#1724

[1]: npm/npm@40afd6aaf34
[2]: npm/npm#7168
@xzyfer xzyfer merged commit 4116c38 into sass:master Nov 2, 2016
@xzyfer xzyfer deleted the fix/no-proxy branch November 2, 2016 13:54
xzyfer added a commit that referenced this pull request Nov 5, 2016
In #1725 we started setting `proxy: ''` to defer proxy resolution
logic to request. However `request` uses `hasOwnProperty` rather
than falsey checks to determine if an option has been set. This
caused `request` to believe were configuring a proxy so it didn't
do it's own proxy resolution.

Fixes #1785
Fixes #1787
xzyfer added a commit that referenced this pull request Nov 5, 2016
In #1725 we started setting `proxy: ''` to defer proxy resolution
logic to request. However `request` uses `hasOwnProperty` rather
than falsey checks to determine if an option has been set. This
caused `request` to believe were configuring a proxy so it didn't
do it's own proxy resolution.

Fixes #1785
Fixes #1787
xzyfer added a commit that referenced this pull request Nov 5, 2016
In #1725 we started setting `proxy: ''` to defer proxy resolution
logic to request. However `request` uses `hasOwnProperty` rather
than falsey checks to determine if an option has been set. This
caused `request` to believe were configuring a proxy so it didn't
do it's own proxy resolution.

Fixes #1785
Fixes #1787
xzyfer added a commit that referenced this pull request Nov 5, 2016
In #1725 we started setting `proxy: ''` to defer proxy resolution
logic to request. However `request` uses `hasOwnProperty` rather
than falsey checks to determine if an option has been set. This
caused `request` to believe were configuring a proxy so it didn't
do it's own proxy resolution.

Fixes #1785
Fixes #1787
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants