dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.23k stars 9.95k forks source link

[SignalR] Strongly-typed Hub Proxies #15198

Open Daniel15 opened 4 years ago

Daniel15 commented 4 years ago

Edits from @anurse

Let's use this to track strongly-typed hub proxies all-up. We've talked about a few ways of doing it (code-gen, reflection emit, etc.). We have to consider a few things

Original issue follows

On the server side, we can create a strongly-typed hub (docs: https://docs.microsoft.com/en-us/aspnet/core/signalr/hubs?view=aspnetcore-3.0#strongly-typed-hubs). However, on the client-side, everything is still loosely typed, which means we lose type safety. Does the TypeScript client have any support for a strongly-typed connection? I imagine it could use generics on the build function, eg:

const connection = new HubConnectionBuilder()
  .withUrl("/hub")
  .build<MyHub>();

My ideal setup would be automatically generating a TypeScript client based on the C# strongly-typed Hub, but for now I'd be fine just manually creating a TypeScript type that replicates the server-side one.

BrennanConroy commented 4 years ago

There is no support for this currently. We've definitely discussed it internally in the past but have prioritized other work as there hasn't been very much customer ask.

This is certainly a feature we would like to have :)

analogrelay commented 4 years ago

I think we'd do this beyond just the TypeScript client. We have https://github.com/aspnet/AspNetCore/issues/5278 which is tracking a slightly different thing (using a client-side class to replace the On methods). It sounds like what you're looking for is something like the Hub proxies ASP.NET SignalR had, where you can call server methods with syntax like this:

connection.Server.MyHubMethod(...)

Instead of

connection.InvokeAsync("MyHubMethod", ...)

Is that correct @Daniel15?

If so, I think we could repurpose this issue to track doing this across all the clients and put it on the backlog. It's not something we have in our current plans though.

Daniel15 commented 4 years ago

I never actually used ASP.NET SignalR, but yeah something like that, assuming the MyHubMethod is strongly-typed.

I did recently discover the Reinforced.Typings project which allows autogeneration of TypeScript interfaces based on C# classes, so I can at least get some typing for the C# models I'm using.

analogrelay commented 4 years ago

I never actually used ASP.NET SignalR, but yeah something like that, assuming the MyHubMethod is strongly-typed.

In ASP.NET SignalR we used dynamic to do it 😨. But we wouldn't do that again :). We'd do something like you proposed which would require a strongly-typed interface of some kind.

Do you mind if I edit your issue a little bit to be more general and then we can use this issue to track that larger work item?

Daniel15 commented 4 years ago

Sure, feel free to edit it :)

analogrelay commented 4 years ago

I did find https://github.com/aspnet/AspNetCore/issues/5276 which we closed due to low enthusiasm, but there is new enthusiasm now :). I'll make the edits and we'll put this on the backlog to consider.

fuchen commented 4 years ago

Definitely need this!

// server side hub
public class MathHub: Hub
{
    public int Add(int a, int b)
    {
        return a + b;
    }
}

Generate below typescript interface:

interface MathHub {
    invoke: {
        Add(a: number, b: number): Promise<number>
    }
    send: {
        Add(a: number, b: number): void
    }
}

// call and wait for return value
const result = await hub.invoke.Add(1, 2) 

// or without return value
hub.send.Add(1, 2) // without return value
iXyles commented 4 years ago

I would say this would be really nice to have, since it helps to prevent human error and annoying debugging session. Also makes developing pretty smooth on both ends by getting notice if you change this you will also have to update this (by help of compiler error as an example).

fredkuhne commented 4 years ago

Yes, please.

jannikbuschke commented 4 years ago

+1

Thorium commented 4 years ago

I hope the non-core functionality of EnableJavaScriptProxies parameter (generating the javascript file with createHubProxies and registerHubProxies) will be included to AspNetCore. Existing projects would like to migrate to dotnet, but without rewriting all the JavaScript calls.

jannikbuschke commented 4 years ago

Does anyone know of any good resources regarding code generation in dotnet core? I always had problem to understand the msbuild system, and how to hook into each build and render some data to some files. The new code generation capabilities that come to dotnet, seem to only alter the generated dlls.

// update: I usually have monorepos with server and client code side-by-side. It would be nice to autogenerate clientside code on every build step.

oising commented 4 years ago

It's been a few years since I've used a signalr .net C# client, and I was surprised to find that it's still all strings and On() buffoonery. I'd love to see a strongly typed client view of the server hub. The symmetry is broken -- we cannot have this! :)

Thorium commented 4 years ago

Is seems like .NET Core strongly-typed SignalR hub proxies are supported by Fable which translates F# to JavaScript: https://github.com/Shmew/Fable.SignalR

JasonEdb commented 4 years ago

I had a stab at this with Castle.DynamicProxy to get the discussion started.

Called like this:

using Microsoft.AspNetCore.SignalR.Client;
using System;
using System.Threading.Tasks;
using SignalR.TypedClient;
using TestCommon;

namespace TestClient
{
    class Program
    {
        static async Task Main(string[] args)
        {
            var builder = new HubConnectionBuilder();
            await Task.Delay(TimeSpan.FromSeconds(3));
            var hub = builder
                .WithUrl("http://localhost:5000/echo")
                .WithAutomaticReconnect()
                .Build<IEchoHub>();

            var unregister = hub.RegisterCallbacks(new MyClientCallbacks());
            await hub.StartAsync();
            var message = await hub.Invoke.Echo("Some message");
            Console.WriteLine($"Result: {message}");

        }
    }

    public class MyClientCallbacks : IEchoHubClient
    {
        public Task OnMessageReceived(string message)
        {
            Console.WriteLine($"{nameof(OnMessageReceived)} - {message}");
            return Task.CompletedTask;
        }
    }
}

I noticed that there are a couple of issues with this approach;

Of course, if this were the taken approach, it would need some more attention, hub attributes would need to be respected and the dependency on Castle.Core would need to be removed.

davidfowl commented 4 years ago

We'd likely be looking an approach that uses source generators.

JasonEdb commented 4 years ago

@davidfowl Yes that absolutely makes sense. That would eliminate all of the shortcomings of this approach.

Thorium commented 3 years ago

My interest is mainly JavaScript proxies: The main motivation to update existing .NET Framework application to .NET Core would be the speed-gain of Kestrel web-server. However, with ASP.NET Core SignalR, without strongly typed proxies:

So, for now, it's not worth of migrating exiting application using SignalR to .NET Core.

davidfowl commented 3 years ago

ASP.NET Core SignalR isn't just more performant, it's also more reliable.

ryandle commented 3 years ago

I found myself immediately wanting this in my Blazor WASM project!

Stirda commented 3 years ago

@ryandle I hope SignalR.Client.TypedHubProxy library will port to ASP.NET Core soon. I want to believe that this will open doors to Blazor WASM.

mehmetakbulut commented 3 years ago

@Stirda @ryandle Over the years I wrote some pieces to handle this issue but just last weekend decided to put them together as a library. https://github.com/mehmetakbulut/SignalR.Strong Comes with two options for client-to-server calls: dynamic proxy (hub.Do()) and expression proxy (ehub.SendAsync(hub => hub.Do())). I don't have a WASM case to test it on but if you try the 0.2 preview from nuget and find issues, I'd be happy to take a peek.

I might investigate a third option (source generators) which I could see getting contributed to dotnet/aspnetcore eventually.

mehmetakbulut commented 3 years ago

Had some free time so I prototyped the source generator option. Since generator has access to same code analyzer APIs, it could be improved to give compile time errors for hub interfaces that violate SignalR constraints. Overall looks promising but I'm not sure if any pre-.NET 5 platform can utilize it.

Thorium commented 3 years ago

This is a way to use Fable (client side compiler of F# to Javascript) and then interop your JS files with your Fable files: https://hashset.dev/article/14_improving_real_time_communication_using_fable_signal_r

ghost commented 3 years ago

Thanks for contacting us. We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. Because it's not immediately obvious that this is a bug in our framework, we would like to keep this around to collect more feedback, which can later help us determine the impact of it. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

davidfowl commented 3 years ago

@BrennanConroy and @halter73 @mehmetakbulut's package might be a great place to start exploring this feature. Also @mehmetakbulut would you be willing to help design and contribute something based on the work you've done? It looks like you've basically explored all the options.

mehmetakbulut commented 3 years ago

I'd be happy to. Is there other work, concerns or timeline on you guys' end I should be aware of?

I expect that the source generator solution is probably what would be of interest.

BrennanConroy commented 3 years ago

Is there other work, concerns or timeline on you guys' end I should be aware of?

If we want something for the .NET 6 release it should probably be done by mid July, we might be able to push it to mid August especially if it's a new package. Otherwise, there are no concerns.

Some things off the top of my head we'll need to think about:

Thorium commented 3 years ago

Generate from server-side hub class? Generate from client-side interface

I'd like to see this feature being usable from non-.NET-client (like TypeScript). The current non-core implementation is just an exe-file that parses the source-code hub-class attributes, and dumps the client in wanted language. Which is enough I think.

BrennanConroy commented 3 years ago

I'd like to see this feature being usable from non-.NET-client (like TypeScript).

Noted, we'll probably start with the .NET client and figure out what sort of pattern and API we want. Then we can start looking at other clients.

mehmetakbulut commented 3 years ago

Generate from server-side hub class vs Generate from client-side interface

My current approach generates on client-side. The interfaces live in a common project so the SignalR hubs implement them as well. I like this better than generating from a hub class because the interface can be defined independently and consumers don't have to take a direct dependency on anything strictly tied to a particular server or client implementation.

CancellationToken on the client side calls, should it just be CancellationToken = default or generate 2 methods each?

I'd say this is tied to whether we return the same interface supplied by the user (e.g. TIntf AsStronglyTypedHub<TIntf>(this HubConnection conn)) or an extended type (e.g. ExtendedTIntf AsStronglyTypedHub<TIntf>(this HubConnection conn)). If both client and server share an interface and we return that interface, then we should respect the interface as defined.

Given an interface:

    public interface IMockHub
    {
        Task<ChannelReader<int>> StreamToClientViaChannel(List<int> a);

        Task<ChannelReader<int>> StreamToClientViaChannelWithToken(List<int> a, CancellationToken cancellationToken);
    }

I currently generate:

    public sealed class GeneratedIMockHub : IMockHub
    {
        private readonly Microsoft.AspNetCore.SignalR.Client.HubConnection conn;
        public GeneratedIMockHub(Microsoft.AspNetCore.SignalR.Client.HubConnection connection)
        {
            this.conn = connection;
        }

        public System.Threading.Tasks.Task<System.Threading.Channels.ChannelReader<int>> StreamToClientViaChannel(System.Collections.Generic.List<int> a)
        {
            return this.conn.StreamAsChannelAsync<int>("StreamToClientViaChannel", a);
        }

        public System.Threading.Tasks.Task<System.Threading.Channels.ChannelReader<int>> StreamToClientViaChannelWithToken(System.Collections.Generic.List<int> a, System.Threading.CancellationToken cancellationToken)
        {
            return this.conn.StreamAsChannelAsync<int>("StreamToClientViaChannelWithToken", a, cancellationToken);
        }
    }

As such generating alternate methods is not too hard. However I currently return the same interface provided by the user so such alternate methods couldn't be exposed to the user. I think this makes it simpler for the user conceptually since they don't need to know about a custom type generated by us.

If we are generating from a hub class or returning an extended type, then yes we can take some creative freedoms..

On handlers

Handlers don't require anything beyond simple reflection to register callbacks with the HubConnection object. An interface is not even necessary. Though without a shared interface, you risk registering callbacks that you may not have intended to be callbacks or mismatch what the server expects due to a typo.

Should there be an interface for these and then if you want to provide an implementation for a specific one you can?

My current solution allows you to provide just an implementation (it is then assumed that all public methods are callbacks) or an interface plus an implementation (subset defined by the interface are callbacks). I think this works well because the client could independently define an interface (if one isn't supplied by server), capturing only the callbacks they care about. I called these handlers "Spokes" (spoke and hub??), in hindsight I admit it wasn't the best name.. The most verbose signature I've is: SpokeRegistration RegisterSpoke<TSpokeIntf, TSpokeImpl>(this HubConnection conn, TSpokeImpl spoke) where SpokeRegistration is an IDisposable so the callback registrations can be undone later.

Should you just register a whole implementation class for all callbacks?

I think there is value in being able to register multiple implementations so different classes could capture different pieces. This does open the door to overlapping surfaces but could potentially be detected by analyzers.

Should the API be part of the builder pattern or something you do after HubConnectionBuilder.Build

I haven't taken much look in the builder code but if the underlying HubConnection is exposed, then it would be easy to do it from builder as well. I originally wrote my own builder but later changed approach to providing extensions methods on HubConnection (for both acquiring a strongly typed hub proxy as well as registering handlers) because I figured (1) that would be the simplest approach for a third party package and (2) it still allowed the underlying HubConnection to be freely used by the user where the library didn't cover all cases and didn't need to worry about how the user got the HubConnection.

mehmetakbulut commented 3 years ago

@davidfowl @BrennanConroy @halter73 Took me a while to get back to this but I've opened up a design proposal capturing some of my own experience as well as questions in this ticket. I think it should be a good starting point but heavy scrutiny is needed to hammer out a design and implementation. https://github.com/dotnet/aspnetcore/issues/32534

BrennanConroy commented 3 years ago

Backlogging this issue, the C# work is being worked on for 6.0 in https://github.com/dotnet/aspnetcore/issues/32534

Other client(s) will be considered in the future.

ghost commented 3 years ago

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

curt-w commented 2 years ago

I'm also looking for this, implementation on Blazor / using the .NET client.