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

Feature: Replace Gorilla WS with Nhooyr WS #128

Open
2 tasks done
ElecTwix opened this issue Mar 9, 2024 · 9 comments · May be fixed by #143
Open
2 tasks done

Feature: Replace Gorilla WS with Nhooyr WS #128

ElecTwix opened this issue Mar 9, 2024 · 9 comments · May be fixed by #143

Comments

@ElecTwix
Copy link
Contributor

ElecTwix commented Mar 9, 2024

Is your feature request related to a problem?

No, but it is will great when fixing #119

Nhooyr's implementation is more lightweight and idiomatic than Gorilla's

For instance, Turso's Go driver, a popular database library, demonstrates the effectiveness of Nhooyr's WebSockets implementation (https://github.com/tursodatabase/libsql-client-go).
Also golang std ws mention Nhooyr's ws as

more actively maintained WebSocket
ref

Describe the solution

Remove Gorilla WS support and add Nhooyr WS support.
These changes will not affect function signatures and will improve the maintainability of the repository.

Alternative methods

There was an attempt to add Can along with Gorilla but it was rejected. #127. I considered using a soft switch but it is not feasible.

SurrealDB version

non related

Contact Details

[email protected]

Is there an existing issue for this?

  • I have searched the existing issues

Code of Conduct

  • I agree to follow this project's Code of Conduct
@gedw99
Copy link

gedw99 commented Mar 26, 2024

Hey @ElecTwix

I also like https://github.com/nhooyr/websocket for many different reasons. The most useful for me was that it can work when you compile your golang to wasm.

I see you have already jumped the gun and are doing it here: https://github.com/ElecTwix/surrealdb-custom.go. Are you planning to try to merge upstream If the SurrealDB maintainer wants to take it ?

https://github.com/btwiuse/wsconn/blob/main/go.mod looks to be an adapter that supports both gorilla and nhooyr. its mint be another option...

@ElecTwix
Copy link
Contributor Author

Hey @gedw99

I also like https://github.com/nhooyr/websocket for many different reasons. The most useful for me was that it can work when you compile your golang to wasm.

I didn't take that to count thanks for pointing that out.

I see you have already jumped the gun and are doing it here: https://github.com/ElecTwix/surrealdb-custom.go. Are you planning to try to merge upstream If the SurrealDB maintainer wants to take it?

I'm a big fan of SurrealDB and would love to contribute. While PR #127 wasn't accepted, I'm open to creating PR that introduces Nhooyr WS to go driver with the SurrealDB team's vision if they're interested in the future.

https://github.com/btwiuse/wsconn/blob/main/go.mod looks to be an adapter that supports both gorilla and nhooyr. its mint be another option...

Looks good, but I need to confirm compatibility with the gorilla's custom system type. I'll look into it.

@gedw99
Copy link

gedw99 commented Apr 4, 2024

HI @ElecTwix

what's required to get the Nhooyr WS accepted though ? Maybe I dont understand what you're suggesting ?

@ElecTwix
Copy link
Contributor Author

ElecTwix commented Apr 4, 2024

HI @ElecTwix

what's required to get the Nhooyr WS accepted though ? Maybe I dont understand what you're suggesting ?

Hi @gedw99,
Nhooyr WS ready to merge; awaiting approval from SurrealDB team. for context is rejected once with: PR

@phughk
Copy link
Contributor

phughk commented May 13, 2024

Heya, thanks for creating this feature request @ElecTwix and ongoing enthusiasm and efforts to improve the driver for everyone!

Overall, not opposed to changing the library. Personally I am not too familiar with the golang websocket space, but having had a search it seems that this library has a lot of performance benefits
https://github.com/gobwas/ws

One thing that stands out is that Nhooyr supposedly handles Context, whereas I haven't immediately been able to see that in gobwas. If gobwas supports context then I think that may be preferable. And they should both work for WebAssembly, seeing as gobwas does not have dependencies that would prevent that https://github.com/gobwas/ws/blob/master/go.mod

What do you think?

@ElecTwix
Copy link
Contributor Author

Heya, thanks for creating this feature request @ElecTwix and ongoing enthusiasm and efforts to improve the driver for everyone!

Thanks for your kind words @phughk.
I'm happy to help SurrealDB and everyone that uses it.

Overall, not opposed to changing the library. Personally I am not too familiar with the golang websocket space, but having had a search it seems that this library has a lot of performance benefits https://github.com/gobwas/ws

I know gobwas, it is great. For the test, I will also create an implementation for surrealdb to see the difference between both.

One thing that stands out is that Nhooyr supposedly handles Context, whereas I haven't immediately been able to see that in gobwas. If gobwas supports context then I think that may be preferable. And they should both work for WebAssembly, seeing as gobwas does not have dependencies that would prevent that https://github.com/gobwas/ws/blob/master/go.mod

What do you think?

gobwas doesn't have context support out of the box but it can be implemented, the problem with gobwas seems like it will need more work to maintain but I'm not sure like I said second answer I will create and we can decide afterward

@ElecTwix
Copy link
Contributor Author

I've been evaluating the gobwas WebSocket implementation. While it performs well, I have some concerns about its suitability for our project. Here's a breakdown of my findings:

  • Non-Idiomatic Go: Using gobwas might require writing code that deviates from idiomatic Go practices. This can lead to maintenance difficulties down the line.

  • Similar Performance with Idiomatic Go: The nhooyr implementation appears to offer comparable performance while adhering to idiomatic Go conventions, potentially making it a more sustainable choice.

  • Limited Client-Side Examples: gobwas seems to lack a comprehensive set of official examples for client-side WebSocket usage. This could make it challenging to integrate it effectively into our project.

I think we should go with nhooyr's version I will create PR replace gorilla ws still we can discuss I just want to gain sometime while waiting.

@phughk I know you are not related to go but we started conversation if you have time could you check thanks :)

@ElecTwix ElecTwix linked a pull request Jun 17, 2024 that will close this issue
@nickchomey
Copy link

Just adding this update here. Coder took over maintenance of the nhooyr/ws package a month ago. It appears that nhooyr developed it while working at Coder. They seem to be mature, well-funded etc.., which can only bode well for the package.

The new repo is at https://github.com/coder/websocket

@nickchomey
Copy link

Here's another option that seems promising

https://github.com/lxzan/gws?tab=readme-ov-file

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

Successfully merging a pull request may close this issue.

4 participants