open-feature / js-sdk

JavaScript SDK for OpenFeature
https://openfeature.dev
Apache License 2.0
168 stars 31 forks source link

implement named client support: open-feature/spec@4cf8229 #428

Closed beeme1mr closed 1 year ago

beeme1mr commented 1 year ago

Add named client support to the JS SDK based on the recent update to the spec: https://github.com/open-feature/spec/commit/4cf8229d23a88feeaadf8b066e5d3662b94ff262

This feature has already been implemented in Java. It can be used as a reference for the JS implementation. https://github.com/open-feature/java-sdk/pull/388

beeme1mr commented 1 year ago

https://github.com/open-feature/js-sdk/issues/424#issuecomment-1557110380

I believe we should be able to do something like this:

import { OpenFeature } from '@openfeature/js-sdk';

const CUSTOM_PROVIDER_NAME = "my-custom-provider";

// Default, unnamed provider
OpenFeature.setProvider(new EnvVarProvider());
// Optional named provider
OpenFeature.setProvider(CUSTOM_PROVIDER_NAME, new MyCustomProvider());

// An empty or invalid name would return the default provider
const envVarProvider = OpenFeature.getClient();

const myCustomProvider = OpenFeature.getClient(CUSTOM_PROVIDER_NAME );

// Evaluation using the envvar provider
const boolValue = await envVarProvider .getBooleanValue('boolFlag', false);

// Evaluation using the custom provider
const boolValue = await myCustomProvider .getBooleanValue('boolFlag', false);

The version argument is admittedly a bit strange. It was originally intended to be used a client-specific metadata that was passed to hooks. Unfortunately, that may make this new feature a bit confusing to end users.

lukas-reining commented 1 year ago

But wouldn't this be a problem because of the breaking change? If I get it right, this would remove the version from it, so current implementations would be broken.

beeme1mr commented 1 year ago

I talked to @lukas-reining about overloading the method and documenting the purpose of each property. The client's name will have meaning after this chance, but only if the name matches a registered provider. The version remains purely informational and doesn't affect runtime behavior.

The overload could look something like this.

getClient(context?: EvaluationContext)
getClient(name: string, context?: EvaluationContext)
getClient(name: string, version: string, context?: EvaluationContext)
getClient(nameOrContext?: string | EvaluationContext, VersionOrContext?: string | EvaluationContext, context?: EvaluationContext)
toddbaert commented 1 year ago

I talked to @lukas-reining about overloading the method and documenting the purpose of each property. The client's name will have meaning after this chance, but only if the name matches a registered provider. The version remains purely informational and doesn't affect runtime behavior.

The overload could look something like this.

getClient(context?: EvaluationContext)
getClient(name: string, context?: EvaluationContext)
getClient(name: string, version: string, context?: EvaluationContext)
getClient(nameOrContext?: string | EvaluationContext, VersionOrContext?: string | EvaluationContext, context?: EvaluationContext)

This all sounds right to me.