Potentially unrequired breaking changes in v3

[brental]

I have been looking at the changes being made in v3 to support dynamic features as I was interested in the concept. While looking at the changes I saw that there have been breaking changes made to several existing interfaces and classes as part of adding dynamic features. However, the changes don't look like they are actually required to support dynamic features and also the updated code does not seem to keep consistency with other existing code or with new code added for dynamic features. The changes I refer to are around changing various parts of code to use FeatureFlag instead of just Feature, such as (it may not be a complete list):

• IFeatureDefinitionProvider - Renamed to IFeatureFlagDefinitionProvider as well as some members renamed:
  • GetFeatureDefinitionAsync - Renamed to GetFeatureFlagDefinitionAsync
  • GetAllFeatureDefinitionsAsync - Renamed to GetAllFeatureFlagDefinitionsAsync
• IFeatureManager / IFeatureManagerSnapshot - Some members renamed:
  • GetFeatureNamesAsync - Renamed to GetFeatureFlagNamesAsync
• FeatureDefinition - Renamed to FeatureFlagDefinition

Are these changes required to support the addition of dynamic features? If not, perhaps consideration should be given to reverting the changes as they will make upgrading to v3 more difficult if people have implemented a custom feature definition provider or are using the GetFeatureNamesAsync method of IFeatureManager / FeatureManagerSnapshot within application code.

As mentioned above, it is also not consistent with other existing code or the new dynamic feature code in terms of naming as there are several cases where the word Flag is not used, some examples being (again, maybe not a complete list):

• FeatureFilterEvaluationContext - Has the following property:
  • FeatureName
• FeatureGatedAsyncActionFilter - Has the following property:
  • FeatureName
• IDynamicFeatureDefinitionProvider - Has the following methods:
  • GetDynamicFeatureDefinitionAsync
  • GetAllDynamicFeatureDefinitionsAsync
• IDynamicFeatureManager / IDynamicFeatureManagerSnapshot - Has the following method:
  • GetDynamicFeatureNamesAsync
• DynamicFeatureDefinition

PR #157 introduced the changes.

[jimmyca15]

Hello @brental

Thank you for the discussion here, these are great points.

In the original release we missed cancellation token support on many async interfaces

• IFeatureManager.IsEnabledAsync
• IFeatureDefinitionProvider
• IFeatureFilter

This is the main wrong that we wanted to right. However, it didn't come without discussion. Any change at all to an interface breaks anyone who implements the interface. We discussed whether there was any reasonable alternative to adding cancellation token to the async methods declared by our interfaces. Ultimately, every alternative seemed like a less attractive end-state for the library. Adding cancellation token will break all these interfaces, but from our evaluation it is the most intuitive solution for the future health of the API surface. The cost is a breaking change in our interfaces for v3.

V3 introduces the concept of dynamic features. In v2, the library frequently used "feature", but now this could be ambiguous between the concepts of feature flags and dynamic features. We thought this could be improved. If we had not already decided to break our interface through the addition of cancellation token parameters, we would not have made any breaking changes. We would have continued to use names like IFeatureDefinitionProvider. However, since we already have to introduce breaking changes we thought that taking the opportunity to add specificity to the API surface would be beneficial.

Thoughts/feedback on both of these topics are certainly appreciated.

Thank you for that list of names that are inconsistent. FeatureName does look like it should be FeatureFlagName to coincide with the other changes that were made.

[brental]

Hey @jimmyca15, thanks for the reply and apologies for the slow response.

I didn't realise that there were already breaking changes being made in v3 to support cancellation tokens. I agree that making the breaking changes to the interfaces to add that support seems like the best option. In that case, the renaming breaking changes I outlined above aren't really an issue and maybe it is just a matter of updating other parts of the code to use FeatureFlag instead of just Feature where appropriate to match the changes that have already been made.

However, in terms of the differentiation between a feature flag and a dynamic feature in v3, is there any doco that outlines the intended usages of dynamic features and how they relate to feature flags? Will they generally be completely separate concepts or will there be cases where they are used together? For example, the dynamic feature manager doesn't have an IsEnabled method so would would you need to use IFeatureManager to first check whether a feature flag is enabled and then use the dynamic feature manager to get the variant of a feature that should be used?

[jimmyca15]

Hello @brental

The readme in the feature/v3 branch documents dynamic features. It outlines what dynamic features are, what they could be used for, and how they can be used. I think that should be able to provide all the insights you're looking for, and if anything is missing we can add it.

[jimmyca15]

To answer your question specifically though, dynamic features are usually used when you already know a feature should be on. It's more of a matter of the application knowing which version/variant of a feature to use.

[brental]

@jimmyca15 Thanks for that explanation and linking those docs, I will have a read. Since these changes aren't really an issue given the other breaking changes and a separate issue raised to update the remaining feature references to feature flag I will close this issue.