-
Notifications
You must be signed in to change notification settings - Fork 416
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
Allow configuring RequestHandler to use HTTP instead of HTTPS #1193
base: dev
Are you sure you want to change the base?
Conversation
Should we not just take the protocol from the rest URL provided? |
The domain doesn't have a protocol prefixed. Using it would probably be neater, but it would break backwards compat. |
@@ -109,11 +109,12 @@ class Client extends EventEmitter { | |||
* @arg {Function} [options.reconnectDelay] A function which returns how long the bot should wait until reconnecting to Discord. | |||
* @arg {Number} [options.requestTimeout=15000] A number of milliseconds before requests are considered timed out. This option will stop affecting REST in a future release; that behavior is [DEPRECATED] and replaced by `options.rest.requestTimeout` | |||
* @arg {Object} [options.rest] Options for the REST request handler | |||
* @arg {Object} [options.rest.agent] A HTTPS Agent used to proxy requests | |||
* @arg {Object} [options.rest.agent] A HTTPS Agent (if https: true, default) or an HTTP agent (if https: false) used to proxy requests |
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.
* @arg {Object} [options.rest.agent] A HTTPS Agent (if https: true, default) or an HTTP agent (if https: false) used to proxy requests | |
* @arg {Object} [options.rest.agent] An HTTP Agent used to proxy requests |
this would probably be better said as HTTP Agent
, noting that difference doesn't seem that important (and if you want to argue for it, HTTP/S
could be used instead of those distinctions)
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.
* @arg {Object} [options.rest.agent] A HTTPS Agent (if https: true, default) or an HTTP agent (if https: false) used to proxy requests | |
* @arg {Object} [options.rest.agent] A HTTP/S Agent used to proxy requests |
Removals: - abalabahaha/eris#1283 - abalabahaha/eris#1285 - abalabahaha/eris#1369 Additions: - abalabahaha/eris#494 - abalabahaha/eris#1193 - abalabahaha/eris#1338 - abalabahaha/eris#1386
My use case was to tunnel all Eris traffic through a central rate limiter (Twilight's http-proxy), which uses HTTP instead of HTTPS. Given that we can already configure the domain and base URL, being able to configure whether to use HTTPS or HTTP seemed logical.
I've named it
https
and enabled it by default, to ensure concious opt-out by doinghttps: false
. A name likeuseHttps
might be more appropriate.Could also consider storing the request method (
https
orhttp
module) as an instance variable on RequestHandler instead of doing the ternary on each request, but I figured this'd be fine.JSDoc and TypeScript typings should be good.