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

feat: add support for web transports #191

Merged
merged 8 commits into from
Jan 18, 2024

Conversation

sruehl
Copy link
Contributor

@sruehl sruehl commented Jan 3, 2024

resolves #190
resolves #193

@philippseith
Copy link
Owner

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

@sruehl
Copy link
Contributor Author

sruehl commented Jan 4, 2024

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

@@ -126,7 +127,14 @@ func NewHTTPConnection(ctx context.Context, address string, options ...func(*htt
var conn Connection
switch {
case negotiateResponse.hasTransport("WebTransports"):
// TODO
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This way the way I noticed btw as this lead to a nasty nil pointer at a completely unrelated place (connection nil and error nil at the same time).

@sruehl
Copy link
Contributor Author

sruehl commented Jan 4, 2024

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
Copy link
Contributor Author

sruehl commented Jan 4, 2024

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

@sruehl
Copy link
Contributor Author

sruehl commented Jan 5, 2024

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 #192.

@sruehl
Copy link
Contributor Author

sruehl commented Jan 5, 2024

seems that Unit test failure is unrelated to this PR and is caused by 35b4977

@sruehl sruehl force-pushed the feat/webtransports branch from a4070cd to 6aa9891 Compare January 8, 2024 08:34
@sruehl
Copy link
Contributor Author

sruehl commented Jan 8, 2024

I rebased this on #192

@sruehl
Copy link
Contributor Author

sruehl commented Jan 8, 2024

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

  1. When Server responds WebTransports the client should stop panic (fix: don't panic when encountering web transports #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

@sruehl sruehl marked this pull request as ready for review January 8, 2024 08:43
@philippseith philippseith merged commit f92bf5c into philippseith:master Jan 18, 2024
5 of 6 checks passed
@sruehl sruehl deleted the feat/webtransports branch January 18, 2024 22: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.

Panic when connecting to old servers (or WebTransports) Feature: WebTransports
2 participants