Skip to content

Add support for IServiceProviderIsService#54047

Merged
davidfowl merged 6 commits intomainfrom
davidfowl/serviceprovider-isservice
Jun 11, 2021
Merged

Add support for IServiceProviderIsService#54047
davidfowl merged 6 commits intomainfrom
davidfowl/serviceprovider-isservice

Conversation

@davidfowl
Copy link
Member

@davidfowl davidfowl commented Jun 11, 2021

  • 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
@ghost
Copy link

ghost commented Jun 11, 2021

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

davidfowl commented Jun 11, 2021

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

davidfowl commented Jun 11, 2021

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

@stephentoub stephentoub stephentoub left review comments

@halter73 halter73 halter73 approved these changes

@eerhardt eerhardt Awaiting requested review from eerhardt

+4 more reviewers

@tillig tillig tillig left review comments

@alistairjevans alistairjevans alistairjevans left review comments

@pakrym pakrym pakrym approved these changes

@maryamariyan maryamariyan maryamariyan approved these changes

Reviewers whose approvals may not affect merge requirements

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