search

grpc / grpc-node

gRPC for Node.js
https://grpc.io
Apache License 2.0
4.49k stars 649 forks source link

Support uppercase *_PROXY env vars. #1292

Open zamnuts opened 4 years ago

zamnuts commented 4 years ago

PR #1243 introduced proxy support, but only supports lowercase environment variables. This is a non-issue on windows given https://github.com/nodejs/node/issues/9157, but not on *nix. It doesn't account for the common use case of uppercase environment variables.

View the relevant source at https://github.com/grpc/grpc-node/blob/%40grpc/grpc-js%400.7.0/packages/grpc-js/src/http_proxy.ts#L45-L56

Solution:

Support both uppercase and lowercase environment variables, i.e. cross-platform case-insensitivity for grpc_proxy, https_proxy, http_proxy, no_grpc_proxy, and no_proxy.

Alternative Solution:

Documenting that the grpc client only accepts lowercase env vars, and that their case sensitivity is governed by nodejs itself.

Additional references:

The popular get-proxy package (e.g. used by got, via the recommended caw), explicitly supports both uppercase and lowercase env vars. As an aside, it also supports equivalent npm-config keys of https-proxy, http-proxy, and proxy, despite this being a different concern altogether (npm vs application). See https://github.com/kevva/get-proxy/blob/v2.1.0/index.js#L5-L8

GNU Emacs mentions case-insensitivity in their manual: https://www.gnu.org/software/emacs/manual/html_node/url/Proxies.html

The library recognizes such variables in either upper or lower case.

sharat87 commented 1 week ago

I'd like to work on this and open a PR if there's interest in merging. Just checking if your team is not already working on this before I actually spend time. Thanks!

murgatroid99 commented 1 week ago

There is no ongoing work on this. Feel free to pick it up.