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 4 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
51 changes: 37 additions & 14 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,33 @@ 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 fetching initial flags');
}
})
}
} else {
if (!preventFetch) {
try {
await this.getFlags();
} catch (e) {
this.log('Exception fetching flags', e);
throw new Error('Error fetching initial 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);
throw new Error('Error fetching initial flags');
}
} else {
if (defaultFlags) {
this._onChange(null,
Expand All @@ -534,7 +551,12 @@ const Flagsmith = class {
try {
const res = AsyncStorage.getItemSync? AsyncStorage.getItemSync(this.getStorageKey()) : await AsyncStorage.getItem(this.getStorageKey());
await onRetrievedStorage(null, res)
} catch (e) {}
} catch (e) {
if (!defaultFlags) {
this.log('Error getting item from storage', e);
throw e;
}
}
}
} else if (!preventFetch) {
await this.getFlags();
Expand All @@ -554,6 +576,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
2 changes: 1 addition & 1 deletion test/init.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ describe('Flagsmith.init', () => {
const { flagsmith, initConfig } = getFlagsmith({
onChange,
evaluationContext: { environment: { apiKey: '' } },
});
}, true);
await expect(flagsmith.init(initConfig)).rejects.toThrow(Error);
});
test('should sanitise api url', async () => {
Expand Down
3 changes: 2 additions & 1 deletion test/test-constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ 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) => {
Expand All @@ -91,6 +91,7 @@ export function getFlagsmith(config: Partial<IInitConfig> = {}) {
const initConfig: IInitConfig = {
AsyncStorage,
fetch: mockFetch,
environmentID: forceNoKey ? '' : environmentID,
...config,
};
initConfig.evaluationContext = {
Expand Down