search

philippseith / signalr

SignalR server and client in go
MIT License
131 stars 39 forks source link

feat: add support for web transports #191

Closed sruehl closed 7 months ago

sruehl commented 8 months ago

resolves https://github.com/philippseith/signalr/issues/190 resolves https://github.com/philippseith/signalr/issues/193

philippseith commented 8 months ago

Nice! Were you able to verify it with any third party client/server? AFAIK aspnetcore did not implement it yet: https://github.com/dotnet/aspnetcore/issues/39583

sruehl commented 8 months ago

At the moment I'm in the process of verification, hence the PR is still in draft. I'll update here once I know that it works

sruehl commented 8 months ago

Also I wonder if we should merge it in this state and fix potential issues later or temporary return a proper error on master?

sruehl commented 8 months ago

And regarding the failing Unit test. I have no idea what is going on there...

sruehl commented 8 months ago

Also I wonder if we should merge it in this state and fix potential issues later or temporary return a proper error on master?

@philippseith I prepared a PR for option 2 https://github.com/philippseith/signalr/pull/192.

sruehl commented 8 months ago

seems that Unit test failure is unrelated to this PR and is caused by https://github.com/philippseith/signalr/commit/35b4977f8fe6e61cdb111ae3977ad717e2bfe4ec

sruehl commented 8 months ago

I rebased this on #192

sruehl commented 8 months ago

This PR should no be safe to merge as it fixes two issues:

  1. When Server responds WebTransports the client should stop panic (#192 at least stops the panic but you still get a error when Server responds with this Transport type)
  2. WebTransports is only used in this state when the Client explicitly configures the Transport via ClientOptions