Add support for native NodeJS WebSocket

JMTK commented 7 months ago

Which application or package is this feature request for?

discord.js

Feature

NodeJS will be enabling the WebSocket class by default in NodeJS 22: https://github.com/nodejs/node/pull/51594 and is already available in NodeJS 21 as experimental. This would make the dependency on ws package optional or allow removing entirely.

I will say that in my own usage of it, there are some discrepancies/missing properties between the ws package and the native, so I'm waiting for the @types/node update so that it is very clear what the differences are.

Ideal solution or implementation

Similar to how you can choose to use ffmpeg/avconv, tweetnacl/libsodium, etc. it would be nice to be able to use the WebSocket provided by node instead of a 3rd party version.

Something like:

const ws = options.useNativeWebSocket && WebSocket ? WebSocket : require('ws');

 ...
 let wsInstance = new ws('wss://...');

or just

let ws = null;
try {
    ws = require('ws');
 }
 catch (ex) {
     if (typeof WebSocket !== 'undefined') { 
         ws = WebSocket;
     }
     else {
         throw new Error("No Websocket class found");
     }
 }

 ...
 let wsInstance = new ws('wss://...');

Alternative solutions or implementations

No response

Other context

No response

cobaltt7 commented 7 months ago

JMTK commented 7 months ago

Oh nice! I didn't know about that one. Is it worth closing this issue then?

cobaltt7 commented 7 months ago

Don't think so, it looks like the code still needs to be updated to support native WebSockets on Node, that PR focused on Deno and Bun support

KhafraDev commented 7 months ago

ws is faster

vladfrangu commented 7 months ago

This is something I'll tackle sometime in the future. As it stands, the global ws is still experimental, and not as battle tested as ws (the npm package).

Maybe after we see the Autobahn tests results for it, as well as some performance comparisons, we can add it in as the defacto implementation used for newer node versions.

As it stands tho, if you really wanna try it out, you can fake that you're in bun/deno by setting that property in globalThis.process.versions property but you're on your own from then on (with that said I'd still love to hear if/what breaks when using node's global ws implementation, so poke me on discord @vladdy in a thread in the discord server)

didinele commented 2 months ago

We now use default WebSocket in deno and bun.

I think ideally, once discord.js is on a new enough node version (and there's no clear drawbacks), we move to full global WS regardless of env.

For now, we could maybe investigate using global WS if available in node as well?

vladfrangu commented 2 months ago

For now, we could maybe investigate using global WS if available in node as well?

I'd wanna see performance numbers and Autobahn results before even considering spending time investigating this 👁️

KhafraDev commented 2 months ago

Performance - not measured yet. Autobahn - 100% https://khafradev.github.io/autobahn/clients/index.html

discordjs / discord.js