Skip to content

Remove pubternal types from Kestrel #8306

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

Closed
jkotalik opened this issue Mar 7, 2019 · 12 comments
Closed

Remove pubternal types from Kestrel #8306

jkotalik opened this issue Mar 7, 2019 · 12 comments
Assignees
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions Done This issue has been fixed feature-kestrel

Comments

@jkotalik
Copy link
Contributor

jkotalik commented Mar 7, 2019

MVC recently made all pubinternal types in MVC internal via #4932. As a reminder, pubinternal types are types that are declared as public but put in an .Internal namespace.

In ASP.NET Core 3.0, we should do the same in Kestrel, specifically for the Microsoft.AspNetCore.Server.Kestrel.Core.Internal namespace. Reference assemblies will also show any pubinternal changes.

cc @muratg @halter73 @Tratcher

@Tratcher
Copy link
Member

Tratcher commented Mar 7, 2019

Just Kestrel? I would expect this to extend to most of AspNetCore.

@jkotalik
Copy link
Contributor Author

jkotalik commented Mar 7, 2019

I mean sure, but let's start somewhere. The issue for MVC was individualized.

@muratg
Copy link
Contributor

muratg commented Mar 7, 2019

With the targetting pack work, I think this is more critical now. Making breaking changes will be harder in these technically public types.

@Eilon @DamianEdwards @davidfowl thoughts?

@Tratcher
Copy link
Member

Tratcher commented Mar 7, 2019

@jkotalik Think bigger 😁

@halter73
Copy link
Member

halter73 commented Mar 8, 2019

Definitely not just Kestrel, though I've heard through the grapevine that EF actually likes pubternal.

@Eilon
Copy link
Contributor

Eilon commented Mar 8, 2019

EF Core is sticking with pubternal. But MVC removed a lot of pubternal (either make true internal or make true public). Up to each feature area to make a call.

@pranavkm
Copy link
Contributor

pranavkm commented Mar 8, 2019

#8308 if there's interest in doing for everything that ships in the targeting pack

@jkotalik
Copy link
Contributor Author

jkotalik commented Mar 8, 2019

Thanks @pranavkm <3.

@benaadams
Copy link
Member

You'll break the TE Platform tests though as that uses pubternal apis /cc @davidfowl

@benaadams
Copy link
Member

If the used apis were made public they'd have to switch from Span<byte> to ReadOnlySpan<byte>; which they should do eventually anyway.

@Eilon
Copy link
Contributor

Eilon commented Mar 8, 2019

TE Platform tests definitely shouldn't be using pubternal types 😭

@jkotalik jkotalik self-assigned this Mar 8, 2019
@jkotalik jkotalik added Done This issue has been fixed and removed 2 - Working labels Mar 19, 2019
@jkotalik
Copy link
Contributor Author

May be some follow up to decide if some types should be public.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 3, 2019
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions Done This issue has been fixed feature-kestrel
Projects
None yet
Development

No branches or pull requests

9 participants