microsoft / FeatureManagement-Dotnet

Microsoft.FeatureManagement provides standardized APIs for enabling feature flags within applications. Utilize this library to secure a consistent experience when developing applications that use patterns such as beta access, rollout, dark deployments, and more.
MIT License
1.06k stars 115 forks source link

feat: Support for an attribute-based feature toggling TagHelper #483

Open benmccallum opened 4 months ago

benmccallum commented 4 months ago

I think this would be extremely straightforward to implement and make for a tidier, less disruptive use of the tag helper.

<ul>
  <!-- Suggestion -->
  <li feat-name="" feat-requirement="" feat-negate="">MenuItem</li>

  <!-- Current -->
  <feature name="" requirement="" negate="">
    <li>MenuItem</li>
  </feature>
</ul>

Required changes:

It's really quite similar to the ConditionTagHelper in the docs here.

I'm happy to implement this as a PR if it will be accepted. I'll probably be creating one in our repo in the short term.

benmccallum commented 3 months ago

A drop-in solution for anyone who wants this right now.

using Microsoft.AspNetCore.Razor.TagHelpers;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;

// May be absorbed into the core library if this issue is approved
// https://github.com/microsoft/FeatureManagement-Dotnet/issues/483

namespace Microsoft.FeatureManagement.Mvc.TagHelpers
{
    /// <summary>
    /// Provides an attribute-based tag helper that can be used to conditionally render content based on a feature's state.
    /// </summary>
    [HtmlTargetElement(Attributes = "feat-name")]
    public class FeatTagHelper : TagHelper
    {
        private readonly IFeatureManager _featureManager;

        /// <summary>
        /// A feature name, or comma separated list of feature names, for which the content should be rendered. By default, all specified features must be enabled to render the content, but this requirement can be controlled by the <see cref="Requirement"/> property.
        /// </summary>
        public string FeatName { get; set; } = "";

        /// <summary>
        /// Controls whether 'All' or 'Any' feature in a list of features should be enabled to render the content within the feature tag.
        /// </summary>
        public RequirementType FeatRequirement { get; set; } = RequirementType.All;

        /// <summary>
        /// Negates the evaluation for whether or not a feature tag should display content. This is used to display alternate content when a feature or set of features are disabled.
        /// </summary>
        public bool FeatNegate { get; set; }

        /// <summary>
        /// Creates a feature tag helper.
        /// </summary>
        /// <param name="featureManager">The feature manager snapshot to use to evaluate feature state.</param>
        public FeatTagHelper(IFeatureManagerSnapshot featureManager)
        {
            _featureManager = featureManager;
        }

        /// <summary>
        /// Processes the tag helper context to evaluate if the feature's content should be rendered.
        /// </summary>
        /// <param name="context">The tag helper context.</param>
        /// <param name="output">The tag helper output.</param>
        public override async Task ProcessAsync(TagHelperContext context, TagHelperOutput output)
        {
            if (output.TagName == "feature")
            {
                // We don't want the feature element to actually be a part of HTML, so we strip it
                output.TagName = null; 
            }

            bool enabled = false;

            if (!string.IsNullOrEmpty(FeatName))
            {
                IEnumerable<string> names = FeatName.Split(',').Select(n => n.Trim());

                enabled = FeatRequirement == RequirementType.All
                    ? await All(names, _featureManager.IsEnabledAsync)
                    : await Any(names, _featureManager.IsEnabledAsync);
            }

            if (FeatNegate)
            {
                enabled = !enabled;
            }

            if (!enabled)
            {
                output.SuppressOutput();
            }
        }

#pragma warning disable IDE1006
#pragma warning disable IDE0007
#pragma warning disable VSTHRD200
        private static async Task<bool> Any<TSource>(IEnumerable<TSource> source, Func<TSource, Task<bool>> predicate)
        {
            bool enabled = false;

            foreach (TSource item in source)
            {
                if (await predicate(item).ConfigureAwait(false))
                {
                    enabled = true;

                    break;
                }
            }

            return enabled;
        }

        private static async Task<bool> All<TSource>(IEnumerable<TSource> source, Func<TSource, Task<bool>> predicate)
        {
            bool enabled = true;

            foreach (TSource item in source)
            {
                if (!await predicate(item).ConfigureAwait(false))
                {
                    enabled = false;

                    break;
                }
            }

            return enabled;
        }
    }
}
zhiyuanliang-ms commented 3 months ago

Hi, @benmccallum Thank you for the suggestion.

When developing new features in the feature management library, we always prioritize avoiding any breaking changes. Therefore, we are unlikely to remove our existing feature tag helper. But, theoretically, we can support both.

Whether the attribute-based tag helper is necessarily better than our current feature tag is subjective. I am not sure if there are any relevant conventions regarding this, but personally, I find our current feature tag clearer in terms of code readability.

benmccallum commented 3 months ago

Hi @zhiyuanliang-ms , yea definitely not suggesting this replace the original tag helper, it would be complimentary and offer users choice, particular for the scenario I've highlighted where the additional nesting of the current helper clutters the HTML.

What GitHub's code highlighter doesn't show here either is the colour VS applies to tag helper elements, which makes it clearer than shown here.

benmccallum commented 1 day ago

Hi @zhiyuanliang-ms, given my above comment, given it's complimentary/non-breaking, would you be open to me PR'ing that attribute-based tag helper in?

zhiyuanliang-ms commented 1 day ago

Hi, @benmccallum Thank you for paying attention to the Feature Management lib. 😃

would you be open to me PR'ing that attribute-based tag helper in?

Yes, we welcome any PRs. Really appreciate that. Even if it won't be merged, it still helps us to learn what customers want.

Just want to confirm, is the only change to add the attribute [HtmlTargetElement(Attributes = "feat-name")] for FeatureTagHelper? And it won't affect the current usage:

<feature name=""></feature>.

And it will allow another usage: <li feat-name="" feat-requirement="" feat-negate="">MenuItem</li>, right?

Do we have to modify the public properties's name, e.g. FeatureTagHelper.Requirement -> FeatureTagHelper.FeatRequirement?

@rossgrambo @zhenlan @jimmyca15