Skip to content
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

Closed
wants to merge 3 commits into from

Conversation

NewMeeh
Copy link

@NewMeeh NewMeeh commented Feb 23, 2024

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.

Copy link

vercel bot commented Feb 23, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments
Name Status Preview Comments Updated (UTC)
discord-js ⬜️ Ignored (Inspect) Visit Preview Feb 28, 2024 4:23pm
discord-js-guide ⬜️ Ignored (Inspect) Visit Preview Feb 28, 2024 4:23pm

@almeidx
Copy link
Member

almeidx commented Feb 23, 2024

Copy link
Member

@didinele didinele left a 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_threads. This completely breaks the worker sharding strategy.

@NewMeeh
Copy link
Author

NewMeeh commented Feb 24, 2024

@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 agent inside the WebSocketShard but this means adding a dependency 🤷‍♂️

@didinele
Copy link
Member

didinele commented Mar 4, 2024

I'm not confident we should go forward with this. Thoughts? @vladfrangu

@vladfrangu
Copy link
Member

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.

@didinele
Copy link
Member

didinele commented Mar 4, 2024

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.

Comment on lines +189 to +193
const { proxy, httpsProxyAgentOptions } = this.strategy.options.proxyAgentOptions ?? {
proxy: undefined,
httpsProxyAgentOptions: undefined,
};
const agent = proxy ? new HttpsProxyAgent(proxy, httpsProxyAgentOptions) : undefined;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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);

@didinele
Copy link
Member

didinele commented May 4, 2024

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.

@didinele didinele closed this May 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Denied
Development

Successfully merging this pull request may close these issues.

None yet

5 participants