adapter-node-ws icon indicating copy to clipboard operation
adapter-node-ws copied to clipboard

Optionally use socket.io

Open joppekoers opened this issue 2 years ago • 4 comments

First of all: thank you for creating this. Now for the PR: I wanted to use Socket.io instead of ws This is by far not complete. But before I continue let's discuss this Shall I extend this module so that the user can choose between socket.io and ws. Or shall I create a new package entirely?

joppekoers avatar Apr 22 '23 17:04 joppekoers

Hi @SirMorfield!

Thanks a lot for your suggestion. I did in fact attempted to integrate Socket.io originally but I failed. Nowadays, I'm very happy with the raw websocket solution.

That said, I'm all up for a configurable solution.

It could be passed as a plugin argument:

// Possible values `websocket`, `socketio`, etc
adapter: adapter({ library: 'socketio' })

Or even as a separate plugin file:

import wsAdapter from "@carlosv2/adapter-node-ws/websocket-adapter";
import socketIoAdapter from "@carlosv2/adapter-node-ws/socketio-adapter";

I'm really curious to see what it would look like with socket.io :-)

carlosV2 avatar Apr 24 '23 21:04 carlosV2

Hi guys, thank you very much for working on this. I only have one reservation: this work is replacing WebSockets with SocketIo. If I merge this anyone using this adapter (including myself) will get their projects broken.

I love the idea of supporting both systems but I believe it should be a conscious decision to use either. For example, by selecting the correct path in the import:

// If a user wants the WebSocket version
import adapter from "@carlosv2/adapter-node-ws/websocket-adapter";

// If a user wants the SocketIo version
import adapter from "@carlosv2/adapter-node-ws/socketio-adapter";

Additionally, the @carlosv2/adapter-node-ws/adapter path should still exist for a while for retro-compatibility reasons (perhaps marking it as deprecated).

If you need help with this, let me know how can I help you. I'm finally a bit let busy than a few weeks ago.

carlosV2 avatar Jul 13 '23 12:07 carlosV2

Hey @carlosV2 I also have some problems with supporting both ws and socket.io Including, but not limited to: 1. The HandleWs type in index.d.ts no it looks like this:

import type { WebSocketServer } from "ws";
export type HandleWs = (wss: WebSocketServer) => void;

And it should become something like:

import type { WebSocketServer } from "ws";
import { Server } from "socket.io";

export type HandleWs<T extends 'ws' | 'socket.io' = 'socket.io'> = T extends 'ws' ? (wss: WebSocketServer) => void :  (server: Server) => void

This happens more often, that I have to default back to ws to remain backwards compatibility. And if the user uses socket.io they might get confused by the fallback2. We are shipping the (quite big)socket.iopackage even though they might only use thews` package

So I propose the following: Either we release a new version that breaks backwards compatibility Or. I fork this repo and make a new node-adapter-socketio package

The last one has my preference.

joppekoers avatar Jul 15 '23 09:07 joppekoers

Hi @SirMorfield,

It's always up to you if you want to fork the project 🙂.

In my humble opinion, I think you are overcomplicating things a bit. You are too focused on merging both systems into a single set of files. Like I said before, I think it would be easier and simpler if we had 2 sets of files, one dedicated to ws and another set dedicated to socket.io.

If you still want to continue working on this project, I suggest you create the socketio folder and copy/paste all the relevant files inside, then you will be free to amend them as you see fit to work on socket.io alone.

Regarding the package size, we can take a look at this at a later stage. Maybe we can request people to additionally install the socket.io package separately.

Hope this helps.

carlosV2 avatar Jul 19 '23 18:07 carlosV2