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

npm_config env variables are preferred over *_PROXY env variables #13

Open
flotwig opened this issue Aug 14, 2020 · 4 comments
Open

npm_config env variables are preferred over *_PROXY env variables #13

flotwig opened this issue Aug 14, 2020 · 4 comments

Comments

@flotwig
Copy link

flotwig commented Aug 14, 2020

Expected behavior: NO_PROXY would override npm_config_noproxy

Actual behavior: npm_config_noproxy, if set, overrides NO_PROXY

I see that this was discussed in #9 and @Rob--W originally recommended the behavior expected in this issue.

I believe this decision should be revisited, I think that it makes more sense for a generic program to prefer NO_PROXY, HTTP_PROXY, than it does for it to prefer NPM-specific environment variables. It's what programmers expect.

Me assuming that the precedence would be NO_PROXY > npm_config_noproxy (like @Rob--W originally assumed in #9 ) has created a regression in our product: cypress-io/cypress#8287 Presumably more people will make this assumption, which will cause more regressions.


If this behavior cannot be changed, it should at least be documented that the precedence is the opposite of what would be commonly expected. The README does not mention NPM config vars at all, and the changelog does not mention the precedence being inverted, which ended up being a breaking change for us.

@Rob--W
Copy link
Owner

Rob--W commented Aug 14, 2020

I'm sorry for breaking your product. I have just updated the release notes of 1.1.0 to mention the change: https://github.com/Rob--W/proxy-from-env/releases/tag/v1.1.0

Swapping the order now may cause even more regressions in dependencies, and the original motivation for reading the npm variants before the generic variants still makes sense. Because of that I'm inclined to resolve this bug by documenting the behavior in the README.

@flotwig
Copy link
Author

flotwig commented Aug 17, 2020

I'm sorry for breaking your product

No worries, you didn't break our product, we should have had better tests around this. ¯\_(ツ)_/¯, bugs happen

Swapping the order now may cause even more regressions in dependencies, and the original motivation for reading the npm variants before the generic variants still makes sense. Because of that I'm inclined to resolve this bug by documenting the behavior in the README.

That's a good point, I didn't think of that. Documenting this more clearly is a good path forward.

@silverwind
Copy link

silverwind commented Sep 16, 2022

I guess ideally the values of npm_config_noproxy and NO_PROXY should be merged, if both are present. Or alternatively, drop npm_config_noproxy, as it's even less of a standard than NO_PROXY.

@Rob--W
Copy link
Owner

Rob--W commented Feb 29, 2024

I'm going to make the npm config behavior opt-in in 2.0.0 because the intent for proxy-from-env has always been to rely on standard environment variables. Support for npm config vars is a convenience but non-standard and surprising, e.g. as seen at axios/axios#5619 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants