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

Add a websocket string binding #26

Merged
merged 3 commits into from
Dec 8, 2021

Conversation

andydotxyz
Copy link
Member

A simple WebSocket based String binding.
Works well in my testing :)

Copy link
Member

@Jacalz Jacalz left a 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 :)

Copy link
Member

@stuartmscott stuartmscott left a 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 :)"

README.md Outdated Show resolved Hide resolved
// 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{})
Copy link
Member

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

@Jacalz
Copy link
Member

Jacalz commented Dec 8, 2021

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 :)"

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 :(

@andydotxyz
Copy link
Member Author

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 did consider this, however we don't have a []byte data primitive so I got a bit stuck. Using Untyped was an option, but I didn't want to expose the Get and Set of interface{} type.
I suppose I could either implement the []byte version to support this.
As for generic approaches I was not sure how valuble they would be, as web sockets seem to be more used for data packets (JSON etc) rather than primitive values - so I figured lowest common denominator would be most useful.

@andydotxyz
Copy link
Member Author

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 :(

In terms of maintained probably the golang.org/x/net/websocket would be best - but their API was harder to understand so I used one of the easier ones :)

@Jacalz
Copy link
Member

Jacalz commented Dec 8, 2021

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 :(

In terms of maintained probably the golang.org/x/net/websocket would be best - but their API was harder to understand so I used one of the easier ones :)

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 😅

@stuartmscott
Copy link
Member

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

@Jacalz
Copy link
Member

Jacalz commented Dec 8, 2021

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?

@stuartmscott
Copy link
Member

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.

@Jacalz
Copy link
Member

Jacalz commented Dec 8, 2021

That's a very good point. I could see []byte bindings being very useful then.

@andydotxyz
Copy link
Member Author

Agreed, a []byte binding is more useful than perhaps we realised.
I will look to add that in Fyne.
In the meantime what should we do here? does NewWebServiceString make sense, and NewWebServiceByte could be added later, along with other possible types as we see fit? Or does this PR need some real changes to avoid assuming the []byte contains a string?

@Bluebugs
Copy link
Contributor

Bluebugs commented Dec 8, 2021

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.

@stuartmscott
Copy link
Member

In the meantime what should we do here?

It is probably okay to merge the string version now and expand it in future, so far the API introduced here has a String suffix so there shouldn't be a problem when more types are added.

@andydotxyz andydotxyz merged commit c819209 into fyne-io:master Dec 8, 2021
@andydotxyz andydotxyz deleted the feature/webdata branch December 8, 2021 23:05
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 this pull request may close these issues.

4 participants