fix: race condition in test provider with suspense (#980)

Fixes an issues with the React `OpenFeatureTestProvider` where the provider was not ready until the next event loop tick when the `flagValueMap` was used. Also removes the initialization in the client in-memory provider, since it was only doing some un-needed validation inconsistent with the server provider. --------- Signed-off-by: Todd Baert <[email protected]>
open-feature · Aug 22, 2024 · 0f187fe · 0f187fe
1 parent cb4e56a
commit 0f187fe
Show file tree

Hide file tree

Showing 6 changed files with 81 additions and 90 deletions.
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
@@ -31,30 +31,17 @@ export class InMemoryProvider implements Provider {
     this._flagConfiguration = { ...flagConfiguration };
   }
 
-  async initialize(context?: EvaluationContext | undefined): Promise<void> {
-    try {
-      for (const key in this._flagConfiguration) {
-        this.resolveFlagWithReason(key, context);
-      }
-      this._context = context;
-    } catch (err) {
-      throw new GeneralError('initialization failure', { cause: err });
-    }
-  }
-
   /**
    * Overwrites the configured flags.
    * @param { FlagConfiguration } flagConfiguration new flag configuration
    */
   async putConfiguration(flagConfiguration: FlagConfiguration) {
-    const flagsChanged = Object.entries(flagConfiguration)
-      .filter(([key, value]) => this._flagConfiguration[key] !== value)
-      .map(([key]) => key);
-
-    this._flagConfiguration = { ...flagConfiguration };
-
     try {
-      await this.initialize(this._context);
+      const flagsChanged = Object.entries(flagConfiguration)
+        .filter(([key, value]) => this._flagConfiguration[key] !== value)
+        .map(([key]) => key);
+
+      this._flagConfiguration = { ...flagConfiguration };
       this.events.emit(ProviderEvents.ConfigurationChanged, { flagsChanged });
     } catch (err) {
       this.events.emit(ProviderEvents.Error);
@@ -98,8 +85,12 @@ export class InMemoryProvider implements Provider {
     return this.resolveAndCheckFlag<T>(flagKey, defaultValue, context || this._context, logger);
   }
 
-  private resolveAndCheckFlag<T extends JsonValue | FlagValueType>(flagKey: string,
-    defaultValue: T, context?: EvaluationContext, logger?: Logger): ResolutionDetails<T> {
+  private resolveAndCheckFlag<T extends JsonValue | FlagValueType>(
+    flagKey: string,
+    defaultValue: T,
+    context?: EvaluationContext,
+    logger?: Logger,
+  ): ResolutionDetails<T> {
     if (!(flagKey in this._flagConfiguration)) {
       const message = `no flag found with key ${flagKey}`;
       logger?.debug(message);

diff --git a/packages/client/test/in-memory-provider.spec.ts b/packages/client/test/in-memory-provider.spec.ts
@@ -1,44 +1,7 @@
-import { FlagNotFoundError, GeneralError, InMemoryProvider, ProviderEvents, StandardResolutionReasons, TypeMismatchError } from '../src';
-import { FlagConfiguration } from '../src/provider/in-memory-provider/flag-configuration';
+import { FlagNotFoundError, InMemoryProvider, ProviderEvents, StandardResolutionReasons, TypeMismatchError } from '../src';
 import { VariantNotFoundError } from '../src/provider/in-memory-provider/variant-not-found-error';
 
 describe('in-memory provider', () => {
-  describe('initialize', () => {
-    it('Should not throw for valid flags', async () => {
-      const booleanFlagSpec = {
-        'a-boolean-flag': {
-          variants: {
-            on: true,
-            off: false,
-          },
-          disabled: false,
-          defaultVariant: 'on',
-        },
-      };
-      const provider = new InMemoryProvider(booleanFlagSpec);
-      await provider.initialize();
-    });
-
-    it('Should throw on invalid flags', async () => {
-      const throwingFlagSpec: FlagConfiguration = {
-        'a-boolean-flag': {
-          variants: {
-            on: true,
-            off: false,
-          },
-          disabled: false,
-          defaultVariant: 'on',
-          contextEvaluator: () => {
-            throw new GeneralError('context eval error');
-          },
-        },
-      };
-      const provider = new InMemoryProvider(throwingFlagSpec);
-      const someContext = {};
-      await expect(provider.initialize(someContext)).rejects.toThrow();
-    });
-  });
-
   describe('boolean flags', () => {
     const provider = new InMemoryProvider({});
     it('resolves to default variant with reason static', async () => {
@@ -531,8 +494,6 @@ describe('in-memory provider', () => {
         },
       });
 
-      await provider.initialize();
-
       const firstResolution = provider.resolveStringEvaluation('some-flag', 'deafaultFirstResolution');
 
       expect(firstResolution).toEqual({
@@ -577,8 +538,6 @@ describe('in-memory provider', () => {
 
       const provider = new InMemoryProvider(flagsSpec);
 
-      await provider.initialize();
-
       // I passed configuration by reference, so maybe I can mess
       // with it behind the providers back!
       flagsSpec['some-flag'] = substituteSpec;

diff --git a/packages/react/package.json b/packages/react/package.json
@@ -46,7 +46,7 @@
   },
   "homepage": "https://github.com/open-feature/js-sdk#readme",
   "peerDependencies": {
-    "@openfeature/web-sdk": "^1.0.2",
+    "@openfeature/web-sdk": "^1.2.2",
     "react": ">=16.8.0"
   },
   "devDependencies": {

diff --git a/packages/react/src/provider/test-provider.tsx b/packages/react/src/provider/test-provider.tsx
@@ -1,5 +1,4 @@
 import {
-  EvaluationContext,
   InMemoryProvider,
   JsonValue,
   NOOP_PROVIDER,
@@ -43,6 +42,19 @@ type TestProviderProps = Omit<React.ComponentProps<typeof OpenFeatureProvider>,
 
 // internal provider which is basically the in-memory provider with a simpler config and some optional fake delays
 class TestProvider extends InMemoryProvider {
+
+  // initially make this undefined, we still set it if a delay is specified
+  // eslint-disable-next-line @typescript-eslint/ban-ts-comment
+  // @ts-ignore - For maximum compatibility with previous versions, we ignore a possible TS error here,
+  // since "initialize" was previously defined in superclass.
+  // We can safely remove this ts-ignore in a few versions
+  initialize: Provider['initialize'] = undefined;
+
+  // "place-holder" init function which we only assign if want a delay
+  private delayedInitialize = async () => {
+    await new Promise<void>((resolve) => setTimeout(resolve, this.delay));
+  };
+
   constructor(
     flagValueMap: FlagValueMap,
     private delay = 0,
@@ -61,10 +73,8 @@ class TestProvider extends InMemoryProvider {
       };
     }, {});
     super(flagConfig);
-  }
-
-  async initialize(context?: EvaluationContext | undefined): Promise<void> {
-    await Promise.all([super.initialize(context), new Promise<void>((resolve) => setTimeout(resolve, this.delay))]);
+    // only define and init if there's a non-zero delay specified
+    this.initialize = this.delay ? this.delayedInitialize.bind(this) : undefined;
   }
 
   async onContextChange() {

diff --git a/packages/react/test/test-provider.spec.tsx b/packages/react/test/test-provider.spec.tsx
@@ -6,8 +6,8 @@ import { OpenFeatureTestProvider, useFlag } from '../src';
 
 const FLAG_KEY = 'thumbs';
 
-function TestComponent() {
-  const { value: thumbs, reason } = useFlag(FLAG_KEY, false);
+function TestComponent(props: { suspend: boolean }) {
+  const { value: thumbs, reason } = useFlag(FLAG_KEY, false, { suspend: props.suspend });
   return (
     <>
       <div>{thumbs ? '👍' : '👎'}</div>
@@ -21,22 +21,22 @@ describe('OpenFeatureTestProvider', () => {
     it('renders default', async () => {
       render(
         <OpenFeatureTestProvider>
-          <TestComponent />
+          <TestComponent suspend={false} />
         </OpenFeatureTestProvider>,
       );
-      expect(await screen.findByText('👎')).toBeInTheDocument();
+      expect(screen.getByText('👎')).toBeInTheDocument();
     });
   });
 
   describe('flagValueMap set', () => {
     it('renders value from map', async () => {
       render(
         <OpenFeatureTestProvider flagValueMap={{ [FLAG_KEY]: true }}>
-          <TestComponent />
+          <TestComponent suspend={false} />
         </OpenFeatureTestProvider>,
       );
 
-      expect(await screen.findByText('👍')).toBeInTheDocument();
+      expect(screen.getByText('👍')).toBeInTheDocument();
     });
   });
 
@@ -45,22 +45,21 @@ describe('OpenFeatureTestProvider', () => {
       const delay = 100;
       render(
         <OpenFeatureTestProvider delayMs={delay} flagValueMap={{ [FLAG_KEY]: true }}>
-          <TestComponent />
+          <TestComponent suspend={false} />
         </OpenFeatureTestProvider>,
       );
 
       // should only be resolved after delay
-      expect(await screen.findByText('👎')).toBeInTheDocument();
+      expect(screen.getByText('👎')).toBeInTheDocument();
       await new Promise((resolve) => setTimeout(resolve, delay * 2));
-      expect(await screen.findByText('👍')).toBeInTheDocument();
+      expect(screen.getByText('👍')).toBeInTheDocument();
     });
   });
 
   describe('provider set', () => {
     const reason = 'MY_REASON';
 
     it('renders provider-returned value', async () => {
-
       class MyTestProvider implements Partial<Provider> {
         resolveBooleanEvaluation(): ResolutionDetails<boolean> {
           return {
@@ -73,27 +72,60 @@ describe('OpenFeatureTestProvider', () => {
 
       render(
         <OpenFeatureTestProvider provider={new MyTestProvider()}>
-          <TestComponent />
+          <TestComponent suspend={false} />
         </OpenFeatureTestProvider>,
       );
 
-      expect(await screen.findByText('👍')).toBeInTheDocument();
-      expect(await screen.findByText(/reason/)).toBeInTheDocument();
+      expect(screen.getByText('👍')).toBeInTheDocument();
+      expect(screen.getByText(new RegExp(`${reason}`))).toBeInTheDocument();
     });
 
     it('falls back to no-op for missing methods', async () => {
-
-      class MyEmptyProvider implements Partial<Provider> {
-      }
+      class MyEmptyProvider implements Partial<Provider> {}
 
       render(
         <OpenFeatureTestProvider provider={new MyEmptyProvider()}>
-          <TestComponent />
+          <TestComponent suspend={false} />
         </OpenFeatureTestProvider>,
       );
 
-      expect(await screen.findByText('👎')).toBeInTheDocument();
-      expect(await screen.findByText(/No-op/)).toBeInTheDocument();
+      expect(screen.getByText('👎')).toBeInTheDocument();
+      expect(screen.getByText(/No-op/)).toBeInTheDocument();
+    });
+  });
+
+  describe('component under test suspends', () => {
+    describe('delay non-zero', () => {
+      it('renders fallback then value after delay', async () => {
+        const delay = 100;
+        render(
+          <OpenFeatureTestProvider delayMs={delay} flagValueMap={{ [FLAG_KEY]: true }}>
+            <React.Suspense fallback={<>🕒</>}>
+              <TestComponent suspend={true} />
+            </React.Suspense>
+          </OpenFeatureTestProvider>,
+        );
+
+        // should initially show fallback, then resolve
+        expect(screen.getByText('🕒')).toBeInTheDocument();
+        await new Promise((resolve) => setTimeout(resolve, delay * 2));
+        expect(screen.getByText('👍')).toBeInTheDocument();
+      });
+    });
+
+    describe('delay zero', () => {
+      it('renders value immediately', async () => {
+        render(
+          <OpenFeatureTestProvider delayMs={0} flagValueMap={{ [FLAG_KEY]: true }}>
+            <React.Suspense fallback={<>🕒</>}>
+              <TestComponent suspend={true} />
+            </React.Suspense>
+          </OpenFeatureTestProvider>,
+        );
+
+        // should resolve immediately since delay is falsy
+        expect(screen.getByText('👍')).toBeInTheDocument();
+      });
     });
   });
 });
diff --git a/packages/react/test/test.utils.ts b/packages/react/test/test.utils.ts
@@ -1,4 +1,4 @@
-import { EvaluationContext, InMemoryProvider } from '@openfeature/web-sdk';
+import { InMemoryProvider } from '@openfeature/web-sdk';
 
 export class TestingProvider extends InMemoryProvider {
     constructor(
@@ -9,9 +9,8 @@ export class TestingProvider extends InMemoryProvider {
     }
 
     // artificially delay our init (delaying PROVIDER_READY event)
-    async initialize(context?: EvaluationContext | undefined): Promise<void> {
+    async initialize(): Promise<void> {
       await new Promise((resolve) => setTimeout(resolve, this.delay));
-      return super.initialize(context);
     }
 
     // artificially delay context changes