-
Notifications
You must be signed in to change notification settings - Fork 23
feat: Extract minimal DI integration to OpenFeature.DependencyInjection.Abstractions #596
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
arttonoyan
wants to merge
19
commits into
open-feature:main
Choose a base branch
from
arttonoyan:feature/di-abstractions-builder-api-587
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 13 commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
2c42d1f
Add OpenFeature.DependencyInjection.Abstractions project
arttonoyan 4d69c6f
Refactor OpenFeature for DI and provider abstraction
arttonoyan 7e26ba1
Refactor AddProvider API for simplicity and consistency
arttonoyan 1edb0c9
Refactor FeatureLifecycleManager initialization
arttonoyan 6cfaeb7
Refactor tests to use OpenFeatureProviderOptions
arttonoyan dbdb9b5
Merge branch 'main' into feature/di-abstractions-builder-api-587
arttonoyan acade51
Merge branch 'main' into feature/di-abstractions-builder-api-587
arttonoyan 10203f3
Merge branch 'main' into feature/di-abstractions-builder-api-587
arttonoyan 3d680f3
Removev ID property
arttonoyan f9a6781
Merge branch 'feature/di-abstractions-builder-api-587' of https://git…
arttonoyan 4b1237a
Update src/OpenFeature.Hosting/Internal/FeatureLifecycleManager.cs
arttonoyan 04ea063
Update src/OpenFeature.Hosting/Internal/FeatureLifecycleManager.cs
arttonoyan f521321
Refactor OpenFeatureOptions to OpenFeatureProviderOptions
arttonoyan 08cdb69
Refactor Dependency Injection Abstractions
arttonoyan 9884603
chore: Merge dev into current branch and resolve merge conflicts
arttonoyan e5e9af6
Merge branch 'main' into feature/di-abstractions-builder-api-587
arttonoyan e5bc0ca
Merge branch 'main' into feature/di-abstractions-builder-api-587
arttonoyan d0de7ee
Remove inheritdoc from FeatureLifecycleManager
arttonoyan 0d7e439
Merge branch 'feature/di-abstractions-builder-api-587' of https://git…
arttonoyan File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
22 changes: 22 additions & 0 deletions
22
...ture.DependencyInjection.Abstractions/OpenFeature.DependencyInjection.Abstractions.csproj
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| <Project Sdk="Microsoft.NET.Sdk"> | ||
|
|
||
| <PropertyGroup> | ||
| <TargetFrameworks>netstandard2.0;net8.0;net9.0;net462</TargetFrameworks> | ||
| <RootNamespace>OpenFeature.DependencyInjection.Abstractions</RootNamespace> | ||
| </PropertyGroup> | ||
|
|
||
| <ItemGroup> | ||
| <PackageReference Include="Microsoft.Extensions.DependencyInjection.Abstractions" /> | ||
| <PackageReference Include="Microsoft.Extensions.Options" /> | ||
| </ItemGroup> | ||
|
|
||
| <ItemGroup> | ||
| <InternalsVisibleTo Include="OpenFeature.Hosting.Tests" /> | ||
| <InternalsVisibleTo Include="DynamicProxyGenAssembly2" /> | ||
| </ItemGroup> | ||
|
|
||
| <ItemGroup> | ||
| <ProjectReference Include="..\OpenFeature\OpenFeature.csproj" /> | ||
| </ItemGroup> | ||
|
|
||
| </Project> | ||
62 changes: 62 additions & 0 deletions
62
src/OpenFeature.DependencyInjection.Abstractions/OpenFeatureProviderBuilder.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| using Microsoft.Extensions.DependencyInjection; | ||
|
|
||
| namespace OpenFeature.DependencyInjection.Abstractions; | ||
|
|
||
| /// <summary> | ||
| /// Describes a <see cref="OpenFeatureProviderBuilder"/> backed by an <see cref="IServiceCollection"/>. | ||
| /// </summary> | ||
| public abstract class OpenFeatureProviderBuilder(IServiceCollection services) | ||
| { | ||
| /// <summary> The services being configured. </summary> | ||
| public IServiceCollection Services { get; } = services; | ||
|
|
||
| /// <summary> | ||
| /// Gets a value indicating whether a default provider has been registered. | ||
| /// </summary> | ||
| public bool HasDefaultProvider { get; internal set; } | ||
|
|
||
| /// <summary> | ||
| /// Gets the count of domain-bound providers that have been registered. | ||
| /// This count does not include the default provider. | ||
| /// </summary> | ||
| public int DomainBoundProviderRegistrationCount { get; internal set; } | ||
|
|
||
| /// <summary> | ||
| /// Indicates whether the policy has been configured. | ||
| /// </summary> | ||
| public bool IsPolicyConfigured { get; internal set; } | ||
|
|
||
| /// <summary> | ||
| /// Validates the current configuration, ensuring that a policy is set when multiple providers are registered | ||
| /// or when a default provider is registered alongside another provider. | ||
| /// </summary> | ||
| /// <exception cref="InvalidOperationException"> | ||
| /// Thrown if multiple providers are registered without a policy, or if both a default provider | ||
| /// and an additional provider are registered without a policy configuration. | ||
| /// </exception> | ||
| public void Validate() | ||
| { | ||
| if (IsPolicyConfigured) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| if (DomainBoundProviderRegistrationCount > 1) | ||
| { | ||
| throw new InvalidOperationException("Multiple providers have been registered, but no policy has been configured."); | ||
| } | ||
|
|
||
| if (HasDefaultProvider && DomainBoundProviderRegistrationCount == 1) | ||
| { | ||
| throw new InvalidOperationException("A default provider and an additional provider have been registered without a policy configuration."); | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Adds an IFeatureClient to the container. If <paramref name="name"/> is supplied, | ||
| /// registers a domain-bound client; otherwise registers a global client. If an evaluation context is | ||
| /// configured, it is applied at resolve-time. | ||
| /// </summary> | ||
| /// <returns>The current <see cref="OpenFeatureProviderBuilder"/>.</returns> | ||
| internal protected abstract OpenFeatureProviderBuilder TryAddClient(string? name = null); | ||
| } |
145 changes: 145 additions & 0 deletions
145
src/OpenFeature.DependencyInjection.Abstractions/OpenFeatureProviderBuilderExtensions.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,145 @@ | ||
| using Microsoft.Extensions.DependencyInjection; | ||
| using Microsoft.Extensions.DependencyInjection.Extensions; | ||
| using OpenFeature.DependencyInjection.Abstractions; | ||
|
|
||
| namespace OpenFeature; | ||
|
|
||
| /// <summary> | ||
| /// Contains extension methods for the <see cref="OpenFeatureProviderBuilder"/> class. | ||
| /// </summary> | ||
| #if NET8_0_OR_GREATER | ||
| [System.Diagnostics.CodeAnalysis.Experimental(DependencyInjection.Abstractions.Diagnostics.FeatureCodes.NewDi)] | ||
| #endif | ||
| public static partial class OpenFeatureProviderBuilderExtensions | ||
| { | ||
| /// <summary> | ||
| /// Adds a feature provider using a factory method without additional configuration options. | ||
| /// This method adds the feature provider as a transient service and sets it as the default provider within the application. | ||
| /// </summary> | ||
| /// <param name="builder">The <see cref="OpenFeatureProviderBuilder"/> used to configure feature flags.</param> | ||
| /// <param name="implementationFactory"> | ||
| /// A factory method that creates and returns a <see cref="FeatureProvider"/> | ||
| /// instance based on the provided service provider. | ||
| /// </param> | ||
| /// <returns>The updated <see cref="OpenFeatureProviderBuilder"/> instance with the default feature provider set and configured.</returns> | ||
| /// <exception cref="ArgumentNullException">Thrown if the <paramref name="builder"/> is null, as a valid builder is required to add and configure providers.</exception> | ||
| public static OpenFeatureProviderBuilder AddProvider(this OpenFeatureProviderBuilder builder, Func<IServiceProvider, FeatureProvider> implementationFactory) | ||
| => AddProvider<OpenFeatureProviderOptions>(builder, implementationFactory, null); | ||
|
|
||
| /// <summary> | ||
| /// Adds a feature provider using a factory method to create the provider instance and optionally configures its settings. | ||
| /// This method adds the feature provider as a transient service and sets it as the default provider within the application. | ||
| /// </summary> | ||
| /// <typeparam name="TOptions"> Type derived from <see cref="OpenFeatureProviderBuilder"/> used to configure the feature provider.</typeparam> | ||
| /// <param name="builder">The <see cref="OpenFeatureProviderBuilder"/> used to configure feature flags.</param> | ||
| /// <param name="implementationFactory"> | ||
| /// A factory method that creates and returns a <see cref="FeatureProvider"/> | ||
| /// instance based on the provided service provider. | ||
| /// </param> | ||
| /// <param name="configureOptions">An optional delegate to configure the provider-specific options.</param> | ||
| /// <returns>The updated <see cref="OpenFeatureProviderBuilder"/> instance with the default feature provider set and configured.</returns> | ||
| /// <exception cref="ArgumentNullException">Thrown if the <paramref name="builder"/> is null, as a valid builder is required to add and configure providers.</exception> | ||
| public static OpenFeatureProviderBuilder AddProvider<TOptions>(this OpenFeatureProviderBuilder builder, Func<IServiceProvider, FeatureProvider> implementationFactory, Action<TOptions>? configureOptions) | ||
| where TOptions : OpenFeatureProviderOptions | ||
| { | ||
| if (builder == null) throw new ArgumentNullException(nameof(builder)); | ||
|
|
||
| builder.HasDefaultProvider = true; | ||
| builder.Services.PostConfigure<TOptions>(options => options.AddDefaultProviderName()); | ||
| if (configureOptions != null) | ||
| { | ||
| builder.Services.Configure(configureOptions); | ||
| } | ||
|
|
||
| builder.Services.TryAddTransient(implementationFactory); | ||
| builder.TryAddClient(); | ||
| return builder; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Adds a feature provider for a specific domain using provided options and a configuration builder. | ||
| /// </summary> | ||
| /// <typeparam name="TOptions"> Type derived from <see cref="OpenFeatureProviderBuilder"/> used to configure the feature provider.</typeparam> | ||
| /// <param name="builder">The <see cref="OpenFeatureProviderBuilder"/> used to configure feature flags.</param> | ||
| /// <param name="domain">The unique name of the provider.</param> | ||
| /// <param name="implementationFactory"> | ||
| /// A factory method that creates a feature provider instance. | ||
| /// It adds the provider as a transient service unless it is already added. | ||
| /// </param> | ||
| /// <param name="configureOptions">An optional delegate to configure the provider-specific options.</param> | ||
| /// <returns>The updated <see cref="OpenFeatureProviderBuilder"/> instance with the new feature provider configured.</returns> | ||
| /// <exception cref="ArgumentNullException"> | ||
| /// Thrown if either <paramref name="builder"/> or <paramref name="domain"/> is null or if the <paramref name="domain"/> is empty. | ||
| /// </exception> | ||
| public static OpenFeatureProviderBuilder AddProvider<TOptions>(this OpenFeatureProviderBuilder builder, string domain, Func<IServiceProvider, string, FeatureProvider> implementationFactory, Action<TOptions>? configureOptions) | ||
| where TOptions : OpenFeatureProviderOptions | ||
| { | ||
| if (builder == null) throw new ArgumentNullException(nameof(builder)); | ||
|
|
||
| builder.DomainBoundProviderRegistrationCount++; | ||
|
|
||
| builder.Services.PostConfigure<TOptions>(options => options.AddProviderName(domain)); | ||
| if (configureOptions != null) | ||
| { | ||
| builder.Services.Configure(domain, configureOptions); | ||
| } | ||
|
|
||
| builder.Services.TryAddKeyedTransient(domain, (provider, key) => | ||
| { | ||
| if (key == null) | ||
| { | ||
| throw new ArgumentNullException(nameof(key)); | ||
| } | ||
| return implementationFactory(provider, key.ToString()!); | ||
| }); | ||
|
|
||
| builder.TryAddClient(domain); | ||
| return builder; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Adds a feature provider for a specified domain using the default options. | ||
| /// This method configures a feature provider without custom options, delegating to the more generic AddProvider method. | ||
| /// </summary> | ||
| /// <param name="builder">The <see cref="OpenFeatureProviderBuilder"/> used to configure feature flags.</param> | ||
| /// <param name="domain">The unique name of the provider.</param> | ||
| /// <param name="implementationFactory"> | ||
| /// A factory method that creates a feature provider instance. | ||
| /// It adds the provider as a transient service unless it is already added. | ||
| /// </param> | ||
| /// <returns>The updated <see cref="OpenFeatureProviderBuilder"/> instance with the new feature provider configured.</returns> | ||
| /// <exception cref="ArgumentNullException"> | ||
| /// Thrown if either <paramref name="builder"/> or <paramref name="domain"/> is null or if the <paramref name="domain"/> is empty. | ||
| /// </exception> | ||
| public static OpenFeatureProviderBuilder AddProvider(this OpenFeatureProviderBuilder builder, string domain, Func<IServiceProvider, string, FeatureProvider> implementationFactory) | ||
| => AddProvider<OpenFeatureProviderOptions>(builder, domain, implementationFactory, configureOptions: null); | ||
|
|
||
| /// <summary> | ||
| /// Configures policy name options for OpenFeature using the specified options type. | ||
| /// </summary> | ||
| /// <typeparam name="TOptions">The type of options used to configure <see cref="OpenFeatureProviderOptions"/>.</typeparam> | ||
| /// <param name="builder">The <see cref="OpenFeatureProviderBuilder"/> instance.</param> | ||
| /// <param name="configureOptions">A delegate to configure <typeparamref name="TOptions"/>.</param> | ||
| /// <returns>The configured <see cref="OpenFeatureProviderBuilder"/> instance.</returns> | ||
| /// <exception cref="ArgumentNullException">Thrown when the <paramref name="builder"/> or <paramref name="configureOptions"/> is null.</exception> | ||
| public static OpenFeatureProviderBuilder AddPolicyName<TOptions>(this OpenFeatureProviderBuilder builder, Action<TOptions> configureOptions) | ||
| where TOptions : PolicyNameOptions | ||
| { | ||
| if (builder == null) throw new ArgumentNullException(nameof(builder)); | ||
| if (configureOptions == null) throw new ArgumentNullException(nameof(configureOptions)); | ||
|
|
||
| builder.IsPolicyConfigured = true; | ||
|
|
||
| builder.Services.Configure(configureOptions); | ||
| return builder; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Configures the default policy name options for OpenFeature. | ||
| /// </summary> | ||
| /// <param name="builder">The <see cref="OpenFeatureProviderBuilder"/> instance.</param> | ||
| /// <param name="configureOptions">A delegate to configure <see cref="OpenFeatureProviderBuilder"/>.</param> | ||
| /// <returns>The configured <see cref="OpenFeatureProviderBuilder"/> instance.</returns> | ||
| public static OpenFeatureProviderBuilder AddPolicyName(this OpenFeatureProviderBuilder builder, Action<PolicyNameOptions> configureOptions) | ||
| => AddPolicyName<PolicyNameOptions>(builder, configureOptions); | ||
| } |
61 changes: 61 additions & 0 deletions
61
src/OpenFeature.DependencyInjection.Abstractions/OpenFeatureProviderOptions.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| using System.Collections.ObjectModel; | ||
|
|
||
| namespace OpenFeature.DependencyInjection.Abstractions; | ||
|
|
||
| /// <summary> | ||
| /// Provider-focused options for configuring OpenFeature integrations. | ||
| /// Contains only contracts and metadata that integrations may need. | ||
| /// </summary> | ||
| public class OpenFeatureProviderOptions | ||
| { | ||
| private readonly HashSet<string> _providerNames = []; | ||
|
|
||
| /// <summary> | ||
| /// Determines if a default provider has been registered. | ||
| /// </summary> | ||
| public bool HasDefaultProvider { get; private set; } | ||
|
|
||
| /// <summary> | ||
| /// The <see cref="Type"/> of the configured feature provider, if any. | ||
| /// Typically set by higher-level configuration. | ||
| /// </summary> | ||
| public Type FeatureProviderType { get; protected internal set; } = null!; | ||
|
|
||
| /// <summary> | ||
| /// Gets a read-only list of registered provider names. | ||
| /// </summary> | ||
| public IReadOnlyCollection<string> ProviderNames | ||
| { | ||
| get | ||
| { | ||
| lock (_providerNames) | ||
| { | ||
| return new ReadOnlyCollection<string>([.. _providerNames]); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Registers the default provider name if no specific name is provided. | ||
| /// Sets <see cref="HasDefaultProvider"/> to true. | ||
| /// </summary> | ||
| internal void AddDefaultProviderName() => AddProviderName(null); | ||
|
|
||
| /// <summary> | ||
| /// Registers a new feature provider name. This operation is thread-safe. | ||
| /// </summary> | ||
| /// <param name="name">The name of the feature provider to register. Registers as default if null.</param> | ||
| internal void AddProviderName(string? name) | ||
| { | ||
| if (string.IsNullOrWhiteSpace(name)) | ||
| { | ||
| HasDefaultProvider = true; | ||
| return; | ||
| } | ||
|
|
||
| lock (_providerNames) | ||
| { | ||
| _providerNames.Add(name!); | ||
| } | ||
| } | ||
| } |
2 changes: 1 addition & 1 deletion
2
src/OpenFeature.Hosting/PolicyNameOptions.cs → ...jection.Abstractions/PolicyNameOptions.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| using Microsoft.Extensions.DependencyInjection; | ||
| using Microsoft.Extensions.Logging; | ||
| using Microsoft.Extensions.Options; | ||
| using OpenFeature.DependencyInjection.Abstractions; | ||
|
|
||
| namespace OpenFeature.Hosting.Internal; | ||
|
|
||
|
|
@@ -22,7 +23,14 @@ public async ValueTask EnsureInitializedAsync(CancellationToken cancellationToke | |
| { | ||
| this.LogStartingInitializationOfFeatureProvider(); | ||
|
|
||
| var options = _serviceProvider.GetRequiredService<IOptions<OpenFeatureOptions>>().Value; | ||
| await InitializeProvidersAsync(cancellationToken).ConfigureAwait(false); | ||
| InitializeHooks(); | ||
| InitializeHandlers(); | ||
| } | ||
|
|
||
| private async Task InitializeProvidersAsync(CancellationToken cancellationToken) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cancellationToken seems to not be used here. |
||
| { | ||
| var options = _serviceProvider.GetRequiredService<IOptions<OpenFeatureProviderOptions>>().Value; | ||
| if (options.HasDefaultProvider) | ||
| { | ||
| var featureProvider = _serviceProvider.GetRequiredService<FeatureProvider>(); | ||
|
|
@@ -34,7 +42,11 @@ public async ValueTask EnsureInitializedAsync(CancellationToken cancellationToke | |
| var featureProvider = _serviceProvider.GetRequiredKeyedService<FeatureProvider>(name); | ||
| await _featureApi.SetProviderAsync(name, featureProvider).ConfigureAwait(false); | ||
| } | ||
| } | ||
|
|
||
| private void InitializeHooks() | ||
| { | ||
| var options = _serviceProvider.GetRequiredService<IOptions<OpenFeatureOptions>>().Value; | ||
| var hooks = new List<Hook>(); | ||
| foreach (var hookName in options.HookNames) | ||
| { | ||
|
|
@@ -43,7 +55,10 @@ public async ValueTask EnsureInitializedAsync(CancellationToken cancellationToke | |
| } | ||
|
|
||
| _featureApi.AddHooks(hooks); | ||
| } | ||
|
|
||
| private void InitializeHandlers() | ||
| { | ||
| var handlers = _serviceProvider.GetServices<EventHandlerDelegateWrapper>(); | ||
| foreach (var handler in handlers) | ||
| { | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Should we have these abstractions in their own namespace? Would it be more accessible to have them in say just the
OpenFeaturenamespace?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kylejuliandev Great question - thanks for calling it out.
I’d keep these in their own namespace for now.
OpenFeature.DependencyInjection.Abstractionstargets provider authors (integrating new providers), not typical app consumers. Keeping it separate avoids cluttering the rootOpenFeaturenamespace and lets us evolve DI-specific types independently.Of course, I’m open to discussion about this.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer the @arttonoyan. This is a very specific Abstraction layer.
I am wondering if we should keep in a
OpenFeature.Abstractionsinstead. This would be a "library" of justOpenFeaturerelated abstractions like "hooks", "provider"... I am happy to keepOpenFeature.DependencyInjection.Abstractionsif the rest of the team agrees.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@askpt @kylejuliandev
Proposal
Rename the package to
OpenFeature.Providers.DependencyInjection. This name better reflects scope. It is intuitive for provider authors who want to add DI integration.Follow-up idea
In a separate discussion, consider extracting common provider contracts into
OpenFeature.Providers.Abstractions. We would then have two clear packages.OpenFeature.Providers.Abstractions, andOpenFeature.Providers.DependencyInjection.Why this helps
Abstractions, optionally on the DI helpers.OpenFeature.Providers.*naming.cc: @beeme1mr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems reasonable to me. What would be the proposed release strategy and impact? I believe the DI package is still marked as experimental, so breaking changes are allowed, but we should try and make it as straightforward for users to migrate. Also, I'd hope that we could remove the experimental badge shortly after making this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now our main pain point is coupling. The current abstractions package depends on the OpenFeature SDK because it includes the
OpenFeatureProviderabstraction. This means any SDK release. even when it has no provider changes. forces us to bump the abstractions package. Then every provider sees a new version and feels compelled to upgrade. even though nothing changed for providers. Providers should only react to provider related changes.Decoupling fixes this. If we move provider contracts into
OpenFeature.Providers.Abstractions. we isolate them from SDK churn. The DI helpers stay inOpenFeature.Providers.DependencyInjection. Result. cleaner dependency graph. fewer unnecessary upgrades. thinner provider packages. and clearer ownership of changes.I realize this is a significant change, and it needs attentive and careful attention. If the direction sounds reasonable, I can open a PR to rename the DI package, then draft a proposal for the abstractions split.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds reasonable to me. We've been looking into something similar (minus the DI part) in Java. FYI @chrfwow @toddbaert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. I’ll proceed and rename the package to
OpenFeature.Providers.DependencyInjectionin this PR.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@arttonoyan @beeme1mr @kylejuliandev: I think this is a great discussion.
I would propose for 2.9 to create:
OpenFeature.Providers.DependencyInjection(this PR)For 2.10:
OpenFeature.Providers.AbstractionsOpenFeature.Hooks.AbstractionsThe story that is happening in Providers, is also true for the Hooks.