feat: maintain state in SDK, add RECONCILING (#795)

Implements the newest spec changes [here](open-feature/spec#241). - Provider state is deprecated (state is now maintained in the SDK itself). This is non-breaking, we just ignore the provider's own state now - add RECONCILING events and state, fire related events with provider's `on context change` - add FATAL state The new tests should help you understand the impact of the changes. --------- Signed-off-by: Todd Baert <[email protected]>
open-feature · Mar 1, 2024 · cfb0a69 · cfb0a69
1 parent 016908c
commit cfb0a69
Show file tree

Hide file tree

Showing 40 changed files with 731 additions and 372 deletions.
diff --git a/jest.config.ts b/jest.config.ts
@@ -174,7 +174,7 @@ export default {
   ],
 
   // Use this configuration option to add custom reporters to Jest
-  reporters: [['github-actions', { silent: false }], 'summary'],
+  reporters: ['default', ['github-actions', { silent: false }], 'summary'],
 
   // Automatically reset mock state before every test
   // resetMocks: false,

diff --git a/package.json b/package.json
@@ -4,7 +4,7 @@
   "private": true,
   "description": "OpenFeature SDK for JavaScript",
   "scripts": {
-    "test": "jest --selectProjects=shared --selectProjects=server --selectProjects=client --verbose",
+    "test": "jest --selectProjects=shared --selectProjects=server --selectProjects=client --silent",
     "e2e-server": "git submodule update --init --recursive && shx cp test-harness/features/evaluation.feature packages/server/e2e/features && jest --selectProjects=server-e2e --verbose",
     "e2e-client": "git submodule update --init --recursive && shx cp test-harness/features/evaluation.feature packages/client/e2e/features && jest --selectProjects=client-e2e --verbose",
     "e2e": "npm run e2e-server && npm run e2e-client",

diff --git a/packages/client/README.md b/packages/client/README.md
@@ -279,7 +279,6 @@ import {
   Logger,
   Provider,
   ProviderEventEmitter,
-  ProviderStatus,
   ResolutionDetails
 } from '@openfeature/web-sdk';
 
@@ -309,8 +308,6 @@ class MyProvider implements Provider {
     // reconcile the provider's cached flags, if applicable
   }
 
-  status?: ProviderStatus | undefined;
-
   // implement with "new OpenFeatureEventEmitter()", and use "emit()" to emit events
   events?: ProviderEventEmitter<AnyProviderEvent> | undefined; 
 

diff --git a/packages/client/e2e/step-definitions/evaluation.spec.ts b/packages/client/e2e/step-definitions/evaluation.spec.ts
@@ -1,13 +1,13 @@
-import { defineFeature, loadFeature } from 'jest-cucumber';
 import {
-  JsonValue,
-  JsonObject,
-  EvaluationDetails,
   EvaluationContext,
+  EvaluationDetails,
+  JsonObject,
+  JsonValue,
   ResolutionDetails,
   StandardResolutionReasons,
 } from '@openfeature/core';
-import { OpenFeature, ProviderEvents, InMemoryProvider } from '../../src';
+import { defineFeature, loadFeature } from 'jest-cucumber';
+import { InMemoryProvider, OpenFeature } from '../../src';
 import flagConfiguration from './flags-config';
 // load the feature file.
 const feature = loadFeature('packages/client/e2e/features/evaluation.feature');
@@ -18,15 +18,12 @@ const client = OpenFeature.getClient();
 const givenAnOpenfeatureClientIsRegisteredWithCacheDisabled = (
   given: (stepMatcher: string, stepDefinitionCallback: () => void) => void
 ) => {
-  OpenFeature.setProvider(new InMemoryProvider(flagConfiguration));
   given('a provider is registered with cache disabled', () => undefined);
 };
 
 defineFeature(feature, (test) => {
-  beforeAll((done) => {
-    client.addHandler(ProviderEvents.Ready, async () => {
-      done();
-    });
+  beforeAll(async () => {
+    await OpenFeature.setProvider(new InMemoryProvider(flagConfiguration));
   });
 
   afterAll(async () => {

diff --git a/packages/client/src/client/client.ts b/packages/client/src/client/client.ts
@@ -1,10 +1,11 @@
-import { ClientMetadata, EvaluationLifeCycle, Eventing, ManageLogger, ProviderStatus } from '@openfeature/core';
+import { ClientMetadata, EvaluationLifeCycle, Eventing, ManageLogger } from '@openfeature/core';
 import { Features } from '../evaluation';
+import { ProviderStatus } from '../provider';
 
 export interface Client extends EvaluationLifeCycle<Client>, Features, ManageLogger<Client>, Eventing {
   readonly metadata: ClientMetadata;
   /**
    * Returns the status of the associated provider.
    */
   readonly providerStatus: ProviderStatus;
-}
+}
diff --git a/packages/client/src/client/open-feature-client.ts b/packages/client/src/client/open-feature-client.ts
@@ -10,7 +10,7 @@ import {
   JsonValue,
   Logger,
   OpenFeatureError,
-  ProviderStatus,
+  ProviderFatalError,
   ResolutionDetails,
   SafeLogger,
   StandardResolutionReasons,
@@ -21,8 +21,9 @@ import { ProviderEvents } from '../events';
 import { InternalEventEmitter } from '../events/internal/internal-event-emitter';
 import { Hook } from '../hooks';
 import { OpenFeature } from '../open-feature';
-import { Provider } from '../provider';
+import { Provider, ProviderStatus } from '../provider';
 import { Client } from './client';
+import { ProviderNotReadyError } from '@openfeature/core';
 
 type OpenFeatureClientOptions = {
   /**
@@ -41,6 +42,7 @@ export class OpenFeatureClient implements Client {
     // functions are passed here to make sure that these values are always up to date,
     // and so we don't have to make these public properties on the API class.
     private readonly providerAccessor: () => Provider,
+    private readonly providerStatusAccessor: () => ProviderStatus,
     private readonly emitterAccessor: () => InternalEventEmitter,
     private readonly globalLogger: () => Logger,
     private readonly options: OpenFeatureClientOptions,
@@ -57,12 +59,12 @@ export class OpenFeatureClient implements Client {
   }
 
   get providerStatus(): ProviderStatus {
-    return this.providerAccessor()?.status || ProviderStatus.READY;
+    return this.providerStatusAccessor();
   }
 
   addHandler(eventType: ProviderEvents, handler: EventHandler): void {
     this.emitterAccessor().addHandler(eventType, handler);
-    const shouldRunNow = statusMatchesEvent(eventType, this._provider.status);
+    const shouldRunNow = statusMatchesEvent(eventType, this.providerStatus);
 
     if (shouldRunNow) {
       // run immediately, we're in the matching state
@@ -206,6 +208,13 @@ export class OpenFeatureClient implements Client {
 
     try {
       this.beforeHooks(allHooks, hookContext, options);
+
+      // short circuit evaluation entirely if provider is in a bad state
+      if (this.providerStatus === ProviderStatus.NOT_READY) {
+        throw new ProviderNotReadyError('provider has not yet initialized');
+      } else if (this.providerStatus === ProviderStatus.FATAL) {
+        throw new ProviderFatalError('provider is in an irrecoverable error state');
+      }
 
       // run the referenced resolver, binding the provider.
       const resolution = resolver.call(this._provider, flagKey, defaultValue, context, this._logger);

diff --git a/packages/client/src/open-feature.ts b/packages/client/src/open-feature.ts
@@ -1,15 +1,17 @@
 import {
+  ClientProviderStatus,
   EvaluationContext,
   GenericEventEmitter,
   ManageContext,
   OpenFeatureCommonAPI,
+  ProviderWrapper,
   objectOrUndefined,
   stringOrUndefined,
 } from '@openfeature/core';
 import { Client, OpenFeatureClient } from './client';
 import { OpenFeatureEventEmitter, ProviderEvents } from './events';
 import { Hook } from './hooks';
-import { NOOP_PROVIDER, Provider } from './provider';
+import { NOOP_PROVIDER, Provider, ProviderStatus } from './provider';
 
 // use a symbol as a key for the global singleton
 const GLOBAL_OPENFEATURE_API_KEY = Symbol.for('@openfeature/web-sdk/api');
@@ -19,14 +21,16 @@ type OpenFeatureGlobal = {
 };
 type DomainRecord = {
   domain?: string;
-  provider: Provider;
+  wrapper: ProviderWrapper<Provider, ClientProviderStatus>;
 };
 
 const _globalThis = globalThis as OpenFeatureGlobal;
 
-export class OpenFeatureAPI extends OpenFeatureCommonAPI<Provider, Hook> implements ManageContext<Promise<void>> {
-  protected _events: GenericEventEmitter<ProviderEvents> = new OpenFeatureEventEmitter();
-  protected _defaultProvider: Provider = NOOP_PROVIDER;
+export class OpenFeatureAPI extends OpenFeatureCommonAPI<ClientProviderStatus, Provider, Hook> implements ManageContext<Promise<void>> {
+  protected _statusEnumType: typeof ProviderStatus = ProviderStatus;
+  protected _apiEmitter: GenericEventEmitter<ProviderEvents> = new OpenFeatureEventEmitter();
+  protected _defaultProvider: ProviderWrapper<Provider, ClientProviderStatus> = new ProviderWrapper(NOOP_PROVIDER, ProviderStatus.NOT_READY, this._statusEnumType);
+  protected _domainScopedProviders: Map<string, ProviderWrapper<Provider, ClientProviderStatus>> = new Map();
   protected _createEventEmitter = () => new OpenFeatureEventEmitter();
 
   private constructor() {
@@ -49,6 +53,14 @@ export class OpenFeatureAPI extends OpenFeatureCommonAPI<Provider, Hook> impleme
     return instance;
   }
 
+  private getProviderStatus(domain?: string): ProviderStatus {
+    if (!domain) {
+      return this._defaultProvider.status;
+    }
+
+    return this._domainScopedProviders.get(domain)?.status ?? this._defaultProvider.status;
+  }
+
   /**
    * Sets the evaluation context globally.
    * This will be used by all providers that have not bound to a domain.
@@ -73,11 +85,11 @@ export class OpenFeatureAPI extends OpenFeatureCommonAPI<Provider, Hook> impleme
     const context = objectOrUndefined<T>(domainOrContext) ?? objectOrUndefined(contextOrUndefined) ?? {};
 
     if (domain) {
-      const provider = this._domainScopedProviders.get(domain);
-      if (provider) {
+      const wrapper = this._domainScopedProviders.get(domain);
+      if (wrapper) {
         const oldContext = this.getContext(domain);
         this._domainScopedContext.set(domain, context);
-        await this.runProviderContextChangeHandler(domain, provider, oldContext, context);
+        await this.runProviderContextChangeHandler(domain, wrapper, oldContext, context);
       } else {
         this._domainScopedContext.set(domain, context);
       }
@@ -88,19 +100,19 @@ export class OpenFeatureAPI extends OpenFeatureCommonAPI<Provider, Hook> impleme
       // collect all providers that are using the default context (not bound to a domain)
       const unboundProviders: DomainRecord[] = Array.from(this._domainScopedProviders.entries())
         .filter(([domain]) => !this._domainScopedContext.has(domain))
-        .reduce<DomainRecord[]>((acc, [domain, provider]) => {
-          acc.push({ domain, provider });
+        .reduce<DomainRecord[]>((acc, [domain, wrapper]) => {
+          acc.push({ domain, wrapper });
           return acc;
         }, []);
 
-      const allProviders: DomainRecord[] = [
+      const allDomainRecords: DomainRecord[] = [
         // add in the default (no domain)
-        { domain: undefined, provider: this._defaultProvider },
+        { domain: undefined, wrapper: this._defaultProvider },
         ...unboundProviders,
       ];
       await Promise.all(
-        allProviders.map((tuple) =>
-          this.runProviderContextChangeHandler(tuple.domain, tuple.provider, oldContext, context),
+        allDomainRecords.map((dm) =>
+          this.runProviderContextChangeHandler(dm.domain, dm.wrapper, oldContext, context),
         ),
       );
     }
@@ -143,12 +155,12 @@ export class OpenFeatureAPI extends OpenFeatureCommonAPI<Provider, Hook> impleme
   async clearContext(domainOrUndefined?: string): Promise<void> {
     const domain = stringOrUndefined(domainOrUndefined);
     if (domain) {
-      const provider = this._domainScopedProviders.get(domain);
-      if (provider) {
+      const wrapper = this._domainScopedProviders.get(domain);
+      if (wrapper) {
         const oldContext = this.getContext(domain);
         this._domainScopedContext.delete(domain);
         const newContext = this.getContext();
-        await this.runProviderContextChangeHandler(domain, provider, oldContext, newContext);
+        await this.runProviderContextChangeHandler(domain, wrapper, oldContext, newContext);
       } else {
         this._domainScopedContext.delete(domain);
       }
@@ -186,6 +198,7 @@ export class OpenFeatureAPI extends OpenFeatureCommonAPI<Provider, Hook> impleme
       // functions are passed here to make sure that these values are always up to date,
       // and so we don't have to make these public properties on the API class.
       () => this.getProviderForClient(domain),
+      () => this.getProviderStatus(domain),
       () => this.buildAndCacheEventEmitterForClient(domain),
       () => this._logger,
       { domain, version },
@@ -203,28 +216,45 @@ export class OpenFeatureAPI extends OpenFeatureCommonAPI<Provider, Hook> impleme
 
   private async runProviderContextChangeHandler(
     domain: string | undefined,
-    provider: Provider,
+    wrapper: ProviderWrapper<Provider, ClientProviderStatus>,
     oldContext: EvaluationContext,
     newContext: EvaluationContext,
   ): Promise<void> {
-    const providerName = provider.metadata.name;
+    // this should always be set according to the typings, but let's be defensive considering JS
+    const providerName = wrapper.provider?.metadata?.name || 'unnamed-provider';
+
     try {
-      await provider.onContextChange?.(oldContext, newContext);
-
-      // only run the event handlers if the onContextChange method succeeded
-      this.getAssociatedEventEmitters(domain).forEach((emitter) => {
-        emitter?.emit(ProviderEvents.ContextChanged, { clientName: domain, domain, providerName });
-      });
-      this._events?.emit(ProviderEvents.ContextChanged, { clientName: domain, domain, providerName });
+      if (typeof wrapper.provider.onContextChange === 'function') {
+        wrapper.incrementPendingContextChanges();
+        wrapper.status = this._statusEnumType.RECONCILING;
+        this.getAssociatedEventEmitters(domain).forEach((emitter) => {
+          emitter?.emit(ProviderEvents.Reconciling, { domain, providerName });
+        });
+        this._apiEmitter?.emit(ProviderEvents.Reconciling, { domain, providerName });
+        await wrapper.provider.onContextChange(oldContext, newContext);
+        wrapper.decrementPendingContextChanges();
+      }
+      // only run the event handlers, and update the state if the onContextChange method succeeded
+      wrapper.status = this._statusEnumType.READY;
+      if (wrapper.allContextChangesSettled) {
+        this.getAssociatedEventEmitters(domain).forEach((emitter) => {
+          emitter?.emit(ProviderEvents.ContextChanged, { clientName: domain, domain, providerName });
+        });
+        this._apiEmitter?.emit(ProviderEvents.ContextChanged, { clientName: domain, domain, providerName });
+      }
     } catch (err) {
       // run error handlers instead
-      const error = err as Error | undefined;
-      const message = `Error running ${provider?.metadata?.name}'s context change handler: ${error?.message}`;
-      this._logger?.error(`${message}`, err);
-      this.getAssociatedEventEmitters(domain).forEach((emitter) => {
-        emitter?.emit(ProviderEvents.Error, { clientName: domain, domain, providerName, message });
-      });
-      this._events?.emit(ProviderEvents.Error, { clientName: domain, domain, providerName, message });
+      wrapper.decrementPendingContextChanges();
+      wrapper.status = this._statusEnumType.ERROR;
+      if (wrapper.allContextChangesSettled) {
+        const error = err as Error | undefined;
+        const message = `Error running ${providerName}'s context change handler: ${error?.message}`;
+        this._logger?.error(`${message}`, err);
+        this.getAssociatedEventEmitters(domain).forEach((emitter) => {
+          emitter?.emit(ProviderEvents.Error, { clientName: domain, domain, providerName, message });
+        });
+        this._apiEmitter?.emit(ProviderEvents.Error, { clientName: domain, domain, providerName, message });
+      }
     }
   }
 }

diff --git a/packages/client/src/provider/in-memory-provider/in-memory-provider.ts b/packages/client/src/provider/in-memory-provider/in-memory-provider.ts
@@ -9,7 +9,6 @@ import {
   ResolutionDetails,
   StandardResolutionReasons,
   TypeMismatchError,
-  ProviderStatus,
 } from '@openfeature/core';
 import { Provider } from '../provider';
 import { OpenFeatureEventEmitter, ProviderEvents } from '../../events';
@@ -22,7 +21,6 @@ import { VariantNotFoundError } from './variant-not-found-error';
 export class InMemoryProvider implements Provider {
   public readonly events = new OpenFeatureEventEmitter();
   public readonly runsOn = 'client';
-  status: ProviderStatus = ProviderStatus.NOT_READY;
   readonly metadata = {
     name: 'in-memory',
   } as const;
@@ -35,18 +33,12 @@ export class InMemoryProvider implements Provider {
 
   async initialize(context?: EvaluationContext | undefined): Promise<void> {
     try {
-
       for (const key in this._flagConfiguration) {
         this.resolveFlagWithReason(key, context);
       }
-
       this._context = context;
-      // set the provider's state, but don't emit events manually;
-      // the SDK does this based on the resolution/rejection of the init promise
-      this.status = ProviderStatus.READY;
-    } catch (error) {
-      this.status = ProviderStatus.ERROR;
-      throw error;
+    } catch (err) {
+      throw new Error('initialization failure', { cause: err });
     }
   }
 
@@ -59,16 +51,10 @@ export class InMemoryProvider implements Provider {
       .filter(([key, value]) => this._flagConfiguration[key] !== value)
       .map(([key]) => key);
 
-    this.status = ProviderStatus.STALE;
-    this.events.emit(ProviderEvents.Stale);
-
     this._flagConfiguration = { ...flagConfiguration };
-    this.events.emit(ProviderEvents.ConfigurationChanged, { flagsChanged });
-
     try {
       await this.initialize(this._context);
-      // we need to emit our own events in this case, since it's not part of the init flow.
-      this.events.emit(ProviderEvents.Ready);
+      this.events.emit(ProviderEvents.ConfigurationChanged, { flagsChanged });
     } catch (err) {
       this.events.emit(ProviderEvents.Error);
       throw err;
@@ -165,9 +151,7 @@ export class InMemoryProvider implements Provider {
 
     const value = variant && flagSpec?.variants[variant];
 
-    const evalReason = isContextEval ? StandardResolutionReasons.TARGETING_MATCH : StandardResolutionReasons.STATIC;
-
-    const reason = this.status === ProviderStatus.STALE ? StandardResolutionReasons.CACHED : evalReason;
+    const reason = isContextEval ? StandardResolutionReasons.TARGETING_MATCH : StandardResolutionReasons.STATIC;
 
     return {
       value: value as T,