Skip to content

WebTransport interfaces require preview features#43419

Merged
adityamandaleeka merged 1 commit intorelease/7.0-rc1from
halter73/42490-preview
Aug 22, 2022
Merged

WebTransport interfaces require preview features#43419
adityamandaleeka merged 1 commit intorelease/7.0-rc1from
halter73/42490-preview

Conversation

@halter73
Copy link
Copy Markdown
Member

AcceptAsync already had the [RequiresPreviewFeatures]. This just extends the treatment to the entire interface so we can make breaking changes to this API in a later release. We think we might expose a MultiplexedConnectionContext down the road.

Closes #42490

AcceptAsync already had this. This just extends the treatment to the
entire interface so we can make breaking changes to this API in a
later release. We think we might expose a MultiplexedConnectionContext.
using Microsoft.AspNetCore.Http.Features.Authentication;
using Microsoft.AspNetCore.Server.Kestrel.Core.Features;

#pragma warning disable CA2252 // WebTransport is a preview feature
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is what Daniel was telling me for a reason he didn't set the preview attribute higher, it's a bit viral. But it's better from a "we can break this in the future" perspective.

By suppressing it normal apps won't see the warning until they touch IHttpWebTransportFeature or IWebTransportSession right?

Copy link
Copy Markdown
Member Author

@halter73 halter73 Aug 20, 2022

Choose a reason for hiding this comment

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

By suppressing it normal apps won't see the warning until they touch IHttpWebTransportFeature or IWebTransportSession right?

Correct. If we don't mark the entire interfaces as previews, we'd theoretically be unable to make changes in a future release that break any callers to or implementers of these interfaces.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, this is exactly why we didn't do it this way earlier. Your fix is correct, but annoying to work with 😁

@halter73
Copy link
Copy Markdown
Member Author

@adityamandaleeka Can we merge this into rc1?

@adityamandaleeka
Copy link
Copy Markdown
Member

Approved.

@adityamandaleeka adityamandaleeka merged commit 33c3c7c into release/7.0-rc1 Aug 22, 2022
@adityamandaleeka adityamandaleeka deleted the halter73/42490-preview branch August 22, 2022 20:45
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
halter73 added a commit to halter73/AspNetCore that referenced this pull request Nov 20, 2024
AcceptAsync already had this. This just extends the treatment to the
entire interface so we can make breaking changes to this API in a
later release. We think we might expose a MultiplexedConnectionContext.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants