Closed aslushnikov closed 4 years ago
Instead of adding new getEnv(...)
calls everywhere, could you modify the getEnv
to fall back to npm_config_*
if needed?
And please add some unit tests as well.
Instead of adding new getEnv(...) calls everywhere, could you modify the getEnv to fall back to npmconfig* if needed?
Not all proxy-related env variables are mirrored in NPM config, e.g. there's no such thing as npm_config_all_proxy
. I can whitelist allowed values in getEnv
, but having these ad-hoc feels more straight-forward.
And please add some unit tests as well.
My bad! Done.
Not all proxy-related env variables are mirrored in NPM config, e.g. there's no such thing as
npm_config_all_proxy
I see. There is no 1:1 relation between the variable names used here and the one used by npm.
Could you use the npm_config_
variants as a fallback? The HTTPS_PROXY
(etc) variable is the more common form, so they should be preferred over the npm_config
variants.
And please add some unit tests as well.
My bad! Done.
Thaanks!
Could you use the npmconfig variants as a fallback? The HTTPS_PROXY (etc) variable is the more common form, so they should be preferred over the npm_config variants.
I was driven by an opposite logic: preferring npm_config_*
over default variants since they're more specific.
I consider the following use case: I have HTTPS_PROXY
somewhere in my .bashrc
for general-purpose web surfing, and if NPM doesn't work for some reason with these settings then I go figure all the npm-config business. WDYT?
Could you use the npmconfig variants as a fallback? The HTTPS_PROXY (etc) variable is the more common form, so they should be preferred over the npm_config variants.
I was driven by an opposite logic: preferring
npm_config_*
over default variants since they're more specific.I consider the following use case: I have
HTTPS_PROXY
somewhere in my.bashrc
for general-purpose web surfing, and if NPM doesn't work for some reason with these settings then I go figure all the npm-config business. WDYT?
This sounds reasonable. I also looked up the origin of the npm config documentation, and found that it was introduced here: https://github.com/npm/npm/pull/6525
In that implementation, the default value of npm_config_*proxy
is unset, to allow the underlying (request) library to observe the standard *_PROXY
environment variable. If npm_config_*proxy
is set, the *_proxy
environment variable is shadowed.
In your current patch, you're using the npm-specific environment variables before falling back to the normal ones, i.e.:
npm_config_[proto]_proxy
npm_config_proxy
[proto]_proxy
all_proxy
Could you swap 2 and 3, so that the protocol-specific environment variables take precedence over the ultimate fallback?
Could you swap 2 and 3, so that the protocol-specific environment variables take precedence over the ultimate fallback?
Indeed, good catch! Done.
is there anything else I can do?
Thanks for your patch. Is there anything else before I publish an update?
Thanks for your patch. Is there anything else before I publish an update?
No, I don't think so. Once you release a new version, we'll bump it right away in Playwright.
Thank you!
Fixes #8