discord.js icon indicating copy to clipboard operation
discord.js copied to clipboard

Add support for native NodeJS WebSocket

Open JMTK opened this issue 1 year ago • 8 comments

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

JMTK avatar Jan 31 '24 14:01 JMTK

Related to https://github.com/discordjs/discord.js/pull/10042 https://github.com/discordjs/discord.js/blob/278396e815add4e028e43034fab586f971a043d4/packages/util/src/functions/runtime.ts#L3-L14

cobaltt7 avatar Jan 31 '24 14:01 cobaltt7

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

JMTK avatar Jan 31 '24 16:01 JMTK

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

cobaltt7 avatar Jan 31 '24 16:01 cobaltt7

ws is faster

KhafraDev avatar Jan 31 '24 21:01 KhafraDev

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)

vladfrangu avatar Jan 31 '24 21:01 vladfrangu

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?

didinele avatar Jul 06 '24 10:07 didinele

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 👁️

vladfrangu avatar Jul 06 '24 11:07 vladfrangu

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

KhafraDev avatar Jul 06 '24 13:07 KhafraDev