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

WIP: Add WebSockets lib #182

Open
wants to merge 33 commits into
base: master
from

Conversation

Projects
None yet
10 participants
@forki
Contributor

forki commented Dec 18, 2017

Not sure where it belongs.

  • - discuss if it should go in separate package
  • - XML document the functions
  • - document the sockets lib in readme
  • - add tests
  • - add samples
@TheAngryByrd

This comment has been minimized.

Member

TheAngryByrd commented Dec 18, 2017

Might we worth adding some tests too using TestServer like

use server = new TestServer(WebHostBuilder().Configure(fun app -> configure app))
let wsClient = server.CreateWebSocketClient()
let! websocket = wsClient.ConnectAsync(server.BaseAddress, CancellationToken.None)
@TheAngryByrd

This comment has been minimized.

Member

TheAngryByrd commented Dec 18, 2017

There also might be some value comparing and contrasting this implementation with https://gist.github.com/TheAngryByrd/22b5df5a308f9f96f959980d0a42f7cd

For instance we probably should rely on WebSocketReceiveResult.EndOfMessage
in the receive part and buffer into a stream rather than hoping the message size is big enough to handle the incoming text data.

@forki

This comment has been minimized.

Contributor

forki commented Dec 18, 2017

@TheAngryByrd please send PRs to my fork

@forki

This comment has been minimized.

Contributor

forki commented Dec 19, 2017

@TheAngryByrd I made receive into a loop. I would appreciate some tests

@forki

This comment has been minimized.

Contributor

forki commented Dec 22, 2017

@TheAngryByrd thanks for working with me together on this. I'm sure we can create something cool out of it.

@@ -12,12 +12,17 @@
<PackageReference Include="Microsoft.AspNetCore.Hosting" Version="2.0.*" />
<PackageReference Include="Microsoft.AspNetCore.TestHost" Version="2.0.*" />
<PackageReference Include="Microsoft.AspNetCore.StaticFiles" Version="2.0.*" />
<PackageReference Include="Microsoft.AspNetCore.WebSockets" Version="2.0.*" />

This comment has been minimized.

@forki

forki Dec 25, 2017

Contributor

This is so dangerous. It should be pinned or converted to paket. Floating versions are such an anti pattern. Especially together with transitive deps.

@forki forki closed this Dec 25, 2017

@forki forki reopened this Dec 25, 2017

forki added some commits Dec 18, 2017

@forki

This comment has been minimized.

Contributor

forki commented Dec 29, 2017

@TheAngryByrd I'm now using TokenRouter to configure the server. And we have a broadcast test

@enricosada

This comment has been minimized.

enricosada commented Dec 29, 2017

Another good idea ihmo (found checking ws with suave in FSAC) will be implement the autobahn test suite to test the server http://websockets.github.io/ws/autobahn/servers/ to test the implementation ( link is the node ws test report ).

@forki

This comment has been minimized.

Contributor

forki commented Dec 30, 2017

@TheAngryByrd @gerardtoconnor for some reason the tests with n=100 / 1000 are going into timeout. Any ideas why it is soooo slow?

@forki

This comment has been minimized.

Contributor

forki commented Dec 30, 2017

ok I found the issue

forki added some commits Dec 29, 2017

@forki

This comment has been minimized.

Contributor

forki commented Dec 31, 2017

@TheAngryByrd could you please review this and comment on the current version?

@forki

This comment has been minimized.

Contributor

forki commented Feb 12, 2018

Incorporated all feedback

@dustinmoris

This comment has been minimized.

Member

dustinmoris commented Feb 13, 2018

Hi, I don't like letting people wait, so I've written an update on the Swagger PR and I was thinking of proposing the same here if that sounds good to you @forki ?

@forki

This comment has been minimized.

Contributor

forki commented Feb 13, 2018

I'm fine with waiting for now.

@dustinmoris

This comment has been minimized.

Member

dustinmoris commented Feb 13, 2018

Ok cool, just wanted to let everyone know that things are progressing every day and I'll eventually get to the bigger PRs as well. If something is urgent then there is that option of an early Alpha release.

@cotyar

This comment has been minimized.

cotyar commented Feb 13, 2018

Guys,
For the ones who don't want to wait until everything is done properly I just created a branch merging both PRs together (just as a temporal solution)
https://github.com/cotyar/Giraffe/tree/SwaggerAndWebsockets
Cheers

@Banashek

This comment has been minimized.

Contributor

Banashek commented Feb 26, 2018

I had created a similar implementation based on the asp docs and this blog post. I like the PR here a bit more.

One thing I noticed in the current PR state is that there is no opportunity to add behavior when a socket is closed. This is useful for updating UI when a user has left the web app. If there was perhaps an overload of onMessage where messageF was of type WebSocketReference -> string -> WebSocketMessageType -> Task<unit> it could allow patching in behavior based on the message type by the user.

@forki

This comment has been minimized.

Contributor

forki commented Feb 26, 2018

@Banashek can you please send a pull request to my fork with your proposal? Let's work together on this to make it great

TheAngryByrd and others added some commits May 15, 2018

Merge pull request #3 from TheAngryByrd/websocketErrorHandling
Adding some error handling around receiving of messages
@Nhowka

This comment has been minimized.

Contributor

Nhowka commented May 30, 2018

I plan to create a version of Elmish.Remoting for Giraffe. Should I wait a little more for this PR to get merged?

@cotyar

This comment has been minimized.

cotyar commented May 31, 2018

No updates from Dustin for quite a while unfortunately.
And the latest version of ws breaks the previous code contract unfortunately and isn't a dropin replacement :-(

I would love to see the remoting working over WebSockets for Giraffe!

@forki

This comment has been minimized.

Contributor

forki commented May 31, 2018

This is still WIP since I'm still trying to figure out an idiomatic way

@Nhowka

This comment has been minimized.

Contributor

Nhowka commented May 31, 2018

Based on what you did until now, do you think it would be too much effort to replicate what Suave did?

@cotyar

This comment has been minimized.

cotyar commented May 31, 2018

I personally use 3 months old version (without latest commits) and it does the job pretty well for me, but I'm missing onClose event. The latest one has onClose but doesn't pass the ref into onOpen (as far as I remember)

@forki

This comment has been minimized.

Contributor

forki commented Jun 1, 2018

@cotyar

This comment has been minimized.

cotyar commented Jun 1, 2018

Thank you @forki , much appreciated!

Hope @dustinmoris will kindly merge it into the master and release soon together with the Swagger PR.

@coolya

This comment has been minimized.

coolya commented Sep 29, 2018

I this PR dead? Haven't got any activity for three month, is there any way I can help here to get websocket support in Giraffe?

@dustinmoris

This comment has been minimized.

Member

dustinmoris commented Sep 29, 2018

@forki

This comment has been minimized.

Contributor

forki commented Sep 29, 2018

@dustinmoris

This comment has been minimized.

Member

dustinmoris commented Sep 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment