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

Pull IServiceScopeFactory from context.RequestServices #796

Closed
Shane32 opened this issue May 20, 2022 · 4 comments
Closed

Pull IServiceScopeFactory from context.RequestServices #796

Shane32 opened this issue May 20, 2022 · 4 comments
Labels
wontfix This will not be worked on

Comments

@Shane32
Copy link
Member

Shane32 commented May 20, 2022

I mean only that specifically this dependency is somewhat different from the rest in the sense that it is not mandatory and it may not be obtained at all. I understand that one can pull all dependencies from HttpContext.RequestServices. I do not call for this - _documentExecuter and _serializer should be taken as dependencies. I'm saying more about not to consider any API for working with scopes as dependency. It's internal infrastructure code.

Originally posted by @sungam3r in #774 (comment)

@Shane32
Copy link
Member Author

Shane32 commented May 20, 2022

I was working on a PR for this just now -- but the WebSocket handler (which will be posted shortly) requires the service scope factory and cannot execute requests without it. So it may not make sense to remove it. In any case, it will be better to review the required changes after the WebSocket handling code is merged in rather than beforehand.

@sungam3r
Copy link
Member

ok

@Shane32
Copy link
Member Author

Shane32 commented Jul 13, 2022

@sungam3r I reviewed this again. IServiceScopeFactory is required when executing batch requests and for any WebSocket connections. I'm not sure what change would be worth making here. Do you want to review this again? Let's discuss or close the issue.

@sungam3r
Copy link
Member

Since IServiceScopeFactory is required for both Subscription servers I'm fine to keep this dependency as is for GraphQLHttpMiddleware.

@sungam3r sungam3r added the wontfix This will not be worked on label Jul 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

2 participants