-
Notifications
You must be signed in to change notification settings - Fork 25
feat: added support for proxy agent in https call #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
feat: added support for proxy agent in https call #30
Conversation
sinha-sahil
commented
Mar 9, 2023
- added parameter for passing https/http agent in https call made for getAccessToken
- updated types
bd511ce to
56c1b3e
Compare
index.js
Outdated
| this.sharedSecret = options.sharedSecret; | ||
| this.redirectUri = options.redirectUri; | ||
| this.apiKey = options.apiKey; | ||
| this.proxyAgent = 'proxyAgent' in options ? options.proxyAgent : null; |
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.
It can actually be any agent so I would rename the option to just agent.
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.
Updated name to agent
|
The new option should also be added to |
56c1b3e to
8aac89c
Compare
index.js
Outdated
| * @param {String} options.apiKey The API Key for the app | ||
| * @param {String} [options.accessMode] The API access mode | ||
| * @param {Number} [options.timeout] The request timeout | ||
| * @param {https.Agent|http.Agent} [options.agent] The agent for all http/https calls |
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.
The endpoint is HTTPS only so I think this should be only https.Agent, no?
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.
Actually there can be different kind of tunnelling using agents.
All possible combinations are:
- Http-Over-Http
- Http-Over-Https
- Https-Over-Http
- Https-Over-Https
In getAccessToken 's use case point 3 & point 4 are valid & useful.
Hence added the support for http.Agent & https.Agent
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.
The proxy can be HTTP or HTTPS but the endpoint is always HTTPS so I think it's 3 & 4 in your list. For example, see https://github.com/delvedor/hpagent/tree/v1.2.0#usage and https://github.com/delvedor/hpagent/blob/v1.2.0/index.js#L64.
index.js
Outdated
| this.sharedSecret = options.sharedSecret; | ||
| this.redirectUri = options.redirectUri; | ||
| this.apiKey = options.apiKey; | ||
| this.agent = 'agent' in options ? options.agent : null; |
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.
| this.agent = 'agent' in options ? options.agent : null; | |
| this.agent = options.agent; |
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.
Updated the PR.
README.md
Outdated
| - `agent` - Optional - An http/https agent which will be passed on to https | ||
| request made for obtaining auth token. This is useful when trying to obtain | ||
| token from a server which has constraints on internet access. |
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.
| - `agent` - Optional - An http/https agent which will be passed on to https | |
| request made for obtaining auth token. This is useful when trying to obtain | |
| token from a server which has constraints on internet access. | |
| - `agent` - Optional - An HTTPS agent which will be passed to the HTTPS | |
| request made for obtaining the auth token. This is useful when trying to | |
| obtain a token from a server that has restrictions on internet access. |
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.
Updated the README.md
0203b5b to
1496ad6
Compare
- added parameter for passing https/http agent in https call made for getAccessToken - updated types & readme for agent parameter
1496ad6 to
aa1d6b6
Compare
|
Thank you. |
|
@lpinca Please publish this change to the NPM as well. I need it one of my applications 😅 (currently pointing it to git repo link, will move it back to NPM package version once this is published). |