Add attributes to SignalR source generator

[BrennanConroy]

Adding attributes to the source generator as a consumable dll, this way the attributes live with the project that will make use of them (because otherwise they aren't useful 😃)

Alternatives considered:

• Put the attributes in the Client.Core package
  • Only projects targeting the new client package will get the attributes
  • Attribute can be placed on valid methods but wouldn't do anything + compile error if source generator isn't also referenced
• Generate the attributes from the source generator
  • Code is owned by consumer so falls prey to issues like conflicting type if public or internals visible to when used across multiple projects
• Do nothing and look for attribute by name
  • Less discoverable and why make customers go through extra pain to use the feature

namespace Microsoft.AspNetCore.SignalR.Client
{
+    [AttributeUsage(AttributeTargets.Method)]
+    public class ClientHubAttribute : Attribute
+    {
+    }

+    [AttributeUsage(AttributeTargets.Method)]
+    public class ServerHubProxyAttribute : Attribute
+    {
+    }
}

Package Name:
Microsoft.AspNetCore.SignalR.Client.SourceGenerator
Attribute DLL Name:
Microsoft.AspNetCore.SignalR.Client.SourceGenerator.HubProxyAttributes.dll

Original name idea:
HubClientProxyAttribute
HubServerProxyAttribute

Alternative names:
ClientMethodsAttribute
SignalRClientAttribute
ClientRegistrationAttribute

[halter73]

Was this intended? The only non-private asset I see is HubProxyAttributes which isn't packaged separately.

[BrennanConroy]

It's needed to put the .netstandard2.0 framework in the nuspec. Changed the reference to not bring in HubProxyAttributes as a reference.

[halter73]

dotnet pack now fails for me with the following:

C:\dev\dotnet\aspnetcore\.dotnet\sdk\7.0.100-alpha.1.21551.3\Sdks\NuGet.Build.Tasks.Pack\build\NuGet.Build.Tasks.Pack.targets(221,5): error NU5019: File not found: 'C:\dev\dotnet\aspnetcore\artifacts\bin\Microsoft.AspNetCore.SignalR.Client.SourceGenerator\Debug\netstandard2.0\Microsoft.AspNetCore.SignalR.Client.SourceGenerator.HubProxyAttributes.dll'. [C:\dev\dotnet\aspnetcore\src\SignalR\clients\csharp\Client.SourceGenerator\src\gen\Microsoft.AspNetCore.SignalR.Client.SourceGenerator.csproj]

What still seems to work for me is PrivateAssets="all" but then you also need a supression with <NoWarn>BUILD005;$(NoWarn)</NoWarn> or similar to silence a warning about using PrivateAssets with project references. It seems to do the right thing though.

[dougbu]

PrivateAssets="all" shouldn't work w/ project references. Private="true" is the way to go.

[halter73]

I tried Private="true" since that's what the build warning suggests, but that still leaves "Microsoft.AspNetCore.SignalR.Client.SourceGenerator.HubProxyAttributes" as a dependency in the nupkg.

[dougbu]

Ah, I meant to Private="false" but that controls copy-local and not dependencies added to a package. It wouldn't work alone (but see below).

@rainersigwald why does PrivateAssets="all" seem to work in this case❔ It's documented as applicable only for @(PackageReference) items and line 16 turns into a @(ProjectReference).

@BrennanConroy @halter73 another option: Use all of

      Private="false"
      ReferenceOutputAssembly="false"
      SkipGetTargetFrameworkProperties="true"

And, change the @(None) item to copy from $(ArtifactsDir)bin\HubProxyAttributes\$(Configuration)\netstandard2.0.

For one thing, this option would avoid timing issues due to HubProxyAttributes holding a file lock a bit after its build completes. @(None) items are handled (copied to this project's bin/ folder) a bit later than copy-local assemblies IIRC. ReferenceOutputAssembly="false" ensures NuGet and most of MSBuild ignores the referenced project and its assembly.

[rainersigwald]

@rainersigwald why does PrivateAssets="all" seem to work in this case❔ It's documented as applicable only for @(PackageReference) items and line 16 turns into a @(ProjectReference).

My guess is that it's inherited by an item generated by NuGet or the SDK flattening references and bringing transitive ones to this project, but I don't know for sure.

[dougbu]

Curious: Why is this a public type nested in an internal one instead of a top-level internal class❔

[dougbu]

Oops, read this totally wrong. I need food…

[pranavkm]

Any reason we feel the need to have a separate assembly? Could we put this in SignalR.Client? I ask because the assembly will end up in users build outputs since it is referenced by app code.

[BrennanConroy]

Yes, called it out in the summary

• Put the attributes in the Client.Core package
  • Only projects targeting the new client package will get the attributes
  • Attribute can be placed on valid methods but wouldn't do anything + compile error if source generator isn't also referenced

[pranavkm]

I guess further question - any reason we couldn't put the source generator in the Client.Core package? If it's only triggered by the presence of these attributes, we're good no?

[BrennanConroy]

Theoretically we could? But then you're limiting the generator to new clients which is an unnecessary restriction.

I ask because the assembly will end up in users build outputs since it is referenced by app code.

Why do we care about a single new dll?

[pranavkm]

I'm not really worried about the binary. I wanted to understand why we're choosing to ship the generator in a way that is markedly different from others.

But then you're limiting the generator to new clients which is an unnecessary restriction.

I feel that having it support arbitrary versions might be problematic if the code generation has to change. It also reduces the testing matrix dramatically every time you release a new version of this since you only ever have to test one version (vs n-versions). And if we aren't testing n-versions as I suspect, maybe it's a reason to tie to SignalR.Client package?

[pranavkm]

I had an offline discussion about this. I'm still not super convinced that trying to support older versions of the client lib is really an important goal but if the engineering team feels very strongly about it, I'm not going to stand in their way.

We should bring the assembly name and new attributes thru API review.

[BrennanConroy]

Alternative attribute names:
ServerHubProxyAttribute - ServerHub sounds better than HubServer
ClientHubAttribute - This represents a "client hub" not a proxy so this seems like a better name.

[mehmetakbulut]

Alternative attribute names: ServerHubProxyAttribute - ServerHub sounds better than HubServer ClientHubAttribute - This represents a "client hub" not a proxy so this seems like a better name.

@BrennanConroy I think "client hub" muddies the meaning of "hub". In SignalR docs, "hub" refers always to the server. Could we do something like ClientMethodsAttribute[*] or SignalRClientAttribute (with the accompanying SignalRHubProxyAttribute) or ClientRegistrationAttribute?

[ghost]

Thank you for your API proposal. I'm removing the api-ready-for-review label. API Proposals should be submitted for review through Issues based on this template.

[mkArtakMSFT]

@BrennanConroy do you plan to continue this work or should this PR be closed?