Skip to content

AddServerSizeBlazor should return a builder #9316

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
davidfowl opened this issue Apr 12, 2019 · 12 comments
Closed

AddServerSizeBlazor should return a builder #9316

davidfowl opened this issue Apr 12, 2019 · 12 comments
Assignees
Labels
area-blazor Includes: Blazor, Razor Components Done This issue has been fixed enhancement This issue represents an ask for new feature or an enhancement to an existing one

Comments

@davidfowl
Copy link
Member

This will make it more seamless to wire up scale out providers and other potential SignalR options. Today to wire up the service in preview 4 it looks like this:

services.AddServerSideBlazor().AddSignalR().AddAzureSignalR();

Alternatively, if we don't want to expose the ISignalRBuilder then a more targeted builder might be useful if we want to add support for an AddAzureSignalR extension method anyways.

cc @rynowak @javiercn @SteveSandersonMS @danroth27 @bradygaster

@javiercn
Copy link
Member

I think we want components to be and feel like something on their own, with SignalR being an "implementation detail", so that would clash a bit with that proposal.

If that is not the case anymore, then a separate builder would just do the trick. We can just return a custom builder and make it implement ISignalRBuilder so that those methods are available.

At that point, we can also just make MapBlazorHub() -> MapHub<BlazorHub>(BlazorHub.DefaultPath)

@davidfowl
Copy link
Member Author

Do we like either of these?

services.AddServerSideBlazor().AddAzureSignalR();
services.AddServerSideBlazor();
services.AddSignalR().AddAzureSignalR();

@javiercn
Copy link
Member

I'm fine with both, but I like the second one more. I'm not a fan of wrapping these types of methods because it breaks composition and then:

  • You don't get things added for free when the underlying abstraction adds new options.
  • You are forced to expose options for all the things you combine.
  • You get to duplicate all things you want to expose.
  • When we version/update things this becomes a mess.

I prefer to strive for getting the design to a place where it can compose well with the rest of the pipeline, that way there is no duplication and things can evolve independently.

As an example of when this goes bad, just look at our past experience with Identity. We've had to tweak things on each minor release.

@davidfowl
Copy link
Member Author

You can achieve all of those things you mention if you expose ISignalRBuilder or a type that implements that.

@Eilon Eilon added the area-signalr Includes: SignalR clients and servers label Apr 12, 2019
@analogrelay analogrelay added area-signalr Includes: SignalR clients and servers and removed area-signalr Includes: SignalR clients and servers labels Apr 12, 2019
@analogrelay
Copy link
Contributor

SignalR being an "implementation detail"

I don't think we should hide SignalR as an implementation detail when using Server-Side Blazor. SignalR dramatically changes how you scale your app, beyond just "recommending" Azure SignalR. I don't want users of Server-Side Blazor to be in the dark about that.

There's a purist part of me that kinda wants to force users (through marker service detection) to have both .AddServerSideBlazor and .AddSignalR (and have Server-Side Blazor throw an error, like MVC does, if .AddSignalR wasn't called). Now, I don't think that's the best customer experience though, so I can tell that purist side to sit down and shut up.

What about an approach that makes SignalR configuration a first-class part of .AddServerSideBlazor but doesn't require a single builder:

services.AddServerSideBlazor()
    .ConfigureSignalR(sR => sR.AddAzureSignalR());

That puts "SignalR" right in the configuration so that it's not hidden, but it also allows Server-Side Blazor to be abstracted a little from SignalR.

If that feels messy, I think I most prefer having the Blazor builder implement/wrap/whatever ISignalRBuilder so that the native configuration shows up.

@javiercn
Copy link
Member

I would be fine with option2 of what fowler suggested and also with getting rid of MapBlazorHub in favor of MapHub. It's other people the ones that need convincing. I love that its trivial to plug in server-side blazor and that we shouldn't hide signalr at all.

@davidfowl davidfowl added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates area-blazor Includes: Blazor, Razor Components and removed area-signalr Includes: SignalR clients and servers labels Apr 12, 2019
@rynowak
Copy link
Member

rynowak commented Apr 13, 2019

I kinda wonder if it's makes sense to lean into SignalR + Blazor branding more. We're now using the term Blazor for both models. Does it make sense to say Server Side Blazor or SignalR Blazor or BlazR or even BlzR

This is all to do with branding.

Right now I prefer to see two lines of code because we haven't tried this pattern anywhere else. Need to do some thinking about this.

@analogrelay
Copy link
Contributor

SignalR Blazor

Ok, so maybe it's because I've spent a lot of time working on it and have a fondness for it, but that really clicks for me.

ideas are happening

@SteveSandersonMS
Copy link
Member

SteveSandersonMS commented Apr 13, 2019 via email

@davidfowl
Copy link
Member Author

Here's another example of whart happens when you hide the hub but don't expose a way to configure it:
#9570

@javiercn
Copy link
Member

I don't think we should hide the hub, endpoints.MapHub<BlazorHub>(BlazorHub.DefaultPath) looks clean enough IMO to be on the templates and discovery is not a problem given that its already on the initial template.

Thats my take, based on the fact that the same thing happens with Identity all the time.

@rynowak
Copy link
Member

rynowak commented Apr 27, 2019

Discussed this in person with @davidfowl and we came up with the following plan.

This is a balance of:

  • Having the correct branding
  • Not leaking real implementation details
  • Not hiding SignalR

note: The fact that we use SignalR isn't an implementation detail. Users need to know about it and care about it. However the fact that we use a type named BlazorHub is an implementation detail and we intend to hide it and make it non-public.

ConfigureServices

We will keep AddServerSideBlazor() and make it return it's own builder type. We'll accept callbacks of HubOptions so you can configure the hub. Configuring options specific to the hub we use is valuable.

We won't return the ISignalRServerBuilder. The other methods supported on that type are GLOBAL for all SignalR usage. We don't want to give the impression that those options only affect Blazor. Those other options are also things like adding protocols or redis scaleout. It's important that users understand SignalR to do that.

The AddAzureSignalR() gesture is the most important one, and it's going away.

Examples:

Vanilla

services.AddServerSideBlazor();

With Hub Options

services.AddServerSideBlazor(options =>
{
    options.MaximumReceiveMessageSize = long.MaxValue; // #YOLO
});

With Redis

services.AddServerSideBlazor(options =>
{
    options.MaximumReceiveMessageSize = long.MaxValue; // #YOLO
});
services.AddSignalR().AddStackExchangeRedis();

Configure

We will add a marker interface to the convention builder returned by MapHub<>. SignalR and Server-Side-Blazor will each define it's own concrete type that applies this marker interface. This allows us to define unique functionality via extensions for vanilla-hubs, all signalr, and the blazor hub.

app.UseEndpoints(endpoints =>
{
    endpoints.MapHub<ChatHub>("/foo").DoABarrelRoll();
    endpoints.MapServerSideBlazor().AddComponent<App>("app");
});

@mkArtakMSFT mkArtakMSFT added area-blazor Includes: Blazor, Razor Components and removed area-blazor Includes: Blazor, Razor Components labels May 1, 2019
rynowak added a commit that referenced this issue May 6, 2019
rynowak added a commit that referenced this issue May 7, 2019
rynowak added a commit that referenced this issue May 8, 2019
@halter73 halter73 changed the title AddServerSizeBlazor should return a builder that derives from ISignalRBuilder AddServerSizeBlazor should return a builder May 8, 2019
@rynowak rynowak closed this as completed in b383695 May 8, 2019
@rynowak rynowak added Done This issue has been fixed and removed 2 - Working labels May 8, 2019
@mkArtakMSFT mkArtakMSFT removed area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels May 9, 2019
@danroth27 danroth27 added the enhancement This issue represents an ask for new feature or an enhancement to an existing one label May 31, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Dec 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Includes: Blazor, Razor Components Done This issue has been fixed enhancement This issue represents an ask for new feature or an enhancement to an existing one
Projects
None yet
Development

No branches or pull requests

8 participants