-
Notifications
You must be signed in to change notification settings - Fork 6
feat: Add initial dependency injection support for LaunchDarkly provider #49
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
22
commits into
launchdarkly:main
Choose a base branch
from
arttonoyan:arttonoyan/feature/issue-48-add-di-support
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 18 commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
cc52ab2
✨ feat(dependency-injection): add LaunchDarkly support
arttonoyan 1292120
✅ test: enhance LaunchDarkly integration tests
arttonoyan d9ec58b
♻️ refactor(dependency-injection): rename methods and update config
arttonoyan 86b9c04
♻️ refactor(tests): update FeatureClient to IFeatureClient
arttonoyan 8fbfab0
♻️ refactor(dependency-injection): rename and enhance LaunchDarkly in…
arttonoyan 92515b1
📝 docs(provider): clarify early configuration validation
arttonoyan 830ad98
✅ test: restructure LaunchDarkly integration tests
arttonoyan c958f44
🔧 chore(tests): remove LaunchDarkly integration tests
arttonoyan 1f6beb8
🔨 build: update project to target .NET 8.0 only
arttonoyan 267bde5
🔧 chore: add .NET 8.0 conditional compilation directives
arttonoyan 741f6d2
✅ test(LaunchDarklyOpenFeatureBuilderExtensions): rename tests
arttonoyan 63f26df
♻️ refactor(config): improve configuration handling
arttonoyan c803a2d
🔧 chore(tests): update dependencies and add integration tests
arttonoyan 5e6b828
♻️ refactor(OpenFeature): remove context configuration
arttonoyan ccbd1d6
♻️ refactor(provider): rename and optimize configuration methods
arttonoyan 54a5753
✅ test(LaunchDarklyOpenFeatureBuilderExtensions): enable scope valida…
arttonoyan 85a36b1
♻️ refactor(tests): change registerWithThrowing to method
arttonoyan a010d52
♻️ refactor(launchdarkly): improve null handling in configuration
arttonoyan 2ba3481
Update src/LaunchDarkly.OpenFeature.ServerProvider.DependencyInjectio…
arttonoyan 3721562
♻️ refactor(dependency-injection): rename stdKey to sdkKey
arttonoyan b6ee8ed
♻️ refactor(dependency-injection): rename provider registration method
arttonoyan 2e8fb99
💄 style(dependency-injection): improve code readability
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
Some comments aren't visible on the classic Files Changed page.
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,32 +1,40 @@ | ||
| | ||
| Microsoft Visual Studio Solution File, Format Version 12.00 | ||
| # Visual Studio Version 16 | ||
| VisualStudioVersion = 16.0.30114.105 | ||
| # Visual Studio Version 17 | ||
| VisualStudioVersion = 17.14.36127.28 d17.14 | ||
| MinimumVisualStudioVersion = 10.0.40219.1 | ||
| Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "LaunchDarkly.OpenFeature.ServerProvider", "src\LaunchDarkly.OpenFeature.ServerProvider\LaunchDarkly.OpenFeature.ServerProvider.csproj", "{B61EC563-2D25-47C8-86A4-0C3A8C625109}" | ||
| EndProject | ||
| Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "LaunchDarkly.OpenFeature.ServerProvider.Tests", "test\LaunchDarkly.OpenFeature.ServerProvider.Tests\LaunchDarkly.OpenFeature.ServerProvider.Tests.csproj", "{A1B1DB34-6B22-4AA4-9974-6EE67145BDB9}" | ||
| EndProject | ||
| Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "LaunchDarkly.OpenFeature.ServerProvider.DependencyInjection", "src\LaunchDarkly.OpenFeature.ServerProvider.DependencyInjection\LaunchDarkly.OpenFeature.ServerProvider.DependencyInjection.csproj", "{DB523227-21AE-4B92-A263-05050CEF6C8D}" | ||
| EndProject | ||
| Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "LaunchDarkly.OpenFeature.ServerProvider.DependencyInjection.Tests", "test\LaunchDarkly.OpenFeature.ServerProvider.DependencyInjection.Tests\LaunchDarkly.OpenFeature.ServerProvider.DependencyInjection.Tests.csproj", "{F2C8E3A6-12D4-4B8F-9A7E-3F5C8D6B4E2A}" | ||
| EndProject | ||
| Global | ||
| GlobalSection(SolutionConfigurationPlatforms) = preSolution | ||
| Debug|Any CPU = Debug|Any CPU | ||
| Release|Any CPU = Release|Any CPU | ||
| EndGlobalSection | ||
| GlobalSection(SolutionProperties) = preSolution | ||
| HideSolutionNode = FALSE | ||
| EndGlobalSection | ||
| GlobalSection(ProjectConfigurationPlatforms) = postSolution | ||
| {B61EC563-2D25-47C8-86A4-0C3A8C625109}.Debug|Any CPU.ActiveCfg = Debug|Any CPU | ||
| {B61EC563-2D25-47C8-86A4-0C3A8C625109}.Debug|Any CPU.Build.0 = Debug|Any CPU | ||
| {B61EC563-2D25-47C8-86A4-0C3A8C625109}.Release|Any CPU.ActiveCfg = Release|Any CPU | ||
| {B61EC563-2D25-47C8-86A4-0C3A8C625109}.Release|Any CPU.Build.0 = Release|Any CPU | ||
| {E8CE160B-0A65-480F-AA3F-028AD9F17F6E}.Debug|Any CPU.ActiveCfg = Debug|Any CPU | ||
| {E8CE160B-0A65-480F-AA3F-028AD9F17F6E}.Debug|Any CPU.Build.0 = Debug|Any CPU | ||
| {E8CE160B-0A65-480F-AA3F-028AD9F17F6E}.Release|Any CPU.ActiveCfg = Release|Any CPU | ||
| {E8CE160B-0A65-480F-AA3F-028AD9F17F6E}.Release|Any CPU.Build.0 = Release|Any CPU | ||
| {A1B1DB34-6B22-4AA4-9974-6EE67145BDB9}.Debug|Any CPU.ActiveCfg = Debug|Any CPU | ||
| {A1B1DB34-6B22-4AA4-9974-6EE67145BDB9}.Debug|Any CPU.Build.0 = Debug|Any CPU | ||
| {A1B1DB34-6B22-4AA4-9974-6EE67145BDB9}.Release|Any CPU.ActiveCfg = Release|Any CPU | ||
| {A1B1DB34-6B22-4AA4-9974-6EE67145BDB9}.Release|Any CPU.Build.0 = Release|Any CPU | ||
| {DB523227-21AE-4B92-A263-05050CEF6C8D}.Debug|Any CPU.ActiveCfg = Debug|Any CPU | ||
| {DB523227-21AE-4B92-A263-05050CEF6C8D}.Debug|Any CPU.Build.0 = Debug|Any CPU | ||
| {DB523227-21AE-4B92-A263-05050CEF6C8D}.Release|Any CPU.ActiveCfg = Release|Any CPU | ||
| {DB523227-21AE-4B92-A263-05050CEF6C8D}.Release|Any CPU.Build.0 = Release|Any CPU | ||
| {F2C8E3A6-12D4-4B8F-9A7E-3F5C8D6B4E2A}.Debug|Any CPU.ActiveCfg = Debug|Any CPU | ||
| {F2C8E3A6-12D4-4B8F-9A7E-3F5C8D6B4E2A}.Debug|Any CPU.Build.0 = Debug|Any CPU | ||
| {F2C8E3A6-12D4-4B8F-9A7E-3F5C8D6B4E2A}.Release|Any CPU.ActiveCfg = Release|Any CPU | ||
| {F2C8E3A6-12D4-4B8F-9A7E-3F5C8D6B4E2A}.Release|Any CPU.Build.0 = Release|Any CPU | ||
| EndGlobalSection | ||
| GlobalSection(SolutionProperties) = preSolution | ||
| HideSolutionNode = FALSE | ||
| EndGlobalSection | ||
| EndGlobal |
52 changes: 52 additions & 0 deletions
52
...er.DependencyInjection/LaunchDarkly.OpenFeature.ServerProvider.DependencyInjection.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,52 @@ | ||
| <Project Sdk="Microsoft.NET.Sdk"> | ||
|
|
||
| <PropertyGroup> | ||
| <!--x-release-please-start-version--> | ||
| <Version>2.1.1</Version> | ||
| <!--x-release-please-end--> | ||
| <!-- The BUILDFRAMEWORKS variable allows us to override the target frameworks with a | ||
| single framework that we are testing; this allows us to test with older SDK | ||
| versions that would error out if they saw any newer target frameworks listed | ||
| here, even if we weren't running those. --> | ||
| <BuildFrameworks Condition="'$(BUILDFRAMEWORKS)' == ''">net8.0</BuildFrameworks> | ||
| <TargetFrameworks>$(BUILDFRAMEWORKS)</TargetFrameworks> | ||
|
|
||
| <DebugType>portable</DebugType> | ||
| <AssemblyName>LaunchDarkly.OpenFeature.ServerProvider.DependencyInjection</AssemblyName> | ||
| <OutputType>Library</OutputType> | ||
| <PackageId>LaunchDarkly.OpenFeature.ServerProvider.DependencyInjection</PackageId> | ||
| <RootNamespace>LaunchDarkly.OpenFeature.ServerProvider.DependencyInjection</RootNamespace> | ||
| <LangVersion>7.3</LangVersion> | ||
| <Description> | ||
| Dependency injection support for the LaunchDarkly OpenFeature .NET Server Provider. | ||
| Enables seamless integration of LaunchDarkly feature flagging with .NET applications via OpenFeature’s DI patterns. | ||
| </Description> | ||
| <Authors>LaunchDarkly</Authors> | ||
| <Owners>LaunchDarkly</Owners> | ||
| <Company>LaunchDarkly</Company> | ||
| <Authors>LaunchDarkly</Authors> | ||
| <Owners>LaunchDarkly</Owners> | ||
| <Copyright>Copyright 2022 LaunchDarkly</Copyright> | ||
| <PackageLicenseExpression>Apache-2.0</PackageLicenseExpression> | ||
| <PackageProjectUrl>https://github.com/launchdarkly/openfeature-dotnet-server</PackageProjectUrl> | ||
| <RepositoryUrl>https://github.com/launchdarkly/openfeature-dotnet-server</RepositoryUrl> | ||
| <RepositoryBranch>main</RepositoryBranch> | ||
| <IncludeSymbols>true</IncludeSymbols> | ||
| <SymbolPackageFormat>snupkg</SymbolPackageFormat> | ||
|
|
||
| <!-- fail if XML comments are missing or invalid --> | ||
| <WarningsAsErrors>1570,1571,1572,1573,1574,1580,1581,1584,1591,1710,1711,1712</WarningsAsErrors> | ||
| </PropertyGroup> | ||
|
|
||
| <ItemGroup> | ||
| <PackageReference Include="OpenFeature.DependencyInjection" Version="[2.2.0, 3.0.0)" /> | ||
| </ItemGroup> | ||
|
|
||
| <ItemGroup> | ||
| <ProjectReference Include="..\LaunchDarkly.OpenFeature.ServerProvider\LaunchDarkly.OpenFeature.ServerProvider.csproj" /> | ||
| </ItemGroup> | ||
|
|
||
| <PropertyGroup> | ||
| <DocumentationFile>bin\$(Configuration)\$(TargetFramework)\LaunchDarkly.OpenFeature.ServerProvider.DependencyInjection.xml</DocumentationFile> | ||
| </PropertyGroup> | ||
| </Project> |
162 changes: 162 additions & 0 deletions
162
...penFeature.ServerProvider.DependencyInjection/LaunchDarklyOpenFeatureBuilderExtensions.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,162 @@ | ||
| using LaunchDarkly.Sdk.Server; | ||
| using Microsoft.Extensions.DependencyInjection; | ||
| using Microsoft.Extensions.DependencyInjection.Extensions; | ||
| using Microsoft.Extensions.Options; | ||
| using OpenFeature; | ||
| using OpenFeature.DependencyInjection; | ||
| using System; | ||
|
|
||
| namespace LaunchDarkly.OpenFeature.ServerProvider.DependencyInjection | ||
| { | ||
| /// <summary> | ||
| /// Provides extension methods for configuring the <see cref="OpenFeatureBuilder"/> to use LaunchDarkly as a <see cref="FeatureProvider"/>. | ||
| /// </summary> | ||
| public static partial class LaunchDarklyOpenFeatureBuilderExtensions | ||
| { | ||
| /// <summary> | ||
| /// Configures the <see cref="OpenFeatureBuilder"/> to use LaunchDarkly as the default provider | ||
| /// using the specified <see cref="Configuration"/> instance. | ||
| /// </summary> | ||
| /// <param name="builder">The <see cref="OpenFeatureBuilder"/> instance to configure.</param> | ||
| /// <param name="configuration">A pre-built LaunchDarkly <see cref="Configuration"/>.</param> | ||
| /// <returns>The updated <see cref="OpenFeatureBuilder"/> instance.</returns> | ||
| /// <exception cref="ArgumentNullException"> | ||
| /// Thrown when the <paramref name="configuration"/> argument is <c>null</c>. | ||
| /// </exception> | ||
| public static OpenFeatureBuilder UseLaunchDarkly(this OpenFeatureBuilder builder, Configuration configuration) | ||
| { | ||
| if (configuration == null) | ||
| { | ||
| throw new ArgumentNullException(nameof(configuration), "Configuration cannot be null."); | ||
| } | ||
|
|
||
| return RegisterLaunchDarklyProvider( | ||
| builder, | ||
| () => configuration, | ||
| sp => sp.GetRequiredService<Configuration>() | ||
| ); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Configures the <see cref="OpenFeatureBuilder"/> to use LaunchDarkly as a domain-scoped provider | ||
| /// using the specified <see cref="Configuration"/> instance. | ||
| /// </summary> | ||
| /// <param name="builder">The <see cref="OpenFeatureBuilder"/> instance to configure.</param> | ||
| /// <param name="domain">A domain identifier (e.g., tenant or environment).</param> | ||
| /// <param name="configuration">A pre-built LaunchDarkly <see cref="Configuration"/> specific to the domain.</param> | ||
| /// <returns>The updated <see cref="OpenFeatureBuilder"/> instance.</returns> | ||
| /// <exception cref="ArgumentNullException"> | ||
| /// Thrown when the <paramref name="configuration"/> argument is <c>null</c>. | ||
| /// </exception> | ||
| public static OpenFeatureBuilder UseLaunchDarkly(this OpenFeatureBuilder builder, string domain, Configuration configuration) | ||
| { | ||
| if (configuration == null) | ||
| { | ||
| throw new ArgumentNullException(nameof(configuration), "Configuration cannot be null."); | ||
| } | ||
|
|
||
| return RegisterLaunchDarklyProviderForDomain( | ||
| builder, | ||
| domain, | ||
| () => configuration, | ||
| (sp, key) => sp.GetRequiredKeyedService<Configuration>(key) | ||
| ); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Configures the <see cref="OpenFeatureBuilder"/> to use LaunchDarkly as the default provider | ||
| /// using the specified SDK key and optional configuration delegate. | ||
| /// </summary> | ||
| /// <param name="builder">The <see cref="OpenFeatureBuilder"/> instance to configure.</param> | ||
| /// <param name="stdKey">The SDK key used to initialize the LaunchDarkly configuration.</param> | ||
| /// <param name="configure">An optional delegate to customize the <see cref="ConfigurationBuilder"/>.</param> | ||
| /// <returns>The updated <see cref="OpenFeatureBuilder"/> instance.</returns> | ||
| public static OpenFeatureBuilder UseLaunchDarkly(this OpenFeatureBuilder builder, string stdKey, Action<ConfigurationBuilder> configure = null) | ||
| => RegisterLaunchDarklyProvider(builder, () => CreateConfiguration(stdKey, configure), sp => sp.GetRequiredService<Configuration>()); | ||
|
|
||
| /// <summary> | ||
| /// Configures the <see cref="OpenFeatureBuilder"/> to use LaunchDarkly as a domain-scoped provider | ||
| /// using the specified SDK key and optional configuration delegate. | ||
| /// </summary> | ||
| /// <param name="builder">The <see cref="OpenFeatureBuilder"/> instance to configure.</param> | ||
| /// <param name="domain">A domain identifier (e.g., tenant or environment).</param> | ||
| /// <param name="stdKey">The SDK key used to initialize the LaunchDarkly configuration.</param> | ||
| /// <param name="configure">An optional delegate to customize the <see cref="ConfigurationBuilder"/>.</param> | ||
| /// <returns>The updated <see cref="OpenFeatureBuilder"/> instance.</returns> | ||
| public static OpenFeatureBuilder UseLaunchDarkly(this OpenFeatureBuilder builder, string domain, string stdKey, Action<ConfigurationBuilder> configure = null) | ||
| => RegisterLaunchDarklyProviderForDomain( | ||
| builder, | ||
| domain, | ||
| () => CreateConfiguration(stdKey, configure), | ||
| (sp, key) => sp.GetRequiredKeyedService<Configuration>(key)); | ||
|
|
||
| /// <summary> | ||
| /// Registers LaunchDarkly as the default feature provider using the given configuration factory and resolution logic. | ||
| /// </summary> | ||
| /// <param name="builder">The <see cref="OpenFeatureBuilder"/> instance to configure.</param> | ||
| /// <param name="createConfiguration">A delegate that returns a <see cref="Configuration"/> instance.</param> | ||
| /// <param name="resolveConfiguration">A delegate that resolves the <see cref="Configuration"/> from the service provider.</param> | ||
| /// <returns>The updated <see cref="OpenFeatureBuilder"/> instance.</returns> | ||
| private static OpenFeatureBuilder RegisterLaunchDarklyProvider( | ||
| OpenFeatureBuilder builder, | ||
| Func<Configuration> createConfiguration, | ||
| Func<IServiceProvider, Configuration> resolveConfiguration) | ||
| { | ||
| // Perform early configuration validation to ensure the provider is correctly constructed. | ||
| // This ensures any misconfiguration is caught during application startup rather than at runtime. | ||
| var config = createConfiguration(); | ||
| builder.Services.TryAddSingleton(config); | ||
|
|
||
| return builder.AddProvider(serviceProvider => new Provider(resolveConfiguration(serviceProvider))); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Registers LaunchDarkly as a domain-scoped feature provider using the given configuration factory and resolution logic. | ||
| /// </summary> | ||
| /// <param name="builder">The <see cref="OpenFeatureBuilder"/> instance to configure.</param> | ||
| /// <param name="domain">A domain identifier (e.g., tenant or environment).</param> | ||
| /// <param name="createConfiguration">A delegate that returns a domain-specific <see cref="Configuration"/> instance.</param> | ||
| /// <param name="resolveConfiguration">A delegate that resolves the domain-scoped <see cref="Configuration"/> from the service provider.</param> | ||
| /// <returns>The updated <see cref="OpenFeatureBuilder"/> instance.</returns> | ||
| private static OpenFeatureBuilder RegisterLaunchDarklyProviderForDomain( | ||
| OpenFeatureBuilder builder, | ||
| string domain, | ||
| Func<Configuration> createConfiguration, | ||
| Func<IServiceProvider, object, Configuration> resolveConfiguration) | ||
| { | ||
| // Perform early validation of the configuration to ensure it is valid before registration. | ||
| // This approach is consistent with the default (non-domain) registration path and helps fail fast on misconfiguration. | ||
| var config = createConfiguration(); | ||
|
|
||
| // Register the domain-scoped configuration as a keyed singleton. | ||
| builder.Services.TryAddKeyedSingleton(domain, (_, key) => config); | ||
|
|
||
| // Register the default configuration provider, which resolves the appropriate domain-scoped configuration | ||
| // using the default name selection policy defined in PolicyNameOptions. | ||
| // This enables resolving Configuration via serviceProvider.GetRequiredService<Configuration>() | ||
| // when no specific domain key is explicitly provided. | ||
| builder.Services.TryAddSingleton(provider => | ||
| { | ||
| var policy = provider.GetRequiredService<IOptions<PolicyNameOptions>>().Value; | ||
| var name = policy.DefaultNameSelector(provider); | ||
| return provider.GetRequiredKeyedService<Configuration>(name); | ||
| }); | ||
|
|
||
| // Register the domain-scoped provider instance. | ||
| return builder.AddProvider(domain, (serviceProvider, key) => new Provider(resolveConfiguration(serviceProvider, key))); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Creates a new <see cref="Configuration"/> using the specified SDK key and optional configuration delegate. | ||
| /// </summary> | ||
| /// <param name="stdKey">The SDK key used to initialize the configuration.</param> | ||
| /// <param name="configure">An optional delegate to customize the <see cref="ConfigurationBuilder"/>.</param> | ||
| /// <returns>A fully constructed <see cref="Configuration"/> instance.</returns> | ||
| private static Configuration CreateConfiguration(string stdKey, Action<ConfigurationBuilder> configure = null) | ||
arttonoyan marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| { | ||
| var configBuilder = Configuration.Builder(stdKey); | ||
| configure?.Invoke(configBuilder); | ||
| return configBuilder.Build(); | ||
| } | ||
| } | ||
| } | ||
41 changes: 41 additions & 0 deletions
41
...yInjection.Tests/LaunchDarkly.OpenFeature.ServerProvider.DependencyInjection.Tests.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,41 @@ | ||
| <Project Sdk="Microsoft.NET.Sdk"> | ||
|
|
||
| <PropertyGroup> | ||
| <!-- The BUILDFRAMEWORKS variable allows us to override the target frameworks with a | ||
| single framework that we are testing; this allows us to test with older SDK | ||
| versions that would error out if they saw any newer target frameworks listed | ||
| here, even if we weren't running those. | ||
|
|
||
| Tests need to run against a specific platform implementation. netstandard2.0 is | ||
| an API, and not a platform, so it is not included in this list. | ||
| Additional information: https://xunit.net/docs/why-no-netstandard | ||
|
|
||
| --> | ||
| <BuildFrameworks Condition="'$(BUILDFRAMEWORKS)' == ''">net8.0</BuildFrameworks> | ||
| <TargetFrameworks>$(BUILDFRAMEWORKS)</TargetFrameworks> | ||
|
|
||
| <GenerateMSBuildEditorConfigFile>false</GenerateMSBuildEditorConfigFile> | ||
| <IsPackable>false</IsPackable> | ||
| </PropertyGroup> | ||
|
|
||
| <ItemGroup> | ||
| <PackageReference Include="Microsoft.NET.Test.Sdk" Version="16.11.0" /> | ||
| <PackageReference Include="xunit" Version="2.4.1" /> | ||
| <PackageReference Include="xunit.runner.visualstudio" Version="2.4.3"> | ||
| <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets> | ||
| <PrivateAssets>all</PrivateAssets> | ||
| </PackageReference> | ||
| <PackageReference Include="Microsoft.TestPlatform.ObjectModel" Version="16.6.1" Condition="$(TargetFramework.StartsWith('net4')) AND '$(OS)' == 'Unix'" /> | ||
| <PackageReference Include="coverlet.collector" Version="3.1.0"> | ||
| <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets> | ||
| <PrivateAssets>all</PrivateAssets> | ||
| </PackageReference> | ||
| <PackageReference Include="Microsoft.Extensions.Logging" Version="8.0.1" /> | ||
| <PackageReference Include="Microsoft.Extensions.DependencyInjection" Version="8.0.1" /> | ||
| </ItemGroup> | ||
|
|
||
| <ItemGroup> | ||
| <ProjectReference Include="..\..\src\LaunchDarkly.OpenFeature.ServerProvider.DependencyInjection\LaunchDarkly.OpenFeature.ServerProvider.DependencyInjection.csproj" /> | ||
| </ItemGroup> | ||
|
|
||
| </Project> |
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.
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.
AddProviderregisters a transient factory method. I don't believe we should be resolving transient instances ofProviderhere. Please correct me if I am wrong, but my understanding is that LaunchDarkly strongly recommends instances ofLdClientto be singletons (for a given sdk-key/environment).Providerencapsulates the construction of theLdClientwithin its own constructor and thus, its lifetime should follow the same guideline. I believe a more correct approach would be to registerProvideras a singleton and resolve it using DI as part of the factory method. e.g.I'm assuming OpenFeature uses a transient factory method so that they don't dictate lifetime to provider implementations. Instead, each provider can be registered per their own guidance.
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.
You’re absolutely right that
LdClientis recommended to be used as a singleton. However, in this case, the factory method is registered as transient because its lifecycle is ultimately governed by the client that consumes it.In OpenFeature, the initialization happens here, and the
_featureApiis registered as a singleton. Consumers don’t interact with providers directly - they useIFeatureClient, which goes through this API (singleton). Given that flow, the effective usage aligns with the singleton recommendation.Registering the provider as transient also leaves flexibility: consumers can create and manage their own provider instance if they have specific scenarios requiring different lifetimes. This avoids OpenFeature itself enforcing a single pattern and instead allows SDK consumers to decide. In the standard usage (via
OpenFeatureandIFeatureClient), it will still behave as a singleton, which follows LaunchDarkly’s guidance.That said, if the LaunchDarkly team concludes that
Providermust always be a singleton for correctness, we can adapt by layering a singleton registration on top of the transient one. For example:Here
AddProviderwires up the necessary OpenFeature services internally (viaTryAdd), and the explicitAddSingletonoverrides the provider’s registration.My recommendation is to keep it as-is (registered as transient), for the reasons I outlined above. That said, this is an excellent highlight, and I fully agree it’s an important consideration that we should call out explicitly in the documentation.
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.
@dgioulakis what do you think?
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.
Hey @arttonoyan, sorry for the delay. I was out for a bit, and then came down with covid and was out some more.
I see what you're saying, although I'm not a maintainer of this repository. It would be best to get someone from LD to review. I think your second
AddSingletonwill not overwrite as you intend unless you specify the service type e.g.builder.Services.AddSingleton<FeatureProvider>(serviceProvider => new Provider(...)).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.
The LaunchDarkly Client should be a singleton but I think the approach to let OpenFeature dictate the provider lifetime here is okay. Especially since it in effect becomes a singleton.