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[bot]]

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[bot]]

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

A member of the Team first needs to authorize it.

[codecov[bot]]

Codecov Report

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

:exclamation: 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%)	:arrow_up:

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%)	:arrow_up:

... and 175 files with indirect coverage changes

:mega: 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.

[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

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.

strategy should be non breaking. specially since the events emitted are the same

[DraftProducts]

This PR should be tagged blocked and removed from v14.8 milestone until issues are fixed and erlpack is implemented in @discordjs/ws otherwise this migration would be a downgrade.

Edit: The package has issues like #9103 with such a small users/devs base, so I can't imagine how many we could have in the main discord.js module if it would be migrated.

[legendhimself]

I presonally think its a bad idea to include these changes in v14 as the current WS implementation is the most stable it could have been with all the bugs and issues squashed. There are no more issues (that I am aware of) on the current WS implementation.

This pr tries to include the new /ws package in a non breaking way but replacing the old code with completely new code that is not even tested in production for large scale can cause several new issues. The new /ws was tested for many small bots and there have been many issues/reports for the new /ws package which is totally fine but its a bad idea to have the new ws replace the old ws in non-major release. I suggest we do this in v15.

[didinele]

This PR should be tagged blocked and removed from v14.8 milestone until issues are fixed and erlpack is implemented in @discordjs/ws otherwise this migration would be a downgrade.

Edit: The package has issues like #9103 with such a small users/devs base, so I can't imagine how many we could have in the main discord.js module if it would be migrated.

Changing internal implementation details is not major regardless of if it's a "downgrade". This can be merged just fine and be prepared for 14.8, we can iron out stability/bugs while its a @dev release.

[didinele]

I presonally think its a bad idea to include these changes in v14 as the current WS implementation is the most stable it could have been with all the bugs and issues squashed. There are no more issues (that I am aware of) on the current WS implementation.

This pr tries to include the new /ws package in a non breaking way but replacing the old code with completely new code that is not even tested in production for large scale can cause several new issues. The new /ws was tested for many small bots and there have been many issues/reports for the new /ws package which is totally fine but its a bad idea to have the new ws replace the old ws in non-major release. I suggest we do this in v15.

And you reckon rolling it out untested in v15 is any better? It doesn't matter what version its in so long as it had enough time to be toyed with as a dev release.

[legendhimself]

And you reckon rolling it out untested in v15 is any better

We have time before v15 and during this time the new /ws code can be tested more therefore we have more reported issues. The /ws code is the most important part of the bot and I think its better to release in major release, and if you want it to be included in v14, make it opt-in (with the client option maybe?)

[DraftProducts]

Changing internal implementation details is not major regardless of if it's a "downgrade". This can be merged just fine and be prepared for 14.8, we can iron out stability/bugs while its a @dev release.

I agree!  But then why should this module be released in the main module when it :

• Contains unresolved bugs issues
• Does not have a 1.0.0 version yet.
• Has been redone from 0 and doesn't take advantage of this opportunity to integrate all the features documented in the Discord api (concurrency sharding (unless it concerns the new sharder module started by @kyranet ?))

[vladfrangu]

Since we're all giving our two cents, here's my three cents:

• You are correct in the fact that /ws is less tested than the current zombie that is the djs v14 ws system. But it's also a piece that we (or at least I) want to move away from and focus less time maintaining, and as we've all probably seen, a LOT of mini PRs fixing some edge case end up with code that is less than ideal
• Version number doesn't guarantee stability. Look at -types, it is still at 0.x.x and its considered stable
• @DraftProducts, I don't think you checked the /ws implementation! max_concurrency has been implemented from the get-go in the module, since it was remade from scratch and ensures it hits everything from Discord's gateway docs. You can see it handled here: https://github.com/discordjs/discord.js/blob/main/packages/ws/src/utils/IdentifyThrottler.ts

There's also advantages by moving to /ws. Not only a more powerful and robust ws system (hammering pending), we can also release fixes without needing to release a full djs version too.

And it's not like we're merging this tonight and releasing yesterday and the world's on fire! This still needs to be reviewed, tested, knock on wood it just works, then release. Instead of holding on to the old system, why not help us test the new one? 🙏 (just by using it raw works). More testing = more confirmations of it being fine / hopefully few to none reports of issues = more stable ws experience for everyone = a W its called.

[DraftProducts]

Indeed, I didn't see that it had been implemented. I agree with the rest that has been said.

[Qjuh]

Seems like, out of all of the WebSocketShardEvents, only these are being emitted:

* `WebSocketShardEvents.Ready`

* `WebSocketShardEvents.AllReady`

* `WebSocketShardEvents.Resumed`

WebSocketShardEvents.Close can be added back rather easily. The other two events can't be emitted in a useful way because of the different approach of @discordjs/ws when it comes to handling InvalidSession replies by the API and destroying WebSocketShards. They were meant to be only used internally by the WebSocketManager before, so I'll mark them as deprecated.

[DraftProducts]

Please tell me if there is a better place to discuss the subject. I noticed that it wasn't planned to integrate erlpack to this new ws package (#9075) (which is a breaking change). Do we have a bench about performance's impact that it would have on a significative number of requests? I read that the package had problems, but couldn't find them 🤔 (I am currently using it in production)

[didinele]

(which is a breaking change).

it's not.

as per the erlpack stuff, after speaking internally a bit, we'll probably be writing our own implementation in Rust, but this PR will be merged even without it.

[Kaisarion]

is there an update here? is it functional/stable enough for me to pull and attempt?

[didinele]

yes, we're looking into merging it soon, just making sure we can delay the next minor for long enough for this to get appropriately tested while in a dev release

[github-actions[bot]]

⚡️ Lighthouse report for the changes in this PR:

Category	Score
🟠 Performance	70
🟢 Accessibility	100
🟢 Best practices	100
🟢 SEO	100
🟠 PWA	70

Lighthouse ran on https://discord-js-git-fork-qjuh-ws-package-in-v14-discordjs.vercel.app/