Skip to content

[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

Merged
merged 1 commit into from
Aug 3, 2023

Conversation

javiercn
Copy link
Member

@javiercn javiercn commented Jul 28, 2023

Fixes #49300

@ghost ghost added the area-blazor Includes: Blazor, Razor Components label Jul 28, 2023
@javiercn javiercn force-pushed the javiercn/support-configuring-hub-options branch from 93f6bd7 to 33c58b8 Compare August 2, 2023 13:27
@javiercn javiercn marked this pull request as ready for review August 2, 2023 13:43
@javiercn javiercn requested a review from a team as a code owner August 2, 2023 13:43
@DamianEdwards
Copy link
Member

DamianEdwards commented Aug 2, 2023

How exactly does this enable configuring the hub options? Are there existing extension methods on IServerSideBlazorBuilder that enable that so that now it's returned from AddServerComponents() they can be called?

@javiercn
Copy link
Member Author

javiercn commented Aug 3, 2023

@DamianEdwards yes

{
/// <summary>
/// Gets the <see cref="IServiceCollection"/>.
/// </summary>
IServiceCollection Services { get; }
public new IServiceCollection Services { get; }
Copy link
Member

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?

Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member Author

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.

@SteveSandersonMS SteveSandersonMS self-requested a review August 3, 2023 14:17
Copy link
Member

@SteveSandersonMS SteveSandersonMS left a 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.

@javiercn javiercn merged commit 771532a into main Aug 3, 2023
@javiercn javiercn deleted the javiercn/support-configuring-hub-options branch August 3, 2023 16:55
@ghost ghost added this to the 8.0-rc1 milestone Aug 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't configure HubOptions when calling IRazorComponentBuilder.AddServerComponents()
3 participants