Skip to content

fix: surface get flags initialization errors #323

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.

Sign up for GitHub

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
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

fix: surface get flags initialization errors #323

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e33a233
fix: catch-errors-and-not-initialize
Zaimwa9 Jun 27, 2025
b0badfa
fix: removed-style-changes
Zaimwa9 Jun 27, 2025
094e211
fix: error-wording
Zaimwa9 Jun 30, 2025
ead3e98
fix: restored-previous-wanted-behaviors
Zaimwa9 Jun 30, 2025
03e1184
fix: re-added-prevent-fetch-condition
Zaimwa9 Jun 30, 2025
bc06ce2
fix: added-test
Zaimwa9 Jul 2, 2025
71c170b
fix: wrapped-on-retrieved-storage-with-error
Zaimwa9 Jul 9, 2025
35d3a81
fix: removed-blank-line
Zaimwa9 Jul 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 54 additions & 16 deletions flagsmith-core.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,13 @@ import {
} from './types';
// @ts-ignore
import deepEqual from 'fast-deep-equal';
import { EvaluationContext } from './evaluation-context';
import angularFetch from './utils/angular-fetch';
import { AsyncStorageType } from './utils/async-storage';
import { ensureTrailingSlash } from './utils/ensureTrailingSlash';
import getChanges from './utils/get-changes';
import angularFetch from './utils/angular-fetch';
import setDynatraceValue from './utils/set-dynatrace-value';
import { EvaluationContext } from './evaluation-context';
import { isTraitEvaluationContext, toEvaluationContext, toTraitEvaluationContextObject } from './utils/types';
import { ensureTrailingSlash } from './utils/ensureTrailingSlash';

enum FlagSource {
"NONE" = "NONE",
Expand Down Expand Up @@ -113,14 +113,14 @@ const Flagsmith = class {
const userTraits: Traits = {};
features = features || [];
traits = traits || [];
features.forEach(feature => {
features?.forEach(feature => {
flags[feature.feature.name.toLowerCase().replace(/ /g, '_')] = {
id: feature.feature.id,
enabled: feature.enabled,
value: feature.feature_state_value
};
});
traits.forEach(trait => {
traits?.forEach(trait => {
userTraits[trait.trait_key.toLowerCase().replace(/ /g, '_')] = {
transient: trait.transient,
value: trait.trait_value,
Expand Down Expand Up @@ -221,10 +221,11 @@ const Flagsmith = class {
this.getJSON(api + 'identities/?identifier=' + encodeURIComponent(evaluationContext.identity.identifier) + (evaluationContext.identity.transient ? '&transient=true' : '')),
])
.then((res) => {
this.evaluationContext.identity = {...this.evaluationContext.identity, traits: {}}
return handleResponse(res?.[0] as IFlagsmithResponse | null)
}).catch(({ message }) => {
const error = new Error(message)
this.evaluationContext.identity = { ...this.evaluationContext.identity, traits: {} };
return handleResponse(res?.[0] as IFlagsmithResponse | null);
})
.catch((err) => {
const error = new Error(err.message)
return Promise.reject(error)
});
} else {
Expand All @@ -238,7 +239,7 @@ const Flagsmith = class {
analyticsFlags = () => {
const { api } = this;

if (!this.evaluationEvent || !this.evaluationContext.environment || !this.evaluationEvent[this.evaluationContext.environment.apiKey]) {
if (!this.evaluationEvent || !this.evaluationContext.environment || !this.evaluationEvent[this.evaluationContext.environment.apiKey] || !this.initialised) {
return
}

Expand Down Expand Up @@ -317,6 +318,11 @@ const Flagsmith = class {
_triggerLoadingState,
applicationMetadata,
} = config;

if (!environmentID) {
throw new Error('`environmentID` and `api` cannot be empty');
}

evaluationContext.environment = environmentID ? {apiKey: environmentID} : evaluationContext.environment;
if (!evaluationContext.environment || !evaluationContext.environment.apiKey) {
throw new Error('Please provide `evaluationContext.environment` with non-empty `apiKey`');
Expand Down Expand Up @@ -498,22 +504,38 @@ const Flagsmith = class {
}
if (shouldFetchFlags) {
// We want to resolve init since we have cached flags

this.getFlags().catch((error) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kyle-ssg , I'm a bit unconfortable here due to this test:
test('should not reject but call onError, when the identities/ API cannot be reached with the cache populated'

Just to give you context. This PR is really specific to flagsmithOnFlagsmith, hence why you can find a check flagsmith !== defaultApi.

I don't want to change the generic SDK behavior.

However here it feels like there is some redundancy => Is there a reason why we don't check shouldFetchFlags a bit earlier and have if (cachePopulated && !shouldFetchFlags) ? A fallback reason ? Maybe we could deal with it in case of failure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, it would help me to understand what's the behavior we want in case of failure. While working on this, there are a couple of points that are unclear to me, especially concerning an API error.

Why don't we want to throw the errors directly and stop the initialization?

To me the cases we have are:

  • Default flags + API error => Initialize with default flags, trigger onError
  • Valid Cache + API error => Initialize with cache, trigger onError
  • API error => Retries and throw error

Well, it's a bit particular with FoF as for most of the flags we are dealing with the platform ones, that shouldn't be critical.

this.onError?.(error)
if (this.api !== defaultAPI) {
this.log('Error fetching initial cached flags', error)
throw new Error(`Error querying ${this.api} for flags`);
}
})
}
} else {
if (!preventFetch) {
await this.getFlags();
try {
await this.getFlags();
} catch (e) {
this.log('Exception fetching flags', e);
throw new Error(`Error querying ${this.api} for flags`);
}
}
}
} catch (e) {
this.log("Exception fetching cached logs", e);
}
} else {
if (!preventFetch) {
await this.getFlags();
try {
await this.getFlags();
} catch (e) {
this.log('Exception fetching flags', e);
if (this.api !== defaultAPI) {
this.log('Error fetching flags', e);
throw new Error(`Error querying ${this.api} for flags`);
}
}
} else {
if (defaultFlags) {
this._onChange(null,
Expand All @@ -533,11 +555,26 @@ const Flagsmith = class {
};
try {
const res = AsyncStorage.getItemSync? AsyncStorage.getItemSync(this.getStorageKey()) : await AsyncStorage.getItem(this.getStorageKey());
await onRetrievedStorage(null, res)
} catch (e) {}
try {
await onRetrievedStorage(null, res)
} catch (e) {
this.log("Error retrieving item from storage", e);
throw e;
}
} catch (e) {
if (!defaultFlags) {
this.log('Error getting item from storage', e);
throw e;
}
}
}
} else if (!preventFetch) {
await this.getFlags();
try {
await this.getFlags();
} catch (e) {
this.log('Error fetching flags', e);
throw new Error(`Error querying ${this.api} for flags`);
}
} else {
if (defaultFlags) {
this._onChange(null, { isFromServer: false, flagsChanged: getChanges({}, defaultFlags), traitsChanged: getChanges({}, evaluationContext.identity?.traits) }, this._loadedState(null, FlagSource.DEFAULT_FLAGS));
Expand All @@ -554,6 +591,7 @@ const Flagsmith = class {
}
} catch (error) {
this.log('Error during initialisation ', error);
this.initialised = false;
const typedError = error instanceof Error ? error : new Error(`${error}`);
this.onError?.(typedError);
throw error;
Expand Down
61 changes: 46 additions & 15 deletions test/init.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { waitFor } from '@testing-library/react';
import {defaultState, FLAGSMITH_KEY, getFlagsmith, getStateToCheck, identityState} from './test-constants';
import { defaultState, FLAGSMITH_KEY, getFlagsmith, getStateToCheck, identityState } from './test-constants';
import { promises as fs } from 'fs';

describe('Flagsmith.init', () => {
Expand Down Expand Up @@ -90,25 +90,28 @@ describe('Flagsmith.init', () => {
});
test('should reject initialize with identity no key', async () => {
const onChange = jest.fn();
const { flagsmith, initConfig } = getFlagsmith({
onChange,
evaluationContext: { environment: { apiKey: '' } },
});
const { flagsmith, initConfig } = getFlagsmith(
{
onChange,
evaluationContext: { environment: { apiKey: '' } },
},
true,
);
await expect(flagsmith.init(initConfig)).rejects.toThrow(Error);
});
test('should sanitise api url', async () => {
const onChange = jest.fn();
const { flagsmith,initConfig } = getFlagsmith({
api:'https://edge.api.flagsmith.com/api/v1/',
const { flagsmith, initConfig } = getFlagsmith({
api: 'https://edge.api.flagsmith.com/api/v1/',
onChange,
});
await flagsmith.init(initConfig)
await flagsmith.init(initConfig);
expect(flagsmith.getState().api).toBe('https://edge.api.flagsmith.com/api/v1/');
const { flagsmith:flagsmith2 } = getFlagsmith({
api:'https://edge.api.flagsmith.com/api/v1',
const { flagsmith: flagsmith2 } = getFlagsmith({
api: 'https://edge.api.flagsmith.com/api/v1',
onChange,
});
await flagsmith2.init(initConfig)
await flagsmith2.init(initConfig);
expect(flagsmith2.getState().api).toBe('https://edge.api.flagsmith.com/api/v1/');
});
test('should reject initialize with identity bad key', async () => {
Expand Down Expand Up @@ -278,7 +281,7 @@ describe('Flagsmith.init', () => {
const onChange = jest.fn();
const { flagsmith, initConfig, AsyncStorage, mockFetch } = getFlagsmith({
onChange,
applicationMetadata: {
applicationMetadata: {
name: 'Test App',
version: '1.2.3',
},
Expand All @@ -295,13 +298,12 @@ describe('Flagsmith.init', () => {
}),
}),
);

});
test('should send app name headers when provided', async () => {
const onChange = jest.fn();
const { flagsmith, initConfig, AsyncStorage, mockFetch } = getFlagsmith({
onChange,
applicationMetadata: {
applicationMetadata: {
name: 'Test App',
},
});
Expand All @@ -316,7 +318,6 @@ describe('Flagsmith.init', () => {
}),
}),
);

});

test('should not send app name and version headers when not provided', async () => {
Expand All @@ -338,4 +339,34 @@ describe('Flagsmith.init', () => {
);
});

test('should throw an environmentID missing error on init', async () => {
const onChange = jest.fn();
const { flagsmith, initConfig } = getFlagsmith(
{
onChange,
environmentID: '',
},
true,
);

await expect(flagsmith.init(initConfig)).rejects.toThrow(Error);
});

test('should throw an error and call onError when API is not reachable with custom api URL', async () => {
const onError = jest.fn();
const customApi = 'https://wrong-host.com';
const { flagsmith, initConfig } = getFlagsmith({
api: customApi,
cacheFlags: false,
fetch: async () => {
return Promise.resolve(new Error('Mocked fetch error'));
},
onError,
});

const initPromise = flagsmith.init(initConfig);
await expect(initPromise).rejects.toThrow(new Error(`Error querying ${customApi}/ for flags`));
expect(onError).toHaveBeenCalledTimes(1);
expect(onError).toHaveBeenCalledWith(new Error(`Error querying ${customApi}/ for flags`));
});
});
57 changes: 31 additions & 26 deletions test/test-constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,11 @@ import Mock = jest.Mock;
import { promises as fs } from 'fs';

export const environmentID = 'QjgYur4LQTwe5HpvbvhpzK'; // Flagsmith Demo Projects
export const FLAGSMITH_KEY = 'FLAGSMITH_DB' + "_" + environmentID;
export const FLAGSMITH_KEY = 'FLAGSMITH_DB' + '_' + environmentID;
export const defaultState = {
api: 'https://edge.api.flagsmith.com/api/v1/',
evaluationContext: {
environment: {apiKey: environmentID},
environment: { apiKey: environmentID },
},
flags: {
hero: {
Expand All @@ -24,25 +24,25 @@ export const defaultState = {
},
};

export const testIdentity = 'test_identity'
export const testIdentity = 'test_identity';
export const identityState = {
api: 'https://edge.api.flagsmith.com/api/v1/',
identity: testIdentity,
evaluationContext: {
environment: {apiKey: environmentID},
environment: { apiKey: environmentID },
identity: {
identifier: testIdentity,
traits: {
string_trait: {value: 'Example'},
number_trait: {value: 1},
}
}
string_trait: { value: 'Example' },
number_trait: { value: 1 },
},
},
},
flags: {
hero: {
id: 1804,
enabled: true,
value: 'https://s3-us-west-2.amazonaws.com/com.uppercut.hero-images/assets/0466/comps/466_03314.jpg'
value: 'https://s3-us-west-2.amazonaws.com/com.uppercut.hero-images/assets/0466/comps/466_03314.jpg',
},
font_size: { id: 6149, enabled: true, value: 16 },
json_value: { id: 80317, enabled: true, value: '{"title":"Hello World"}' },
Expand All @@ -53,10 +53,10 @@ export const identityState = {
export const defaultStateAlt = {
...defaultState,
flags: {
'example': {
'id': 1,
'enabled': true,
'value': 'a',
example: {
id: 1,
enabled: true,
value: 'a',
},
},
};
Expand All @@ -72,38 +72,43 @@ export function getStateToCheck(_state: IState) {
return state;
}

export function getFlagsmith(config: Partial<IInitConfig> = {}) {
export function getFlagsmith(config: Partial<IInitConfig> = {}, forceNoKey = false) {
const flagsmith = createFlagsmithInstance();
const AsyncStorage = new MockAsyncStorage();
const mockFetch = jest.fn(async (url, options) => {
switch (url) {
case 'https://edge.api.flagsmith.com/api/v1/flags/':
return {status: 200, text: () => fs.readFile('./test/data/flags.json', 'utf8')}
return { status: 200, text: () => fs.readFile('./test/data/flags.json', 'utf8') };
case 'https://edge.api.flagsmith.com/api/v1/identities/?identifier=' + testIdentity:
return {status: 200, text: () => fs.readFile(`./test/data/identities_${testIdentity}.json`, 'utf8')}
return { status: 200, text: () => fs.readFile(`./test/data/identities_${testIdentity}.json`, 'utf8') };
}

throw new Error('Please mock the call to ' + url)
throw new Error('Please mock the call to ' + url);
});

//@ts-ignore, we want to test storage even though flagsmith thinks there is none
flagsmith.canUseStorage = true;
const initConfig: IInitConfig = {
AsyncStorage,
fetch: mockFetch,
environmentID: forceNoKey ? '' : environmentID,
...config,
};
initConfig.evaluationContext = {
environment: {apiKey: environmentID},
environment: { apiKey: environmentID },
...config?.evaluationContext,
}
};
return { flagsmith, initConfig, mockFetch, AsyncStorage };
}
export const delay = (ms:number) => new Promise((resolve) => setTimeout(resolve, ms));
export function getMockFetchWithValue(mockFn:Mock, resolvedValue:object, ms=0) {
mockFn.mockReturnValueOnce(delay(ms).then(()=>Promise.resolve({
status:200,
text: () => Promise.resolve(JSON.stringify(resolvedValue)), // Mock json() to return the mock response
json: () => Promise.resolve(resolvedValue), // Mock json() to return the mock response
})))
export const delay = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms));
export function getMockFetchWithValue(mockFn: Mock, resolvedValue: object, ms = 0) {
mockFn.mockReturnValueOnce(
delay(ms).then(() =>
Promise.resolve({
status: 200,
text: () => Promise.resolve(JSON.stringify(resolvedValue)), // Mock json() to return the mock response
json: () => Promise.resolve(resolvedValue), // Mock json() to return the mock response
}),
),
);
}