Skip to content
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

Add support for DI in Hub methods #34047

Merged
merged 11 commits into from
Jan 26, 2022
Merged

Add support for DI in Hub methods #34047

merged 11 commits into from
Jan 26, 2022

Conversation

BrennanConroy
Copy link
Member

@BrennanConroy BrennanConroy commented Jul 2, 2021

Fixes #16686

The issue talks about having a [FromServices] attribute, that isn't addressed in the current form of this PR.
Using IFromServiceMetadata like we do for Minimal APIs to check if a parameter should be resolved from a service. This makes the feature opt-in and less likely for us to accidentally mess something up.

Source generator wont be able to do anything special about hub methods with services, but that might just be something we don't care about and developers should design their interfaces with that in mind.

Currently using a mask (ulong) to store indices for service positions in Hub methods which limits hub methods to 64 arguments, I think this is fine though 😄

API

public class HubOptions
{
+    public bool DisableImplicitFromServiceParameters { get; set; }
}

@ghost ghost added the area-signalr Includes: SignalR clients and servers label Jul 2, 2021
@davidfowl
Copy link
Member

Currently using a mask (int) to store indices for service positions in Hub methods which limits hub methods to 32 arguments, I think this is fine though 😄

This is jank and it should work for any number of arguments even though 32 is insane. 64? 😄

@BrennanConroy
Copy link
Member Author

This is jank and it should work for any number of arguments even though 32 is insane. 64?

Changing the field to long would allow 64, I'd prefer not going bigger because then we'll need to allocate/lazy-alloc and have another field.

@halter73
Copy link
Member

halter73 commented Jan 7, 2022

This is jank and it should work for any number of arguments even though 32 is insane. 64?

Changing the field to long would allow 64, I'd prefer not going bigger because then we'll need to allocate/lazy-alloc and have another field.

Sounds reasonable. I don't think allocating a bool[] once per hub method would be that bad, but it also doesn't feel that valuable to support a service argument after the 64th position. The startup error message is clear enough.

/// <remarks>
/// Disabled by default.
/// </remarks>
public bool EnableInferredFromServiceParameters { get; set; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should infer service parameters by default. I'd be tempted to remove this option altogether. How many service types are also serializable? I see wanting an option to opt-out though since there isn't another escape hatch I can see if you happen to have an argument type registered as a service.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see wanting an option to opt-out though since there isn't another escape hatch I can see if you happen to have an argument type registered as a service.

This is my main concern, which is why I switched the whole PR to using IFromServiceMetadata and then having an opt-in for inferring.

Copy link
Member

@davidfowl davidfowl Jan 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets do an opt-out then.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to leave as opt-in for now, we can discuss in chat and/or API review

@BrennanConroy BrennanConroy added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Jan 18, 2022
@ghost
Copy link

ghost commented Jan 18, 2022

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.

@BrennanConroy BrennanConroy marked this pull request as ready for review January 18, 2022 17:18
@ghost ghost removed the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Jan 18, 2022
@mkArtakMSFT mkArtakMSFT added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Jan 23, 2022
@ghost
Copy link

ghost commented Jan 23, 2022

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.

@ghost ghost removed the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Jan 23, 2022
@BrennanConroy
Copy link
Member Author

@davidfowl @halter73 This should be ready for final review, updated to be opt out and use the API approved name.

Co-authored-by: Stephen Halter <halter73@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-signalr Includes: SignalR clients and servers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inject dependencies into SignalR hub method
4 participants