-
Notifications
You must be signed in to change notification settings - Fork 36
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
Add support for proxyRequestOptions #72
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Can you update the requestOption
object to allow adding other custom options, such as the one described in #69?
Edit: I've missed a line in the review, the changes are good.
Can you update the types and the README as well?
Thanks!
@delvedor Will do! Expect an update within a day or two. |
@delvedor See a112371. Please note that I defined a single type for the EDIT: Sorry for the force-pushes, I realized the new interface was misnamed. I think I found a relevant name in the end, but I'm open to renaming it if needed. |
6410a30
to
67c3218
Compare
67c3218
to
a112371
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Released in |
This PR adds a new property to the constructor
options
,proxyRequestOptions
, which allows callers to provide options for theCONNECT request made against the proxy. There are various reasons to do this:
ca
may need to be passed to the proxy connection but not the upstream connection (or vice versa) and theca
value is not known at launch or can change over time, preventing the use ofNODE_EXTRA_CA_CERTS
as a viable alternative because that environment variable is only used by Node during startupProxy-Authentication
should be sent only to the proxy andAuthorization
should only be sent to the upstream server.Implements #69
Note: this PR introduces a merge conflict with #71. Whichever lands first will require a manual merge before the other can land.