-
Notifications
You must be signed in to change notification settings - Fork 61
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 a websocket string binding #26
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, I don't have much knowledge of the web stuff. However, I have seen some people saying that https://github.com/nhooyr/websocket might be an even better library for websockets, especially since gorilla isn't maintained any more (which kind of seems to be the case with the one above as well sadly). Just getting the information out there. I'll not force you to switch :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the data received is just byte[]
which this casts to a string
, I wonder if this could be made more generic to provide NewWebSocketXYZ
for all primitive data binding types in Fyne.
I can't really comment on gorilla websocket library vs the nhooyr one suggested because I haven't used either, but I am surprised at the claim gorilla is not maintained since the last commit was in April this year and its mux library is widely used - @Jacalz was there an announcement I missed? Gorilla seems like a org that are passionate about net libraries. That's not to say nhooyr isn't, but his status is currently "on a sabbatical, back soon :)"
data/binding/webstring.go
Outdated
// You should also call `Close()` on the binding once you are done to free the connection. | ||
func NewWebSocketString(url string) (StringCloser, error) { | ||
s := binding.NewString() | ||
sock, _, err := websocket.DefaultDialer.Dial(url, http.Header{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer conn
- consistent with type and field name
They have an issue open about the lack of a maintainer: gorilla/websocket#370. That april commit is also an outlier. Apart from that commit, the latest commit is from September last year. https://github.com/nhooyr/websocket is not massively better in that regard either to be honest. Seems like all big websocket libraries for Go are more or less abandoned :( |
I did consider this, however we don't have a |
In terms of maintained probably the |
That project is actually even worse: https://github.com/golang/net/commits/master/websocket. Latest commit was pre-covid and that says a lot unfortunately 😅 |
Still, at least two people have offered to maintain the gorilla one, and the community is bigger so I think it is a safer bet than either of the others mentioned. Kinda surprised there is no byte[] binding, figured it would be useful for a binding to a file. Then there would be data converters like byte[] to string so this websocket binding could be generic byte[] and a converter would be used to attach it to a label |
I can see the use case for a []byte binding, but wouldn't it make more sense to bind to an io.Reader for files? |
Well bindings are two-way, so it would have to be io.Reader & io.Writer, but since the bindings don't duplicate the data it wouldn't make a difference to pass the reference to the byte slice. Also you could do some optimizations, like if only one byte in the file changes you just notify the binding of that index, rather than passing around a new reader to read the whole again. Similarly if the file was appended it would be a length change notification. You might be able to do something similar with io.ReadSeeker, but you'd need another binding to pass the seek offset. |
That's a very good point. I could see []byte bindings being very useful then. |
Agreed, a |
If you are looking at adding lower primitive to Fyne, on the nice to have for network services a JSON and protobuf type would be useful. Maybe something that can be converted from a struct to a JSON/Protobuf. |
It is probably okay to merge the string version now and expand it in future, so far the API introduced here has a |
A simple WebSocket based
String
binding.Works well in my testing :)