Clean up Pubternal Types in SignalR

[mikaelm12]

Issue: #10169

Some things to note

• ServerSentEventsMessageFormatter used in the Microbenchmark project but that causes a bunch of issues
• HubOptionsSetup and HubOptionsSetup<T> is used in SignalRDependencyInjectionExtensions so Ieft those as pubternal. Didn't want to use internals visible to in this case. Should this just be public?

[halter73]

I think HubOptionsSetup and HubOptionsSetup<T> should be public if they're referenced across shipped assemblies.

[halter73]

The approval assumes HubOptionsSetup and HubOptionsSetup<T> are the last pubternal types and will be made public.

[mikaelm12]

I agree about the HubOptions. The ServerSentEventsMessageFormatter is currently still pubternal because of the Microbenchmarking project. I still need to resolve that

[analogrelay]

The ServerSentEventsMessageFormatter is currently still pubternal because of the Microbenchmarking project. I still need to resolve that

It seems OK to use InternalsVisibleTo there, does it not work? It turns out Arcade is the one that provides the InternalsVisibleTo MSBuild item and we do support it in AspNetCore. See https://github.com/aspnet/AspNetCore/blob/f28cf2bbc8e97a05be575a61af08f01edf0ac9c9/src/Middleware/HeaderPropagation/src/Microsoft.AspNetCore.HeaderPropagation.csproj#L12-L14

[analogrelay]

• HubOptionsSetup and HubOptionsSetup<T> is used in SignalRDependencyInjectionExtensions so Ieft those as pubternal.

Seems unfortunate but it would be pretty funky to refactor. We could put a method in ...SignalR.Core somewhere that takes an IServiceProvider and registers HubOptionsSetup in it, but that gets a bit messy. Make sure only the constructor (because DI requires public constructors) and interface implementations are public

[davidfowl]

The ref assembly is the only way to review this change.

[mikaelm12]

It seems OK to use InternalsVisibleTo there, does it not work?

I should have elaborated in the description. Just adding internals visible to doesn't doesn't quite solve the issue. It causes some ambiguous references to types like MemoryBufferWriter. That's the last thing I need to work through.

[BrennanConroy]

We should also try to switch to the new <InternalsVisibleTo> tag in .csproj's!

[mikaelm12]

Had to revert the change to use the InternalsVisibleTo msbuild tag because it doesn't work with Moq
Filed an issue with arcade: dotnet/arcade#3068

[mikaelm12]

@aspnet-hello triage https://dev.azure.com/dnceng/public/_build/results?buildId=227887&view=logs

[aspnet-hello]

This comment was made automatically. If there is a problem contact [email protected].

I've triaged the above build. I've created/commented on the following issue(s)
https://github.com/aspnet/AspNetCore-Internal/issues/2303

[Pilchie]

Roslyn is able to use InternalsVisibleTo in MSBuild with Moq (see here)

[BrennanConroy]

https://github.com/dotnet/roslyn/blob/d4afaea39dc21a45ac119dd2bec9be678ce5081d/src/VisualStudio/Core/Def/Microsoft.VisualStudio.LanguageServices.csproj#L120

Ah, they had to provide the key

[mikaelm12]

Ah okay, I'll try that out. Thanks!

[davidfowl]

Wow this is kinda gross 😄

[BrennanConroy]

Should we also remove HubDispatcher from DI now that it's no longer used in DI?

[davidfowl]

I'll follow up with why ServerSentEventsMessageFormatter is public.

[mikaelm12]

Going to follow this up with another clean up pr for InternalsVisibleTo and SSEMessageFormatter

[parkycai]

Is there a new way to implement custom HubDispatcher and inject it?