-
Notifications
You must be signed in to change notification settings - Fork 9
use proxy defined in HTTP_PROXY environment variable #30
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
base: main
Are you sure you want to change the base?
Conversation
Thanks for PR! I agree with the direction of removing However, it looks like the current PR causes an error on older versions of Node.js. |
Could you try setting the It might work, since // `proxy` defines the hostname, port, and protocol of the proxy server.
// You can also define your proxy using the conventional `http_proxy` and
// `https_proxy` environment variables. If you are using environment variables
// for your proxy configuration, you can also define a `no_proxy` environment
// variable as a comma-separated list of domains that should not be proxied.
// Use `false` to disable proxies, ignoring environment variables.
// `auth` indicates that HTTP Basic auth should be used to connect to the proxy, and
// supplies credentials.
// This will set an `Proxy-Authorization` header, overwriting any existing
// `Proxy-Authorization` custom headers you have set using `headers`.
// If the proxy server uses HTTPS, then you must set the protocol to `https`.
proxy: {
protocol: 'https',
host: '127.0.0.1',
// hostname: '127.0.0.1' // Takes precedence over 'host' if both are defined
port: 9000,
auth: {
username: 'mikeymike',
password: 'rapunz3l'
}
}, |
Hello, |
Thanks to your explanation, I now understand that http_proxy may not work correctly in some cases. I had forgotten this myself, but the reason I chose axios over fetch was to maintain compatibility with node.js v14. |
for information, the fetch included in recent nodejs is also available as a library https://github.com/nodejs/undici (undici v5.x is still maintained and is compatible with nodejs >= 14). Regarding
(In addition, undici request is 2 to 3 times faster than fetch or axios) |
I've noticed that stream/promises isn't compatible with NodeJs 14 either. But if you prefer, I've also made a branch that supports Nodejs 14 https://github.com/floriannari/ip-location-api/tree/adapt_to_nodejs_14 (using stream instead of stream/promises) (in any case, I've reverted floriannari@41c9839 because EnvHttpProxyAgent requires Undici 6.x, which requires nodejs 18. |
Have you tried using I also tried creating a |
Hello, Concerning the fetch-version branch, it's a good solution. But as it is, it doesn't work because the res.body.pipe(ws) function exists in undici request, but not in (node-)fetch. (Note that I've run your unit tests ( https://github.com/sapics/ip-location-api/blob/main/.github/workflows/test.yml#L9 ) on my https://github.com/floriannari/ip-location-api/commits/adapt_to_nodejs_14/ and https://github.com/floriannari/ip-location-api/commits/drop_nodejs14_support/ branches, and that I've also tested these 2 branches on our servers. )
I understand your apprehension about modifying a central function of your library, and it's totally legitimate. |
This PR deletes axios ( prolonging PR #29 ) and uses https://github.com/nodejs/undici/blob/main/docs/docs/api/EnvHttpProxyAgent.md to use a proxy according to what is defined in the
HTTP_PROXY
,HTTPS_PROXY
, andNO_PROXY
environment variables.PS: I haven't updated https://github.com/sapics/ip-location-api/tree/main/cjs
Possible improvement to avoid a potential breaking change: add a setting like ILA_ENV_HTTP_PROXY to specify whether to use the EnvHttpProxyAgent dispatcher, or the default one.