refactor(WebSocketManager): use /ws package internally

[Qjuh]

Please describe the changes this PR makes and why it should be merged:

Use @discordjs/ws for WebSocketShard handling, while maintaining the same interface v14 uses to accomplish this in a non-breaking way.

Resolves #8259

It worked in tests with both a sharded and a single-shard bot.

Status and versioning classification:

• Code changes have been tested against the Discord API, or there are no code changes
• I know how to update typings and have done so, or typings don't need updating

[vercel]

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		Apr 25, 2023 6:25pm
discord-js-guide	⬜️ Ignored (Inspect)	Visit Preview		Apr 25, 2023 6:25pm

[vercel]

@Qjuh is attempting to deploy a commit to the discordjs Team on Vercel.

A member of the Team first needs to authorize it.

[didinele]

Other than what Almeida already pointed out, LGTM.

One thing you can consider is to simplify this enum now that most of it is essentially unused:

discord.js/packages/discord.js/src/util/Status.js

Lines 23 to 33 in 78cc7cf

	module.exports = createEnum([
	'Ready',
	'Connecting',
	'Reconnecting',
	'Idle',
	'Nearly',
	'Disconnected',
	'WaitingForGuilds',
	'Identifying',
	'Resuming',
	]);

Perhaps make it an extension of the status enum exported by /ws? Or just forward the status from WS as-is and use bools instead for the "special" ones (like WaitingForGuilds), if possible.

[codecov]

Codecov Report

Merging #9099 (a64f46f) into main (2e09cb4) will increase coverage by 24.42%.
The diff coverage is 0.00%.

❗ Current head a64f46f differs from pull request most recent head 1e47dca. Consider uploading reports for the commit 1e47dca to get more accurate results

@@             Coverage Diff             @@
##             main    #9099       +/-   ##
===========================================
+ Coverage   59.30%   83.73%   +24.42%     
===========================================
  Files         222      100      -122     
  Lines       14629     9546     -5083     
  Branches     1257     1101      -156     
===========================================
- Hits         8676     7993      -683     
+ Misses       5913     1514     -4399     
+ Partials       40       39        -1     

Flag	Coverage Δ
guide	?
next	∅ <ø> (∅)
website	?
ws	58.21% <0.00%> (+2.65%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
packages/ws/src/ws/WebSocketShard.ts	22.19% <0.00%> (+1.84%)	⬆️

... and 175 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[JMTK]

Since the new ws package doesn't support erlpack (#9075) it should probably be removed from the readme/guide as an optional package with this PR

[Deivu]

Personally I think shard.getStatus() is redundant, as you can mark shard.status based on what the discordjs/ws emits.
It can possibly be a bottleneck in worker sharding, where you need to rely on ipc to get the data from the ws shards.

You can remove the promise and set the status on what the discordjs/ws emits.

discordjs/ws emits ready => set the proxy ws class to waiting for guilds => forward discordjs/ws dispatch events to that proxy ws class =>set status to ready fire ready once able.

discordjs/ws emits closed => set the proxy ws to reconnecting => then go upwards for ident flow or downwards for the resumed flow

discordjs/ws emits resumed => set the proxy ws to ready => emit resumed.

No need for promises on that regard

[Qjuh]

Personally I think shard.getStatus() is redundant, as you can mark shard.status based on what the discordjs/ws emits. It can possibly be a bottleneck in worker sharding, where you need to rely on ipc to get the data from the ws shards.

You can remove the promise and set the status on what the discordjs/ws emits.

discordjs/ws emits ready => set the proxy ws class to waiting for guilds => forward discordjs/ws dispatch events to that proxy ws class =>set status to ready fire ready once able.

discordjs/ws emits closed => set the proxy ws to reconnecting => then go upwards for ident flow or downwards for the resumed flow

discordjs/ws emits resumed => set the proxy ws to ready => emit resumed.

No need for promises on that regard

You are right that this could be a bottleneck when using WorkerStrategy. It won't be for SimpleStrategy because the promises immediately resolve here. Changing the strategy isn't exposed though so isn't supported at the moment anyway and we talked about this internally that we saw problems if the status of /ws and proxy ws class got out-of-sync.

But after looking further into it this looks like it should work especially since we have different enums in /ws and main lib anyway, so we need to map the status and can't just pass it through. So it can and will never be in sync.

[Deivu]

Personally I think shard.getStatus() is redundant, as you can mark shard.status based on what the discordjs/ws emits. It can possibly be a bottleneck in worker sharding, where you need to rely on ipc to get the data from the ws shards.
You can remove the promise and set the status on what the discordjs/ws emits.
discordjs/ws emits ready => set the proxy ws class to waiting for guilds => forward discordjs/ws dispatch events to that proxy ws class =>set status to ready fire ready once able.
discordjs/ws emits closed => set the proxy ws to reconnecting => then go upwards for ident flow or downwards for the resumed flow
discordjs/ws emits resumed => set the proxy ws to ready => emit resumed.
No need for promises on that regard

You are right that this could be a bottleneck when using WorkerStrategy. It won't be for SimpleStrategy because the promises immediately resolve here. Changing the strategy isn't exposed though so isn't supported at the moment anyway and we talked about this internally that we saw problems if the status of /ws and proxy ws class got out-of-sync.

But after looking further into it this looks like it should work especially since we have different enums in /ws and main lib anyway, so we need to map the status and can't just pass it through. So it can and will never be in sync.

I aleady have an implementation that is tested and working, if you want I can pr the changes on your fork

Also I think you should not hardcode what worker strategy you should use, and let the users use the original configuration options available on the ws package as well

[Qjuh]

I‘ll happily look at your implementation, open a PR and I‘ll take a look. The important part is to do it in a non-breaking way.
I‘ll try to allow setting strategy too but that would need more extensive testing to make sure it‘s not breaking.