fix!: Improve error handling when environment fetch fails. Minor type safety breaking changes

sdk/polling_manager.ts

@@ -1,20 +1,27 @@
 import Flagsmith from './index.js';
+import { Logger } from 'pino';
 
 export class EnvironmentDataPollingManager {
     private interval?: NodeJS.Timeout;
     private main: Flagsmith;
     private refreshIntervalSeconds: number;
+    private logger: Logger;
 
-    constructor(main: Flagsmith, refreshIntervalSeconds: number) {
+    constructor(main: Flagsmith, refreshIntervalSeconds: number, logger: Logger) {
         this.main = main;
         this.refreshIntervalSeconds = refreshIntervalSeconds;
+        this.logger = logger;
     }
 
     start() {
         const updateEnvironment = () => {
             if (this.interval) clearInterval(this.interval);
             this.interval = setInterval(async () => {
-                await this.main.updateEnvironment();
+                try {
+                    await this.main.updateEnvironment();
+                } catch (error) {
+                    this.logger.error('failed to poll environment', error);
+                }
             }, this.refreshIntervalSeconds * 1000);
         };
         updateEnvironment();

sdk/types.ts

@@ -40,7 +40,7 @@ export interface FlagsmithConfig {
     enableAnalytics?: boolean;
     defaultFlagHandler?: (featureName: string) => DefaultFlag;
     cache?: FlagsmithCache;
-    onEnvironmentChange?: (error: Error | null, result: EnvironmentModel) => void;
+    onEnvironmentChange?: (error: Error | null, result?: EnvironmentModel) => void;
     logger?: Logger;
     offlineMode?: boolean;
     offlineHandler?: BaseOfflineHandler;

sdk/utils.ts

@@ -48,23 +48,56 @@ export const retryFetch = (
     timeoutMs: number = 10, // set an overall timeout for this function
     customFetch: Fetch,
 ): Promise<Response> => {
-    return new Promise((resolve, reject) => {
-        const retryWrapper = (n: number) => {
-            customFetch(url, {
+    const retryWrapper = async (n: number): Promise<Response> => {
+        try {
+            return await customFetch(url, {
                 ...fetchOptions,
                 signal: AbortSignal.timeout(timeoutMs)
-            })
-            .then(res => resolve(res))
-            .catch(async err => {
-                if (n > 0) {
-                    await delay(1000);
-                    retryWrapper(--n);
-                } else {
-                    reject(err);
-                }
             });
-        };
-
-        retryWrapper(retries);
-    });
+        } catch (e) {
+            if (n > 0) {
+                await delay(1000);
+                return await retryWrapper(n - 1);
+            } else {
+                throw e;
+            }
+        }
+    };
+    return retryWrapper(retries);
 };
+
+/**
+ * A deferred promise can be resolved or rejected outside its creation scope.
+ *
+ * @template T The type of the value that the deferred promise will resolve to.
+ *
+ * @example
+ * const deferred = new Deferred<string>()
+ *
+ * // Pass the promise somewhere
+ * performAsyncOperation(deferred.promise)
+ *
+ * // Resolve it when ready from anywhere
+ * deferred.resolve("Operation completed")
+ * deferred.failed("Error")
+ */
+export class Deferred<T> {
+    public readonly promise: Promise<T>;
+    private resolvePromise!: (value: T | PromiseLike<T>) => void;
+    private rejectPromise!: (reason?: unknown) => void;
+
+    constructor(initial?: T) {
+        this.promise = new Promise<T>((resolve, reject) => {
+            this.resolvePromise = resolve;
+            this.rejectPromise = reject;
+        });
+    }
+
+    public resolve(value: T | PromiseLike<T>): void {
+        this.resolvePromise(value);
+    }
+
+    public reject(reason?: unknown): void {
+        this.rejectPromise(reason);
+    }
+}

tests/sdk/flagsmith-cache.test.ts

@@ -61,14 +61,16 @@ test('test_get_environment_flags_uses_local_environment_when_available', async (
   const cache = new TestCache();
   const set = vi.spyOn(cache, 'set');
 
-  const flg = flagsmith({ cache });
+  const flg = flagsmith({ cache, environmentKey: 'ser.key', enableLocalEvaluation: true });
   const model = environmentModel(JSON.parse(environmentJSON));
-  flg.environment = model;
+  const getEnvironment = vi.spyOn(flg, 'getEnvironment')
+  getEnvironment.mockResolvedValue(model)
 
-  const allFlags = await (await flg.getEnvironmentFlags()).allFlags();
+  const allFlags = (await flg.getEnvironmentFlags()).allFlags();
 
   expect(set).toBeCalled();
   expect(fetch).toBeCalledTimes(0);
+  expect(getEnvironment).toBeCalledTimes(1);
   expect(allFlags[0].enabled).toBe(model.featureStates[0].enabled);
   expect(allFlags[0].value).toBe(model.featureStates[0].getValue());
   expect(allFlags[0].featureName).toBe(model.featureStates[0].feature.name);

tests/sdk/flagsmith-environment-flags.test.ts

@@ -20,20 +20,6 @@ test('test_get_environment_flags_calls_api_when_no_local_environment', async ()
   expect(allFlags[0].featureName).toBe('some_feature');
 });
 
-test('test_get_environment_flags_uses_local_environment_when_available', async () => {
-  fetch.mockResolvedValue(new Response(flagsJSON));
-
-  const flg = flagsmith();
-  const model = environmentModel(JSON.parse(environmentJSON));
-  flg.environment = model;
-
-  const allFlags = await (await flg.getEnvironmentFlags()).allFlags();
-  expect(fetch).toBeCalledTimes(0);
-  expect(allFlags[0].enabled).toBe(model.featureStates[0].enabled);
-  expect(allFlags[0].value).toBe(model.featureStates[0].getValue());
-  expect(allFlags[0].featureName).toBe(model.featureStates[0].feature.name);
-});
-
 test('test_default_flag_is_used_when_no_environment_flags_returned', async () => {
   fetch.mockResolvedValue(new Response(JSON.stringify([])));
 

tests/sdk/flagsmith-identity-flags.test.ts

@@ -1,5 +1,13 @@
 import Flagsmith from '../../sdk/index.js';
-import { fetch, environmentJSON, flagsmith, identitiesJSON, identityWithTransientTraitsJSON, transientIdentityJSON } from './utils.js';
+import {
+  fetch,
+  environmentJSON,
+  flagsmith,
+  identitiesJSON,
+  identityWithTransientTraitsJSON,
+  transientIdentityJSON,
+  badFetch
+} from './utils.js';
 import { DefaultFlag } from '../../sdk/models.js';
 
 vi.mock('../../sdk/polling_manager');
@@ -125,7 +133,6 @@ test('test_default_flag_is_used_when_no_identity_flags_returned_and_no_custom_de
   expect(flag.enabled).toBe(false);
 });
 
-
 test('test_get_identity_flags_multivariate_value_with_local_evaluation_enabled', async () => {
   fetch.mockResolvedValue(new Response(environmentJSON));
   const identifier = 'identifier';
@@ -170,10 +177,10 @@ test('test_transient_identity', async () => {
 test('test_identity_with_transient_traits', async () => {
   fetch.mockResolvedValue(new Response(identityWithTransientTraitsJSON));
   const identifier = 'transient_trait_identifier';
-  const traits = { 
+  const traits = {
     some_trait: 'some_value',
     another_trait: {value: 'another_value', transient: true},
-    explicitly_non_transient_trait: {value: 'non_transient_value', transient: false} 
+    explicitly_non_transient_trait: {value: 'non_transient_value', transient: false}
   }
   const traitsInRequest = [
     {
@@ -206,3 +213,12 @@ test('test_identity_with_transient_traits', async () => {
   expect(identityFlags[0].value).toBe('some-identity-with-transient-trait-value');
   expect(identityFlags[0].featureName).toBe('some_feature');
 });
+
+test('getIdentityFlags fails if API call failed and no default flag handler was provided', async () => {
+  const flg = flagsmith({
+    fetch: badFetch,
+  })
+  expect(flg.getIdentityFlags('user'))
+      .rejects
+      .toThrow('getIdentityFlags failed and no default flag handler was provided')
+})