Support keyed service for variant service provider - #606
Conversation
|
|
||
| private TService ResolveVariantService(string variantName) | ||
| { | ||
| if (_serviceProvider is IKeyedServiceProvider) |
There was a problem hiding this comment.
it's behavioural BC. if there is keyed di compatible user he'll need to change his whole DI setup after update
There was a problem hiding this comment.
i think you forgot that useKeyedService option
There was a problem hiding this comment.
Yes. I discussed with Jimmy Campbell (@jimmyca15). The discussion result is to not have useKeyedService toggle.
it's behavioural BC. if there is keyed di compatible user he'll need to change his whole DI setup after update
This is not a breaking change, because we have fallback logic, it will first try to get keyed service and if there is no keyed service match the variant name, it will still use the previous way (GetRequiredService<IEnumerable>()) to get all implementation and inspect them one by one.
There was a problem hiding this comment.
This disrupts backward compatibility.
Let's say I am net8 variant sp user. Till now my services were registered like this:
services.AddSingleton<IFeature, Variant1>();
services.AddSingleton<IFeature, Variant2>();After I get the update my setup won't work as variant sp will use keyed di because it's available by default in net8. This will make me change DI setup to keyed just to have it working.
Am I wrong in BC definition?
There was a problem hiding this comment.
I just realized i read code wrong, sorry for misunderstanding
There was a problem hiding this comment.
No worries. How do you like the current implementation? :)
There was a problem hiding this comment.
Honestly, good enough to me! I think it's great you guys found the right spot to have it both: keyed di and fallback.
The thing i'd like to improve is code readability. IMHO VariantServiceProvider's complexity is increasing now so it's crucial to be able mantaining it well. You saw how i got it wrong first time reading :)
So i updated #605 to give my small ideas on how to do it
There was a problem hiding this comment.
Updated my PR and also includes the change to support "feature status service". Check the updated PR description please.
There was a problem hiding this comment.
Hey, Степан (@Stepami)
I reverted the change of adding VariantServiceProviderOptions. Whether to add a new API WithFeaturedService is still not decided (our internal dicussion is still not finalized). This PR will only be scoped at supporting keyed service.
The thing i'd like to improve is code readability. IMHO VariantServiceProvider's complexity is increasing now so it's crucial to be able mantaining it well. You saw how i got it wrong first time reading :)
I added some comments for better readability.
3 participants
Why this PR?
#605 #564
VariantServiceProvider<TService>previously took IEnumerable, which forced the DI container to instantiate every registered implementation ofTServiceat construction time — even variants that would never be selected. This PR changes it to take the IServiceProvider directly and, at runtime, checks whether a keyed service can be retrieved; if so, lazy on-demand instantiation is achieved.Thanks Степан (@Stepami) for bringing the keyed service implementation to us, and for clarifying and addressing our compatibility concerns.