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

Add support for native NodeJS WebSocket #10109

Open
JMTK opened this issue Jan 31, 2024 · 8 comments
Open

Add support for native NodeJS WebSocket #10109

JMTK opened this issue Jan 31, 2024 · 8 comments

Comments

@JMTK
Copy link
Contributor

JMTK commented Jan 31, 2024

Which application or package is this feature request for?

discord.js

Feature

NodeJS will be enabling the WebSocket class by default in NodeJS 22: nodejs/node#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
Copy link
Contributor

cobaltt7 commented Jan 31, 2024

Related to #10042

export function shouldUseGlobalFetchAndWebSocket() {
// Browser env and deno when ran directly
if (typeof globalThis.process === 'undefined') {
return 'fetch' in globalThis && 'WebSocket' in globalThis;
}
if ('versions' in globalThis.process) {
return 'deno' in globalThis.process.versions || 'bun' in globalThis.process.versions;
}
return false;
}

@JMTK
Copy link
Contributor Author

JMTK commented Jan 31, 2024

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

@cobaltt7
Copy link
Contributor

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
Copy link
Contributor

ws is faster

@vladfrangu vladfrangu self-assigned this Jan 31, 2024
@vladfrangu
Copy link
Member

vladfrangu commented Jan 31, 2024

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
Copy link
Member

didinele commented Jul 6, 2024

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
Copy link
Member

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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants