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

Adding interpretation of no_proxy #565

Closed
wants to merge 8 commits into from
Closed

Adding interpretation of no_proxy #565

wants to merge 8 commits into from

Conversation

inthemill
Copy link

As suggested by #434 the interpretation of no_proxy is implemented here.

I would be happy if axios supports no_proxy, as i would be able to use it in my company.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 93.909% when pulling ed47ef8 on inthemill:master into cfe33d4 on mzabriskie:master.

@geekflyer
Copy link

can we please merge this soon? Spend hours to figure out why our integration tests were not running in CI environment and it turns out this is due to axios' lack of interpreting no_proxy.

@gustarus
Copy link

gustarus commented Apr 7, 2017

Guys, hello! Is there any movements? We need this feature 🔥.

@lgordey
Copy link

lgordey commented Apr 12, 2017

@mzabriskie @nickuraltsev @rubennorte Hi guys!

We are waiting for this improvement 4 months.
Can we merge this or find out the comments?

Otherwise we will be forced to refuse to use your package, although we really like it 😢

@geekflyer
Copy link

hey guys, I just couldn't wait anymore, so I republished this branch on npm under axios-no-proxy https://www.npmjs.com/package/axios-no-proxy

@jphilipstevens
Copy link
Collaborator

We are running into issues because of this. Please bring this in ASAP

@@ -99,6 +116,9 @@ module.exports = function httpAdapter(config) {
password: proxyUrlAuth[1]
};
}
if (isANoProxyHost(proxy.host)) {

Choose a reason for hiding this comment

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

Should that be parsed.hostname?

Copy link
Author

Choose a reason for hiding this comment

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

of course. I'll fix that, and check, why the tests don't fail.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 93.734% when pulling 24ecb51 on inthemill:master into cfe33d4 on mzabriskie:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 93.734% when pulling bdcad15 on inthemill:master into f1fb3de on mzabriskie:master.

@nareshbhatia
Copy link

Any reason why this PR has not been merged since last 8 months? I have spent over 2 weeks tracking down a proxy issue, which could have been solved in 2 minutes if this had been merged. @mzabriskie and other committers, could you please help by accepting this PR?

Thanks in advance.

@nareshbhatia
Copy link

@mzabriskie, any chance you will be able to accept this PR? My project is blocked because of this. Please just drop a quick response so that we can plan accordingly.

Thanks in advance.

@rubennorte
Copy link
Member

rubennorte commented Aug 12, 2017

Sorry for the unreasonable delay.

This NO_PROXY implementation is a good starting point but doesn't implement some usual use cases (like wildcards). I've discovered the proxy-from-env package that handles that (getting the proxy URL for a specific URL using the configuration in the environment), so you may want to use it instead.

WDYT?

@@ -224,3 +244,6 @@ module.exports = function httpAdapter(config) {
}
});
};


module.exports._isANoProxyHost = isANoProxyHost;
Copy link
Member

Choose a reason for hiding this comment

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

Please don't export functions just for testing. This can be tested throught the behaviour of the request.

@nareshbhatia
Copy link

@rubennorte, proxy-from-env looks good but if we use that, it will interfere with Axios. Since Axios already supports HTTP_PROXY and HTTPS_PROXYit will take the URL provided by proxy-from-env and apply the proxy to it anyway. I would prefer to simply add NO_PROXY support to Axios and not have to worry about one more dependency. Of course, the NO_PROXY support would have to implement wildcards also.

@rubennorte
Copy link
Member

I was suggesting that you could use it because we're most likely to do it anywhere in a following PR, so it could save you some work in the implementation of the wildcards. This only works in Node.js so adding a new dependency here wouldn't be an issue.

@nareshbhatia
Copy link

Ah, ok @rubennorte. So I assume that your message to use proxy-from-env was addressed to @inthemill - the submitter of the PR.

@rubennorte
Copy link
Member

@nareshbhatia yep, sorry.

@rubennorte
Copy link
Member

I just merged #691, which is related to this one.

@inthemill
Copy link
Author

@rubennorte When I use proxy-from-env it would make sense to change the whole part where the proxy is read from env-variables. This will have a much deeper impact on existing code. Seeing that it is only covered by integration tests, i'm not very confident doing this. I could try, but i would need some help testing all edge cases. Do you want me to do that?

@rubennorte
Copy link
Member

@inthemill that'd be great. I can give you some support if you need help with anything.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 93.812% when pulling 44967bf on inthemill:master into 07a7b7c on mzabriskie:master.

@inthemill
Copy link
Author

@rubennorte what about these changes?

@williangd
Copy link

Any update about no_proxy?

@inthemill
Copy link
Author

Yes please. @rubennorte let's merge this PR, it's old enough.

@ian-harshman-ck
Copy link

@nickuraltsev @rubennorte Please, let's get this merged! We love axios and are looking to support axios connections for a Thrift client library, but we need no_proxy support to use it anywhere beyond dev environments.

@gigachel
Copy link

Any news about no_proxy?

@JulienBourgain
Copy link

It's not a good idea to wait 1year before merging an important PR without good reasons...

@jphilipstevens
Copy link
Collaborator

@nickuraltsev @rubennorte
Can you provide any update?

@dejayc
Copy link

dejayc commented Feb 22, 2018

This is so typical of the Node ecosystem, lol. Becoming dependent upon abandonware.

@daggerjames
Copy link

seems like we should wait for next year lol

@alexbonhomme
Copy link

Hey guys !
You should close this PR in favor of #1693 who's merged into master and fix the problem 🎉
This this confusing to keep this open.

Please give a try to https://github.com/axios/axios/tree/v0.19.0-beta.1

@inthemill inthemill closed this Aug 30, 2018
@axios axios locked and limited conversation to collaborators May 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet