Register built-in filter by default #287
Conversation
|
Should be targeting main branch. |
|
I don't expect any issues, but can you confirm no issues occur if someone manually adds these with
|
| @@ -58,6 +58,12 @@ public static IFeatureManagementBuilder AddFeatureManagement(this IServiceCollec | |||
|
|
|||
| services.TryAddScoped<IVariantFeatureManagerSnapshot>(sp => sp.GetRequiredService<FeatureManagerSnapshot>()); | |||
|
|
|||
| services.AddSingleton<PercentageFilter>(); | |||
There was a problem hiding this comment.
Probably a good idea to call our AddFeatureFilter overload. It only adds the filter if it hasn't already been added.
There was a problem hiding this comment.
Oh, sorry. This is my fault. I found that services.AddSingleton<PercentageFilter>(); is completly wrong. Because the feature filter will be injected as IFeatureFilterMetadata. Besides, AddFeatureFilter() will also check whether the feature filter only implement exactly one of IFeatureFilter and IContextualFeatureFilter.
There was a problem hiding this comment.
All previous testcases called AddFeatureFilter() by themselves. I have not written any new testcases.
There was a problem hiding this comment.
Sounds like it would be worth to have a test that would've caught the issue.
| @@ -58,6 +58,12 @@ public static IFeatureManagementBuilder AddFeatureManagement(this IServiceCollec | |||
|
|
|||
| services.TryAddScoped<IVariantFeatureManagerSnapshot>(sp => sp.GetRequiredService<FeatureManagerSnapshot>()); | |||
|
|
|||
| services.AddSingleton<PercentageFilter>(); | |||
There was a problem hiding this comment.
We probably shouldn't add PercentageFilter. It was created to showcase how to write a feature filter. We may want to drop it one day.
There was a problem hiding this comment.
Discussed with @zhenlan offline. My concern is that if we have the filter in the library, it makes sense to add it by default. If we want to exclude it by default, then most likely we should just remove it from the library as well.
Given that we do have current usage of this filter, it doesn't make sense to remove it at this point. We agreed that we should add the percentage filter by default.
|
@jimmyca15 |
zhiyuanliang-ms
force-pushed
the
zhiyuanliang/register-builtin-filter
branch
from
6859335
to
089b4ce
Compare
| /// </summary> | ||
| /// <typeparam name="T">An implementation of <see cref="ITargetingContextAccessor"/></typeparam> | ||
| /// <returns>The feature management builder.</returns> | ||
| IFeatureManagementBuilder WithTargeting<T>() where T : ITargetingContextAccessor; |
There was a problem hiding this comment.
Adding a method on an interface is a breaking change. This should be an extension method. I believe we already have IFeatureManagementBuilderExtensions
Mitigate issue #248