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(websocket): use proxy for websocket connections #10149
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Ignored Deployments
|
Documentation for this option needs to be added to: |
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 key problem here is that your new agent option is not serializable over worker_thread
s. This completely breaks the worker sharding strategy.
@didinele I see the problem. I don't really know how threads works in Javascript, if there's no way to share the agent option between threads, should I construct the |
I'm not confident we should go forward with this. Thoughts? @vladfrangu |
I'm... 🤷. I wasn't a fan from the get-go because this was a feature only node users could use (good luck deno/bun), and if there's even more issues with this... I'd say instead people should build their own ws server that gets proxied and maybe we implement something in /ws to support a custom gw url, or who knows. Overall, I'll leave it up to you @didinele, since you mentioned you wanted this before iirc. |
Yeah. Frankly, if the agent API was universal and we found a nice interface for users to pass an instance even in worker mode, I'd be a lot more in favor of this, but as it is it feels whacky at best. |
const { proxy, httpsProxyAgentOptions } = this.strategy.options.proxyAgentOptions ?? { | ||
proxy: undefined, | ||
httpsProxyAgentOptions: undefined, | ||
}; | ||
const agent = proxy ? new HttpsProxyAgent(proxy, httpsProxyAgentOptions) : undefined; |
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.
const { proxy, httpsProxyAgentOptions } = this.strategy.options.proxyAgentOptions ?? { | |
proxy: undefined, | |
httpsProxyAgentOptions: undefined, | |
}; | |
const agent = proxy ? new HttpsProxyAgent(proxy, httpsProxyAgentOptions) : undefined; | |
const { proxy, httpsProxyAgentOptions } = this.strategy.options.proxyAgentOptions ?? { | |
proxy: undefined, | |
httpsProxyAgentOptions: undefined, | |
}; | |
const agent = proxy && new HttpsProxyAgent(proxy, httpsProxyAgentOptions); |
Hi. Especially considering the direction we're taking of ensuring the package works in non-node envs, on top of everything else already discussed in this thread, we won't be going forward with this. |
Please describe the changes this PR makes and why it should be merged:
The PR adds a way to use discord.js through a proxy.
Status and versioning classification:
I tested it and it works for my configuration.
I am not sure how to update typings but I'll be happy to do so :)
This PR adds one parameter but it is optional.
I will also be happy to add some documentation if needed.