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.

Sign up for GitHub

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

Jump to bottom

Add support for IServiceProviderIsService #54047

Merged
merged 6 commits into from
Jun 11, 2021
Merged

Add support for IServiceProviderIsService #54047

merged 6 commits into from
Jun 11, 2021

Conversation

davidfowl
Copy link
Member

  • This optional service lets consumers query to see if a service is resolvable without side effects (not having to explicitly resolve the service).
  • Added new spec tests to verify the baseline behavior based on IServiceCollection features.

Fixes #53919

@davidfowl
Add support for IServiceProviderIsService
754dd34 
- This optional service lets consumers query to see if a service is resolvable without side effects (not having to explicitly resolve the service).
- Added new spec tests to verify the baseline behavior based on IServiceCollection features.
@davidfowl davidfowl requested a review from pakrym June 11, 2021 05:08
@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented Jun 11, 2021

Tagging subscribers to this area: @eerhardt, @maryamariyan
See info in area-owners.md if you want to be subscribed.

Issue Details
  • This optional service lets consumers query to see if a service is resolvable without side effects (not having to explicitly resolve the service).
  • Added new spec tests to verify the baseline behavior based on IServiceCollection features.

Fixes #53919

Author: davidfowl
Assignees: -
Labels:

area-Extensions-DependencyInjection, new-api-needs-documentation
Milestone: -

@davidfowl @tillig
Update src/libraries/Microsoft.Extensions.DependencyInjection.Abstrac…
b556a0e 
…tions/src/IServiceProviderIsService.cs

Co-authored-by: Travis Illig <tillig@paraesthesia.com>
@pakrym
Copy link
Contributor

pakrym commented Jun 11, 2021 •
edited
Loading

Can you put Debug.Assert(CanGetService(type)) into GetService implementation and run the test suit with that? Make sure we didn't miss anything. And maybe keep it long-term.

@davidfowl
Handle built in services as part of IsServce
6aeff37 
- Special case built in services as part of the IsService check
- Added a test case
halter73
halter73 approved these changes Jun 11, 2021
View reviewed changes
Copy link
Member

@halter73 halter73 left a comment

Choose a reason for hiding this comment

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

I think I agree with @pakrym that we should put this in DependencyInjectionSpecificationTests with SupportsIsService true by default for greater discoverability when third-party containers update their Specification.Tests dependency.

@davidfowl
PR feedback
8f7d3ec 
- Make the tests part of the core DI tests and enable skipping via a property
@davidfowl
Copy link
Member Author

I think I agree with @pakrym that we should put this in DependencyInjectionSpecificationTests with SupportsIsService true by default for greater discoverability when third-party containers update their Specification.Tests dependency.

Done!

@davidfowl
Copy link
Member Author

Errors as unrelated, Merging.

@davidfowl davidfowl merged commit c14ac48 into main Jun 11, 2021
@BrennanConroy BrennanConroy deleted the davidfowl/serviceprovider-isservice branch June 11, 2021 22:39
@ghost ghost locked as resolved and limited conversation to collaborators Jul 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tillig tillig tillig left review comments

@stephentoub stephentoub stephentoub left review comments

@alistairjevans alistairjevans alistairjevans left review comments

@halter73 halter73 halter73 approved these changes

@pakrym pakrym pakrym approved these changes

@maryamariyan maryamariyan maryamariyan approved these changes

@eerhardt eerhardt Awaiting requested review from eerhardt

Assignees
No one assigned
Labels
area-Extensions-DependencyInjection new-api-needs-documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add ability to detect if a service is registered in the DI container
7 participants
@davidfowl @pakrym @halter73 @tillig @stephentoub @maryamariyan @alistairjevans