surrealdb / surrealdb.js

SurrealDB SDK for JavaScript
https://surrealdb.com
Apache License 2.0
294 stars 48 forks source link

Feature: Temp solution for Bun websocket? #195

Closed soya-miruku closed 5 months ago

soya-miruku commented 10 months ago

Is your feature request related to a problem?

The current functionality of Bun's WebSocket, which utilizes uws, presents a notable issue due to an ongoing bug. This bug significantly impacts the use of the surrealdb.js package. Specifically, without resorting to the ExperimentalSurrealHttp, the effective employment of this package is compromised. Although this workaround enables Bun to function at a basic level, it restricts certain advanced capabilities, notably the select live feature, which is essential for my project.

Describe the solution

I propose a temporary workaround by integrating the tcp-websocket package. This idea emerged from insights gained in a GitHub issue discussion related to Bun's WebSocket challenges (see this thread). The tcp-websocket package appears to be a suitable interim solution until Bun releases an official update or bug fix.

More information about the tcp-websocket package can be found here: TCP-WebSocket on GitHub.

Preliminary Testing and Results: To assess the feasibility of this solution, I conducted tests by cloning the surrealdb.js repository and incorporating the tcp-websocket package into the setup. The initial results were encouraging, showing that this package could effectively provide the needed functionality while retaining the critical select live feature.

Conclusion and Next Steps: Utilizing the tcp-websocket package as a temporary measure could offer a practical solution for developers currently hindered by the limitations in Bun's WebSocket implementation. I suggest further exploration and broader testing of this approach to determine its overall effectiveness as an interim fix, pending a permanent resolution from the Bun development team.

Alternative methods

n/a

SurrealDB version

1.0.0+20231109.7fe90d9 for linux on x86_64

SurrealDB.js version

0.11.0

Contact Details

No response

Is there an existing issue for this?

Code of Conduct

LucyEgan commented 10 months ago

I kinda like this solution for running it in bun and I don't think its too much of a jump as there is already a src/library/WebSocket/[node.ts, deno.ts]

From your testing was tcp-websocket a drop in replacement for the standard websocket?

Currently I've been able to just use ExperimentalSurrealHttp as live support isn't required where I'm using it but would prefer using websocket if possible, but from what I've seen of the websocket client bug in bun it might be a while

soya-miruku commented 10 months ago

Yes i agree, in terms of changes, i only had to modify the node.ts file (in the websocket folder) and just adding --platform node in the build script (just took the lazy approach here)

import TCPWebSocket from "npm:tcp-websocket";

function ModifyCloseMethod<T extends { new (...args: any[]): any }>(BaseClass: T) {
    return class extends BaseClass {
            // Override close method with a different parameter type
            close(code: number, reason: string) {
                    super.close(reason, code)
            }
    };
}

const WebSocketClose = ModifyCloseMethod(TCPWebSocket);

class WebSocket extends WebSocketClose {
    addEventListener = this.addListener;
}

export default WebSocket;

(i haven't had time to fully optimise the code) but after doing that, i was able to get everything to work normally thereafter. However one other thing to take note is that, trying to send large data causes the
ws.addEventListener("message", (e) => { to fail when trying to parse the message data as JSON

kearfy commented 10 months ago

While we can differentiate between browser/node/deno, I'm not so sure if we can do the same for bun. Besides that, this will add unneeded overhead node_modules folder for node projects if pulled off.

My suggestion is to either wait until bun fixes their websocket implementation, or to patch the library. Will leave this issue open in case I find time to look into this

LucyEgan commented 10 months ago

From what I can see in https://github.com/sxzz/unws/blob/main/package.json they use the global WebSocket "bun": "./src/native.mjs", so it might be able to be temp patched by making the tcp-websocket be global.WebSocket as the first thing done before loading surrealdb. But also if this is a valid fix for buns web sockets then it should be apart of unws if `tcp-websocket is more complete than buns?

(I've not validated any of this, just based on a morning browse :D )

Also yeh valid points @kearfy especially the overhead since there's not really a way to do bun only dependences right?

kearfy commented 10 months ago

You can probably detect some global constant that is only present for bun during runtime, but that's already extra overhead for dependencies. Let's hope they fix it soon. You could alternatively overwrite the global WebSocket class maybe? Just thinking along with you all 😅

Vexcited commented 10 months ago

Hi! Author of tcp-websocket right here,

@LucyEgan

tcp-websocket is more complete than buns

It's a bit false because my implementation was first made to fix my particular own issue with Bun's WebSockets.

So my library has some not implemented features that I might look into because Bun developers may not fix that HTTP headers casing issue (after some discussion in their Discord)

I don't really know surrealdb.js so if there's crucial WebSocket features you need, just tell me and I'll make them my priority for my next updates.


About the additional dependency, I understand that might be not good.

Patching the global WebSocket may be a solution, but maybe you can install tcp-websocket as optional dependency in the library and then tell somewhere in the documentation that if people are using Bun, they have to install that optional dependency. Then in the library, just check if it's being run under Bun, if so take the tcp-websocket package if installed, otherwise the global WebSocket.

LucyEgan commented 10 months ago

This is now working for us in bun 1.0.15 as they have updated their websocket client

kearfy commented 5 months ago

Closing as stale