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

Inject dependencies into SignalR hub method #16686

Closed
Daniel15 opened this issue Oct 31, 2019 · 16 comments · Fixed by #34047
Closed

Inject dependencies into SignalR hub method #16686

Daniel15 opened this issue Oct 31, 2019 · 16 comments · Fixed by #34047
Labels
affected-few This issue impacts only small number of customers api-approved API was approved in API review, it can be implemented area-signalr Includes: SignalR clients and servers Docs This issue tracks updating documentation enhancement This issue represents an ask for new feature or an enhancement to an existing one severity-minor This label is used by an internal tool
Milestone

Comments

@Daniel15
Copy link
Contributor

Daniel15 commented Oct 31, 2019

Is there a way to inject dependencies into a SignalR hub method, similar to how we can use FromServicesAttribute with ASP.NET MVC to inject a service into an action method? I tried just listing the dependency in the method signature but got an error "Failed to invoke foo due to an error on the server"

API

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

Alternative names

EnableImplicitFromServiceParameters
InferFromServiceParameters
InferParametersFromService(s)

Additional thoughts

Instead of opt-in, make the option an opt-out and attempt to detect services by default. (This would likely require a breaking change announcement)

@davidfowl
Copy link
Member

No there isn't. It's something we could look at in the future though.

@davidfowl davidfowl added the area-signalr Includes: SignalR clients and servers label Oct 31, 2019
@jkotalik jkotalik added this to the Backlog milestone Oct 31, 2019
@halter73
Copy link
Member

FWIW, the lifetime of the hub is typically only the duration of a Hub method invocation. I think the same is true for Controllers and Actions for that matter. So ctor injection might not look as nice, it should work more or less the same.

@Daniel15
Copy link
Contributor Author

Daniel15 commented Oct 31, 2019

The issue is if each hub method requires different dependencies, you end up with a giant constructor, and may construct dependencies that aren't actually needed by the method that's being called. :)

I haven't tested but I guess injecting the IServiceProvider into the constructor would work, then I could use that or ActivatorUtilities in the hub method.

@halter73
Copy link
Member

I haven't tested but I guess injecting the IServiceProvider into the constructor would work, then I could use that or ActivatorUtilities in the hub method.

That should definitely work, but I understand that's far from ideal.

@erikmf12
Copy link

@Daniel15 - I know this doesn't solve your problem, but it might help to use partial classes for your hub class. You could have your constructor in a MyHub.Constructor.cs and all other methods in a different MyHub.ClientMethods.cs.

@BrennanConroy BrennanConroy added affected-few This issue impacts only small number of customers enhancement This issue represents an ask for new feature or an enhancement to an existing one severity-minor This label is used by an internal tool labels Nov 9, 2020 — with ASP.NET Core Issue Ranking
@davidfowl
Copy link
Member

Maybe we can support this better now that we have synthetic arguments (like cancellation tokens).

@ghost
Copy link

ghost commented Apr 6, 2021

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
Copy link
Member

We need to define a [FromServices] attribute lower in the stack that can be used by SignalR. The one in MVC is well tied to MVC.

@BrennanConroy
Copy link
Member

api-suggestion?

@davidfowl davidfowl added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Apr 6, 2021
@halter73
Copy link
Member

halter73 commented Apr 6, 2021

I hate having multiple [FromServices] attributes. Type forwarding and still needing using Microsoft.AspNetCore.Mvc; doesn't seem great either though.

I wonder if we could infer the parameter is a service without an attribute? Even if we could, we'd probably still want an attribute. 😢

@BrennanConroy
Copy link
Member

We also need a way to allow opting out of services per parameter or per method.
e.g. [FromClient], [FromInvocation], [FromCall]

Or, only DI inject if [FromServices] is on the parameter.

@ghost
Copy link

ghost commented Jul 29, 2021

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.

@BrennanConroy BrennanConroy added api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jan 18, 2022
@ghost
Copy link

ghost commented Jan 18, 2022

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@pranavkm
Copy link
Contributor

API review:

API approved. We spoke about this and think this should be an opt-out feature. We discussed the name and think DisableImplicitFromServiceParameters (using implicit over inferred) is better.

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

@BrennanConroy
Copy link
Member

Was the breaking change nature discussed? We probably need to make an announcement if we make it opt-out.

@halter73
Copy link
Member

I think I mentioned how unlikely I feel this was to break people 😆 Making an announcement is a good idea though.

@BrennanConroy BrennanConroy modified the milestones: Backlog, 7.0-preview2 Jan 26, 2022
@BrennanConroy BrennanConroy added Docs This issue tracks updating documentation api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Feb 3, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Mar 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affected-few This issue impacts only small number of customers api-approved API was approved in API review, it can be implemented area-signalr Includes: SignalR clients and servers Docs This issue tracks updating documentation enhancement This issue represents an ask for new feature or an enhancement to an existing one severity-minor This label is used by an internal tool
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants