Open Bluebugs opened 2 years ago
Thanks for the PR on this. Have you been able to successfully run your modified version of the library within a browser?
I'm really interested in feedback on this change from those using Websockets (because I don't). Whilst the Gorilla Websocket package is looking for a new maintainer it's still fairly active, widely used (github stats say used by 45.3k projects) and stable. github.com/nhooyr/websocket
is a lot newer (first release 2019), not as widely used (github only lists one user) and the vast majority of commits are from the main author (however the API does look clean and it offers a comparison with the Gorilla library).
Gorilla websockets uses the BSD 2-Clause "Simplified" License and github.com/nhooyr/websocket
the MIT License so I don't forsee any issues on that front.
Yes, I have been using it to actually run the application described in this blog inside a browser: https://fynelabs.com/2022/01/10/easy-mqtt-application-with-fyne/ . I don't have a convenient way to host it at the moment, but if that would help, I could figure out a place to host it.
Thanks @Bluebugs - just wanted to confirm that it was all working (as I have not tried using this with tinygo/WASM).
Sorry, I have been using go main compiler not tinygo. tinygo is on my list of things to experiment with, but I haven't gotten to it yet.
Anything I could do to help get this issue move forward?
@Bluebugs adding some tests is likely to speed up the process (without these I'll have to test manually). Having said that I don't see a reason to rush this because there is a workaround (use ClientOptions.SetCustomOpenConnectionFn
).
My aim would be to make a new release before accepting this. It's a relatively major change so I'd like to see it used by those pulling @master
for a while before unleashing it on everyone.
@MattBrittan how would you recommend me going at those tests. Do you have a mqtt server setup with websocket I can use in the context of the automated tests?
@Bluebugs yep. The tests are on the fvt
folder and there is a docket folder under that (the idea being to spin up mosquito and run the tests). You can just add websockets to the mosquito confiig file.
Ok, I will look into that in a week or so as I have a lot for next week.
I've had another quick look at this. My main concern is that nhooyr.io/websocket
has a significant number of dependencies and changing to this significantly increases the size of resulting executables. As a test I cloned the current library and your PR and compiled cmd.sample
on both; the resulting executable sizes (windows) were:
6327808 sample.exe <- master 7107584 sample.exe <- your PR
So that's a 12% increase (around 800kb) with the move to nhooyr.io/websocket
(and this change impacts all users). For most packages this would not be much of an issue (go executables are pretty big anyway) but as this library gets used on fairly low spec devices (I run it on Teltonika routers) it's something I want to keep an eye on. Based on this I'm reluctant to accept this PR - I would expect that ClientOptions.SetCustomOpenConnectionFn
should provide a fairly easy to implement workaround without impacting all users.
Note: If any serious issues are identified with the gorilla package then I'd be happy to move (but do note that a new version of that package was released in Feb).
I didn't realize this would have been a concern as this module doesn't compile with tinygo last time I checked. I would be interested to see with a real application how much that does really represent as the sample application doesn't do very much, but I understand your concern. I will try to see how it fair with SetCustomOpenConnectionFn.
Thanks - please let me know how you get on (would be happy for such a module to be contributed; might add a plug-in folder off the repo for this kind of thing).
I'm not saying no to the PR but am trying to weigh up the benefits vs costs. If there is an easy workaround that meets your needs, which I suspect there is, then that removes any immediate need to make the change. I need to consider the impact of any change on average users (who are probably not even using web sockets) so think checking the impact on output size is of relevance (in this case I was concerned by number of dependencies nhooyr.io/websocket
has; realise that almost all of these relate to tests but it means go mod tidy
pulls in quite a bit!).
My use-case may be a little unusual as I just use the standard compiler; runs fine on a MIPS 74Kc with 128kb RAM (but I do keep an eye on the size due to fairly low bandwidth comms.
It should be possible to support web assembly as a target for this library. The only way to support wasm target is to use a websocket library that support it as a target.
The current websocket library github.com/gorilla/websocket does not support and won't support wasm soon (https://github.com/gorilla/websocket/issues/432). gorilla/websocket is also looking for a maintainer (https://github.com/gorilla/websocket/issues/370) even if still does see some development activity (https://github.com/gorilla/websocket/graphs/code-frequency).
github.com/nhooyr/websocket does have support for wasm and is actively maintained. The library is also simpler and follow go net API more closely. It will actually simplify this library code switching to this other websocket library. The work for just a change can be seen here: 583