-
Notifications
You must be signed in to change notification settings - Fork 10.4k
[Blazor] Support configuring hub options #49712
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
Conversation
93f6bd7
to
33c58b8
Compare
How exactly does this enable configuring the hub options? Are there existing extension methods on |
@DamianEdwards yes |
{ | ||
/// <summary> | ||
/// Gets the <see cref="IServiceCollection"/>. | ||
/// </summary> | ||
IServiceCollection Services { get; } | ||
public new IServiceCollection Services { get; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How come this is being made public here and there's an explicit interface implementation below? Wouldn't just one of those be sufficient?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this is to avoid a binary breaking change. I see the base interface has a default implementation of Services
. AFAIK it would be source-compatible just to add the base interface and remove the property from IServerSideBlazorBuilder
entirely, but perhaps that's binary-breaking.
Is that the reason or am I missing some point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an interface, it was already public.
I had to apply new
because by making IServerSideBlazorBuilder
extend IRazorComponentBuilder
it was hiding the member. If I removed the ServiceCollection from here, it was a breaking change. We can take the breaking change if we want to, but that would impact any library that compiled against IBlazorServerBuilder
so it just felt safer to do it this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that the reason or am I missing some point?
We were both typing at the same time. Yes, its to avoid the binary breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I've asked a question to clarify this, I think I have probably guessed the answer and in any case this looks uncontroversial (at least, not dangerous!) so seems very reasonable to just approve as-is.
Fixes #49300